Add an MSBuild task to call MinVer

[bording]

This PR adds an MSBuild task assembly that is used to call MinVer instead of using the built-in Exec task that was used previously.

This custom task lets us cache the result of the MinVer call, so that MinVer only needs to be invoked once per build.

The cache key uses all of the relevant MinVer input parameters, so if projects are configured to use different values, they will properly not share cache results.

This is the first step of addressing #112.

[adamralph]

Thanks very much @bording, I'm in the process of reviewing.

[slang25]

This looks really nice, is it still be reviewed?

[adamralph]

This looks really nice, is it still be reviewed?

@slang25 it's taken me a while to get to this. I'm currently investigating an alternative – #1021

[slang25]

@adamralph very cool. I think exposing the cache is a nice idea and lowers the barrier to entry for this sort of change.

For me, it still makes sense to move MinVer into its own task though, cause at the moment when there's no cache it needs to spin up a new process, and it needs to have a good dotnet on the path, and the dotnet runtime needs to start, and then the MinVer spins up git processes. Overall a lot of moving parts, so I was keen to see it move into a MSBuild task (with caching of course). But the proposed lightweight change is a welcome one too.

[adamralph]

@slang25 thanks for chiming in.

it needs to have a good dotnet on the path

The .NET SDK has to be present to be building a project in the first place. If there is no dotnet in PATH, it's difficult to see how a build process could be running in the first place, so I think this is a moot point.

MinVer spins up git processes

Note that this PR isn't changing that. A switch to libgit2sharp is a separate concern. But that has costs and risks of it's own, and I believe the performance increase would be incremental in comparison with how much caching buys us.

when there's no cache it needs to spin up a new process…and the dotnet runtime needs to start, and then the MinVer spins up git processes

That's true, but that seems to be very fast. E.g. if I run minver-cli from the command line (which is the equivalent of what the current MinVer target is doing) on fresh git repo (with no commits) it returns almost instantly. So again, I think the performance benefit would be incremental, perhaps even insignificant.

More generally, I appreciate that we can squeeze out more performance by switching to in-process invocation in a task, and in-process git manipulation using libgit2sharp, and without caching, those changes would probably make a big difference to performance when building large solutions. But I believe that, after caching is introduced, these would be little more than micro-optimisations. The current out of process approach has a lot of benefit, not least because the .NET Framework-based build that VS invokes does not have any isolation of dependencies. I.e. if something else in the build process had different versions of the same dependencies as the MinVer task, bad stuff could, and probably would, happen. I wouldn't even know how to go about resolving that, and I'm not sure I even know how to test and/or debug a custom task. It feels like something best avoided if possible, but I'm willing to go down that path with a very lightweight custom task that does the absolute minimal possible to give the biggest performance increase, and that's exactly what #1021 is.

[slang25]

@adamralph you have me convinced 🙂

My dotnet comment has never been an issue to be clear, more of a hypothetical one, and like you say a moot point.

It might be worth looking at perf again, I have a local branch that uses GitReader, but I have one last annoying bug (related to annotated tags). Although now that libgit2sharp is being maintained again it's probably best to use that.

[adamralph]

@slang25 for me a switch from out of process Git invocation to in-process libgit2sharp is similar to the switch out of process MinVer invocation to in-process invocation in a task. I.e. after caching is in place, in the vast majority of scenarios it would be an incremental performance gain, tending towards micro-optimisation, and not worth doing.

[adamralph]

closing in favour of #1021