minver
minver copied to clipboard
adamralph
Use a custom MSBuild task
This will replace the use of the Exec task to run MinVer.dll as an executable, with the use of a custom task within that assembly. This is dependent on either #256, or changes in LibGit2Sharp which @bording is aware of.
One functional effect of this change is that the MSBuild verbosity level will be respected, which means that MinVerVerbosity will be ignored (probably deprecated initially, with a warning message). This could be considered a breaking change. It will also mean that the calculated version will not be outputted to the console (when the MSBuild verbosity level is detailed) as a side effect of using the Exec task.
I've been thinking about the pros and cons of introducing this in 1.0.0, but still delegating to an executable for now. In order to do proper logging, the executable can write to stderr with prefixes that indicate the log level, so that the task can parse them and propagate them back to the MSBuild logger.
Pros
- removes stdout noise
- removes the need for
MinVerVerbosity(the logging level will be inherited from MSBuild) - nicer errors than with the
Exectask - simplifies targets file
Cons
- involves managing System.Process
- requires listening to stderr and parsing (I'm not sure how reliable this is)
- requires another project sitting between MinVer and MinVer.Lib, and more complex packaging
involves managing System.Process requires listening to stderr and parsing (I'm not sure how reliable this is)
There is a ToolTask type that would be worth looking at to see if it helps with these.
requires another project sitting between MinVer and MinVer.Lib, and more complex packaging
It seems like it could be possible to use the minver-cli output instead of needing a separate project.
One thing to bear in mind is that there is no such thing as a "current log level" in the context of an MSBuild task, since the host process can have many different loggers attached to it, each configured to a different level. That means that every message, including all debug and trace messages, will have to be output to stderr, and parsed, on every run.
Also, I'm not entirely sure that we can just use minver-cli for this, since we'd also need a way of communicating the MINVERXXXX error and warning codes back to the task, which would either mean adding them to the message (either always or via a "hidden" option) or having some kind of convention based on the pattern of the text. Or, it could always be the same error code, but that seems clunky.
Putting aside the stdout noise (one line) and the slightly uglier error messages, I believe the only significant functional difference here is the removal of MinVerVerbosity, and allowing each of the configured loggers to choose which level of messages it receives. I'm leaning toward that not being enough of a benefit to outweigh the costs.
The fact that there is no way to know the configured log level is rather annoying.
I agree with your assessment that it doesn't make sense to go ahead and try for a task now just to get rid of MinVerVerbosity.
I'd say this can wait until it's feasible to actually use LibGit2Sharp directly from an MSBuild task.
Another thing to consider is that currently, MinVer is completely platform agnostic. I.e. it will work for any target framework supported by SDK-style projects. If we introduce a task assembly, then target frameworks start to become something to worry about as detailed in https://natemcmaster.com/blog/2017/07/05/msbuild-task-in-nuget/
To cover everything currently supported, net461;netstandard2.0 should be enough.
@bording it shows that we will lose platform support by doing this. Of course, net20 isn't relevant, but perhaps some others may be.
The target platforms of the projects using MinVer aren't relevant for this. What matters is the version of .NET that VS and the SDK are using.
Since we only support SDK-style projects, that means VS 2017, so for VS and its MSBuild, we need net461. (Or possibly net46. I'd need to doublecheck to be 100% on that). For building from the .NET Core SDK, we need something that works with .NET Core 2.x, which is where the netstandard2.0 assembly comes in.
Building two task assemblies, net461;netstandard2.0, means we've covered all the bases, and nothing is lost.
@ursenzler is it correct that you discovered that the target platform of the project does matter?
@adamralph yes. You cannot use a build task with target netstandard2.0 (the target the MinVerTask would have due to LibGit2) in a project targeting netstandard1.0 for example. https://twitter.com/terrajobst/status/1064589484740603906
@ursenzler If you're using the GitVersion package has an example, then I think you might be misunderstanding what's going on because of some choices made by the GitVersion maintainers. The latest version of that package actually has dependencies, and that's going to be what't blocking the package installation on a netstandard1.0 project. It has nothing to do with what the underlying build task is built against.
If done properly, a versioning package like this should have no dependencies, so it will work in any project regardless of the target framework.
It does, but that's not relevant here because the package will never have a LibGit2Sharp dependency. It will always be bundled inside the package, and will never creep into your projects as a dependency.
This would probably allow https://github.com/adamralph/minver/issues/244.
@bording I'm leaning towards closing this as a wontfix.
The only benefit I can think of is the removal of MinVerVerbosity and I don't think many people (myself included) care much about it.
There are several costs I can think of:
- The package will have to be multi-targetted for
net461andnetstandard2.0. - Depending on how MSBuild develops in the future, those TFM's may need to change again, which will lead to more releases and versioning considerations.
- The code will increase in complexity, since the MinVer package will have to switch to using the MSBuild task API, while the minver-cli package will remain as a CLI.
Now that you're no longer using LibGit2Sharp, I agree that there's less of a reason to do this. Even with a custom task, you're still calling a separate process and having to parse output.
Re-opening based on https://github.com/adamralph/minver/issues/940 /cc @bording