Bugfix/logging

[tpaxatb]

Updates the GitVersionTask MSBuild task logging to consolidate the code and fix the problem when the tasks are hosted and reused inside Visual Studio devenv. Applies to #1512 and #1643

[arturcic]

There is a PR in progress related to GitVersionTask #1634, can you have a look and see if you can adapt to those changes?

[tpaxatb]

I just looked at the code for pull request for that, and see that my changes are not applicable at all (in other words, any merge of this code to the branch stazz/NuGetUtilsUpdate should be ignored).

The only requirement I would see is that the host of the calls to Execute methods of each of the tasks in the GitVersionTask dll needs to ensure the calls are serialized so that Logger.SetLoggers/Logger.Reset pairs do not step on each other as they are not thread safe. If the same host calls two Executes at the same time, the logging can cause issues (i.e. Reset being called while something else is still logging, etc).

I don't know what the roadmap is, but if this new NuGetUtilsUpdate branch is going to be pulled into 5.0, then this pull request is irrelevant and the PRs #1512 and #1643 will be closed at that point. But if the NuGetUtilsUpdate branch is going to be delayed until the next cycle, this pull should be incorporated.

[arturcic]

@asbjornu, @dazinator thoughts?

[asbjornu]

The reason I haven't merged #1634 is because I understand absolutely zip of it. I understand a bit more of this PR, but if this PR is made obsolete by merging #1634 and @tpaxatb vouches for merging it, I say we go on and merge it. If there are details in #1634 we need to discuss (such as #1634 (comment)), let's discuss that there (and not here).

There is nothing related to the roadmap of 5.0.0 that hinders us in merging either of these pull requests, just so that's clear.

[tpaxatb]

About all I can vouch for is that this request suppresses the resetting of the Core static Logger callbacks in the situation when the MSBuild task objects are reused. This behavior is exhibited in .wixproj files when being built within devenv, but not when being built by the command line. This behavior does not occur in .csproj/.vbproj references because those tasks are delegated by devenv to external msbuild processes.

An asside, this fix doesn't do anything to address my opinions about the logger being static in the Core, because by doing so it is making a pretty large assumption that only one object within the app domain at any one time; so it makes it inherently not thread safe. Nor does it address the fact that IMO, the fact that the Logger hasn't been set should throw an Exception at all. I just would like a version I can reference for our team to use that will not cause errors during a build when using visual studio and this seemed the quickest way to address that particular issue.

[asbjornu]

@tpaxatb, we're on the same page wrt. the logger (or pretty much anything, really) being static. I detest that keyword unless it's paired with private. What I don't quite understand is the relationship between this PR and #1634. Will the merge of #1634 make #1644 (this PR) obsolete?

[tpaxatb]

Ah sorry, yes, the merge of #1634 makes #1644 (this PR) completely obsolete. Both will solve and close the issues #1512 and #1643. If #1644 (this PR) is merged first, then whomever is authoring #1634 will face merge conflicts that should be ignored in favor of #1634 code. If #1634 is merged first, then #1644 will not be needed at all and the request can be closed without action.

[stazz]

@tpaxatb is correct (author of #1634 here). The #1634 makes GitVersion task completely independant on any MSBuild stuff, including loggers (Console.Out and Console.Error are used instead).

[asbjornu]

I see. Thanks for the explanation, @tpaxatb. I understand now that #1634 will remove the integration with MSBuild logging, which (as mentioned) is a regression. So it's not clear which way we should go here, I would appreciate more input from more users and maintainers of GitVersion.

[dazinator]

I think if this fix is needed right now, we should merge it and forward integrate PR #1634. Otherwise if @tpaxatb is happy to wait for #1634 to land in a future release, then we can put this PR on hold. It's less work for us to do the latter so that's my preference :-)

[tpaxatb]

Well to be frank (and slightly selfish), I would prefer having it implemented sooner than later, as it fixes the two issues that will remain closed with a forward integrated #1634. The reasoning being that I would rather be able to utilize a fixed version now, and waiting until a release with #1634 merged would have to make me develop something using the command line to generate the variables. I would even venture that it would be safe for 4.0.1 as long as 5.0 stable has it (actually I think this problem has existed for several years based on the log history). I have no opinion one way or the other with respect to the #1634 not utilizing MSBuild Tasks, as the internals of the GitVersionTask are, in my case, irrelevant. I just need a build task that works both from an MsBuild command line and from within Visual Studio.

Quick history on why this is important in my case: I want to use the GitVersionTask that is inserted prior to WixEnsureDirectoriesExist(?) task that generates the various GitVersion_* properties. After that is performed I have a custom BeforeCompileAndLink target in our WiX projects that then updates the property DefineConstants to include the values of those properties for the command lines used. Then, in our .wxs files, we can reference $(var.GitVersion_*) values and automagiclly use them without manual intervention or jumping through the hoops calling the external GitVersion executable and setting the values from the results of the output (which is, of course doable, but can potentially be time consuming)

In addition, I know at some point someone queried how to integrate GitVersion into their WiX projects, as well but no one knew a definitive answer...I would be willing to send something for a HOWTO on how to integrate GitVersion into a .wixproj...but that is not possible with the GitVersionTask in its current state since build failures occur within Visual Studio. If this is done so it works, it would be a matter of "Add the nuget package to your project, and at the end of the project file, copy-paste this and MAGIC!"

[dazinator]

@tpaxatb - ok understood. My vote then is to get this PR merged and for us to release a hotfix. If you could contribute to the docs a "how-to" for wix projects that would be awesome..

[asbjornu]

Ok, I kind of wish we didn't use trunk based development in GitVersion now. 😅 I suppose we need to create a support/4.0 branch from e715eb5 and merge this into it as well as to master and then tag a 4.0.1 release on the support branch. Will that cause new stable packages to be deployed everywhere, @arturcic, @dazinator? I don't know the build process intimately enough to say.

[arturcic]

I will have a look if we can have the 4.0.1 version built

[arturcic]

@tpaxatb have a question. Are you using the 4.0.0 version of the package or you're using the latest 5.0.0 beta bits?

@asbjornu if we create a branch based on e715eb5 then that version will not include the netstandard 2.0 feature. If @tpaxatb is using the latest bits I'd suggest to merge the PR so that it fixes the issues and he can start using the package, and we can continue the work on the final 5.0.0 version that will include the changes that @stazz is doing. I'd prefer doing this way as I don't think we can support 2 parallel versions.

[asbjornu]

If @tpaxatb is happy using a prerelease version, that wold make it much easier for us. Thanks for suggesting that, @arturcic.

[tpaxatb]

It's OK to use a prerelease version on my end. The 4.0.1 comment was more of a tongue in cheek comment on the irony of using master as the devlop branch (sorry...one of the reasons I want to use gitversion is I am trying to get our team out of the horrible habit of doing just that, they are so used to it as svn people) 😀

[arturcic]

Ok then, @tpaxatb please rebase on top of master and we'll get it merged

[arturcic]

@tpaxatb thank you for your contribution!