MSBuild integration

[adamralph]

Essentially, making this work: https:/adamralph/min-ver#quick-start

• most of the work
• add the targets file to MSBuildAllProjects
• suppress the target for design-time builds - moved to Suppress the MinVer target for design time builds #55

[thefringeninja]

Hate to say it but I think you need an msbuild task here. https:/natemcmaster/Yarn.MSBuild might be an inspiration

[bording]

@adamralph There are two main ways you can integrate the logic into MSBuild:

1. Ship a command-line tool of some sort that gets called from an MSBuild target via an Exec task.
2. Create an MSBuild task that can be called from a target

Option 1 means you have to output all the needed values to the console and them parse them back out in your MSBuild targets, which can be messy.

Option 2 still means you could also ship a separate command-line tool if you want (for example as a .NET Core global tool), but the actual logic would be invoked directly as a task, and then you can get all the needed values back as regular properties.

Option 2 might seem like the best option, but there is a caveat here as well. Because you're using LibGit2Sharp, getting an MSBuild task working fully cross-platform is non-trivial. There are platform-specific native binaries that you have to deal with. MSBuild offers no assistance in making sure the correct one is loaded, so you have to handle all of that yourself. At the moment, that would involve some level of RID analysis to choose the correct binary. There are LibGit2Sharp changes coming "soon" that will make all of this more palatable, though.

Invoking a command-line tool instead could leverage .NET Core's ability to load the correct binary for you, so that's one reason that might be better. This is likely to be slower though, and this something needs to be invoked every time you build.

[thefringeninja]

I think option 2) is the the most viable. Publish a dotnet cli tool e.g. dotnet min-ver. Then, using the technique described here: https://natemcmaster.com/blog/2017/07/05/msbuild-task-in-nuget/ you could get that to invoke via dotnet build.

Also, I don't think slowness is an issue here... normally you would invoke this as part of your CI process, not on your dev machine.

[bording]

I think option 2) is the the most viable. Publish a dotnet cli tool e.g. dotnet min-ver. Then, using the technique described here: https://natemcmaster.com/blog/2017/07/05/msbuild-task-in-nuget/ you could get that to invoke via dotnet build.

You seem to be mixing things a bit. Option 1 would be the "invoke a command-line" approach, but you wouldn't really be able to tie that to a .NET Core global tool, since there would be no way to ensure the tool was installed.

The link you posted is detailing what I called Option 2, but that doesn't address any of the difficulties around the native libraries.

Also, I don't think slowness is an issue here... normally you would invoke this as part of your CI process, not on your dev machine.

I disagree. This is something you'll have installed in your project, and it will run as part of your build.

[thefringeninja]

Sorry if I wasn't more clear. What I am suggesting is that you can slip in an <Exec /> task via obj/$(MSBuildProject).nuget.g.props into BeforeBuild when the nuget package installs the cli tool. And of course this would require Lib2SharpGit to get updated.

[adamralph]

I'd like to avoid building an MSBuild task if possible, so right now I'm favouring option 1. I'm hoping it can be as simple as having an executable sitting next to the targets file.

I've pushed my initial naive attempt to https:/adamralph/min-ver-sandbox. (All the MinVer*.* artifacts are the binaries which result from building this repo.)

If you run build.cmd, you can see that MinVer is getting invoked, and it's calculating the version as 0.0.0-alpha.0, but I've no idea where to go from there.

One obvious problem is that <Exec ... ConsoleToMsBuild="true"> is picking up both stdout and stderr, even though the docs say that it should only contain stdout. All the lines which start with MinVer: are actually in stderr, and only the version number itself is in stdout.

Another thing I've observed is, if I set <VersionPrefix> in a <PropertyGroup> in the MinVer target, it has no effect on the package version, so I guess the target needs to be injected somewhere in the pipeline other than BeforeTargets="BeforeBuild".

The idea is that MinVer will indeed run on every build, not just on a CI server. I'm hoping performance won't be a problem.

[bording]

I'd like to avoid building an MSBuild task if possible, so right now I'm favouring option 1. I'm hoping it can be as simple as having an executable sitting next to the targets file.

Are you okay with requiring the .NET Core SDK to be installed? If not, you'll need to ship two different executables and choose which one to run, either .NET Core or .NET Framework.

I've pushed my initial naive attempt to https:/adamralph/min-ver-sandbox. (All the MinVer*.* artifacts are the binaries which result from building this repo.)

You'll need to package the dotnet publish output when/if you make this work for real. The sandbox repo doesn't have the dependencies.

One obvious problem is that <Exec ... ConsoleToMsBuild="true"> is picking up both stdout and stderr, even though the docs say that it should only contain stdout. All the lines which start with MinVer: are actually in stderr, and only the version number itself is in stdout.

Take a look at https://docs.microsoft.com/en-us/visualstudio/msbuild/exec-task?view=vs-2017. It says both standard error and standard output will be captured.

Another thing I've observed is, if I set in a in the MinVer target, it has no effect on the package version, so I guess the target needs to be injected somewhere in the pipeline other than BeforeTargets="BeforeBuild".

You'll also want to set PackageVersion property. Instead of VersionPrefix, it's also probably better to just use Version. It could also make sense to set some of the other assembly version properties, if you have different things you might want to have in there.

You should also change your for BeforeTargets to BeforeTargets="CoreCompile;GenerateNuspec"

That ensures the logic is run even with dotnet pack --no-build

[adamralph]

Are you okay with requiring the .NET Core SDK to be installed?

Yes, I believe that's an acceptable pre-requisite.

You'll need to package the dotnet publish output...

Thanks. FWIW, I switched the sandbox to that output in https:/adamralph/min-ver-sandbox/commit/27df9e8bf4aa6f7ebbff28431ca6bae03326255d

Instead of VersionPrefix, it's also probably better to just use Version.

If possible, I'd still like to allow the use of VersionSuffix to set build metadata, as described in the quick start, so that's why I was favouring VersionPrefix. If that's not possible, then it's not the end of the world.

You'll also want to set PackageVersion property.

I was hoping to avoid having to set PackageVersion explicitly, since I don't set that anywhere currently. I was hoping that VersionPrefix could be set at the right time for it, and VersionSuffix, to flow into PackageVersion automatically, as they currently do.

It could also make sense to set some of the other assembly version properties, if you have different things you might want to have in there.

Right now I'm hoping to just replace the setting of VersionPrefix and nothing else, since that will pick up the major, minor, patch, and pre-release identifiers, and VersionSuffix can continue to be used for build metadata via environment variables set on the CI server. I guess the other thing that could be interesting is the commit ID, but currently SourceLink is doing that for me, and tacking it onto the end of what is coming out of VersionPrefix and VersionSuffix and putting that into AssemblyInformationalVersion.

You should also change your for BeforTargets to BeforeTargets="CoreCompile;GenerateNuspec"

Thanks. I've changed it to that in https:/adamralph/min-ver-sandbox/commit/d08d4dbfc7ae2a0e2f8b3283d798137efe8d0567

So I guess the thing I'm still stuck on is working out which MSBuild incantations I need to get the version number out of stdout and into VersionPrefix, at the right time, so that everything else, i.e. putting it together with VersionSuffix, getting that into PackageVersion, allowing SourceLink to add the commit ID for AssemblyInformationalVersion, etc. all "just works". Whether this is achievable or not, I still don't know...

[adamralph]

Sadly it looks like VersionSuffix can't be used in the way I was hoping. It can't just contain build metadata, since MSBuild adds a hyphen between VersionPrefix and VersionSuffix when constructing Version. So I guess setting Version is indeed the right thing to do.

[bording]

I'm also not sure you'll be able to avoid setting PackageVersion since Version is being calculated.

It is set here: https:/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets#L28

But since that is in a top-level PropertyGroup, that means it's evaluated before you have a chance to set any version values in targets.

[bording]

can continue to be used for build metadata via environment variables set on the CI server.

Since MSBuild takes all environment variables and turns them into properties, you could still do this by appending that property when you set Version.

[adamralph]

But since that is in a top-level PropertyGroup, that means it's evaluated before you have a chance to set any version values in targets.

Aha, that explains it, thanks! Indeed, if I set PackageVersion in my custom target, that is used for the package. And if I inspect the assembly itself, the Version which I set in my custom target is being used for assembly version.

Since MSBuild takes all environment variables and turns them into properties, you could still do this by appending that property when you set Version.

I'd like to avoid giving MinVer knowledge of specific CI environments, but I guess this could be done the other way round: MinVer could look for an environment variable named MINVER_BUILD_METADATA.

[adamralph]

Woot! I've got it working, but in a very hacky way. It's using a file to capture stdout: https:/adamralph/min-ver-sandbox/commit/3b38e968e6c2cb049a25625ee0fe2d4eb4418b0d

I also threw SourceLink in there just to see how it behaves.

Another thing I've realised now is that Version cannot be set to the full version, with the pre-release identifiers. It has to be just the major.minor.patch, so the CLI tool will indeed have to output something richer than just a single version string.

[bording]

Another thing I've realised now is that Version cannot be set to the full version, with the pre-release identifiers.

Why is that? When I was testing it earlier, I was able to do that just fine.

[adamralph]

Try loading the assembly into ILSpy. It blows up. 😄

I guess ILSpy is making an assumption about the AssemblyVersion being of the form n.n.n.n, which may not be a CLR-enforced thing, but I think it's still better to play by the conventions.

[bording]

When I do that, everything looks fine. AssemblyVersion is just the numerical portion, as I would expect.

[adamralph]

Yeah, on closer inspection, the ILSpy exception stack trace suggests it might just be some other bug:

System.ArgumentException: Illegal characters in path.
   at System.IO.Path.CheckInvalidPathChars(String path, Boolean checkAdditional)
   at System.IO.Path.Combine(String path1, String path2, String path3, String path4)
   at ICSharpCode.Decompiler.DotNetCorePathFinder..ctor(String parentAssemblyFileName, String targetFrameworkId, String version, ReferenceLoadInfo loadInfo)
   at ICSharpCode.Decompiler.UniversalAssemblyResolver.FindAssemblyFile(AssemblyNameReference name)
   at ICSharpCode.ILSpy.LoadedAssembly.LookupReferencedAssemblyInternal(AssemblyNameReference fullName, Boolean isWinRT)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at ICSharpCode.ILSpy.LoadedAssembly.LookupReferencedAssembly(AssemblyNameReference name)
   at ICSharpCode.ILSpy.LoadedAssembly.MyAssemblyResolver.Resolve(AssemblyNameReference name)
   at ICSharpCode.Decompiler.TypeSystem.DecompilerTypeSystem..ctor(ModuleDefinition moduleDefinition, DecompilerSettings settings)
   at ICSharpCode.ILSpy.CSharpLanguage.DecompileAssembly(LoadedAssembly assembly, ITextOutput output, DecompilationOptions options)
   at ICSharpCode.ILSpy.TreeNodes.AssemblyTreeNode.Decompile(Language language, ITextOutput output, DecompilationOptions options)
   at ICSharpCode.ILSpy.TextView.DecompilerTextView.DecompileNodes(DecompilationContext context, ITextOutput textOutput)
   at ICSharpCode.ILSpy.TextView.DecompilerTextView.<>c__DisplayClass33_0.<DecompileAsync>b__0()

How are you seeing AssemblyVersion?

[bording]

I'm using dotPeek:

[adamralph]

OK, I just loaded it into ildasm and you're right, it is just the numeric part. Which makes sense, since you can also set a Version with pre-release identifiers, and the AssemblyVersion comes out correctly.

So I guess the next step is to find a less hacky way than piping stdout to a file and reading it back in.

Also I think this isn't going to work:

If your tags get into a mess and you can't find a way out, you can temporarily switch off MinVer and switch back to the old way of doing things by simply adding a Version element (or VersionPrefix and VersionSuffix elements) to your project. When you've figured out the problem, remove the element and MinVer will resume active duty.

I think MinVer will have to look for a specific property as an override, e.g. MinVerVersion.

[bording]

So I guess the next step is to find a less hacky way than piping stdout to a file and reading it back in.

Can you add a commandline parameter to skip all the other output, so you just get the value you need, simplifying the parsing?

Otherwise, you'll have to do all of that in the target.

[adamralph]

I'd quite like the ability to have some MinVer log messages in the output, e.g.

  MinVer: Detected { Version: 1.0.0, Commit: 27df9e8bf4aa6f7ebbff28431ca6bae03326255d, Height: 3 }.
  MinVer: 4 commits checked.                                                                       
  MinVer: Using { Version: 1.0.0, Commit: 27df9e8bf4aa6f7ebbff28431ca6bae03326255d, Height: 3 }.   
  MinVer: Calculated 1.1.0-alpha.0.3.                                                              

But until now, no actual parsing is required. I'm only using the file because I can't isolate stdout in the Exec task. All the above log messages go to stderr.

[adamralph]

FTR I found the bug in ILSpy. It didn't like when AssemblyInformationalVersion had spaces in it. It had spaces in it because MinVer.Cli.exe is doing a WriteLine() to stdout which results in a CRLF. Trimming the output fixed it.

[bording]

@adamralph I do and I will! 😄

[adamralph]

FWIW I've added a checklist to the description of this issue.

[bording]

@adamralph I've opened #53 to set the MSBuildAllProjects property.

I also look a look at the design-time build stuff, and it turns out that we need to let this run during design-time builds in order to get project references versioned correctly in the generated nuspec. I'm not entirely sure why that is. It feels like it might be a bug, but I need to do a bit more research before filing an issue about it on NuGet/Home.

[bording]

One last thing, are you sure https:/adamralph/min-ver/blob/e2f3f6662918ef5db7a50cc1caa3ae9b94a6f650/MinVer/MinVer.targets#L24 is needed? It seems like that line could be removed and nothing would change. At that point either MinVerVersion has the correct value, or it needs the build metadata appended. It shouldn't need to be set again.

[adamralph]

@bording thanks for looking into the design time build stuff. In that case, let's leave that for now. If you find out more then we can always re-open this issue or open a new one.

You're correct about that line in the targets. I think this was a badly edited copy-paste. I've fixed it in #54.

[adamralph]

In the end I decided to spin off the design time build suppression into #55.

[adamralph]

Full implementation (#55 notwithstanding) released in alpha 7.

Thanks @bording for all your help with this. It was invaluable!