Use a custom MSBuild task

[adamralph]

Use a custom MSBuild task

[bording]

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.

[bording]

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.

[adamralph]

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 Exec task
• 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

[adamralph]

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.

[adamralph]

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/

[bording]

To cover everything currently supported, net461;netstandard2.0 should be enough.

[adamralph]

I tested MinVer in a net20 project and it worked. 😉

[bording]

@adamralph While that's cool, I'm not sure how that is relevant?

[adamralph]

@bording it shows that we will lose platform support by doing this. Of course, net20 isn't relevant, but perhaps some others may be.

[bording]

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.

[adamralph]

@ursenzler is it correct that you discovered that the target platform of the project does matter?

[ursenzler]

@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

[bording]

@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.

[ursenzler]

@bording true, but MinVer has a dependency to LibGit2, doesn't it?

[bording]

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.

[adamralph]

This would probably allow #244.

[adamralph]

No longer blocked by LibGit2Sharp, but unsure if I want to do it anyway.

[adamralph]

@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 net461 and netstandard2.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.

[bording]

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.

[adamralph]

Re-opening based on #940 /cc @bording