Include SourceRevisionId in InformationalVersion #2028

tmat · 2018-03-08T00:17:52Z

When a source control provider is available in the project append the value of SourceRevisionId set by the provider to InformationalVersion string.

See https://github.com/tmat/repository-info/blob/master/docs/Readme.md and dotnet/msbuild#3063 for details.

tmat · 2018-03-08T00:18:49Z

@jaredpar @nguerrera @AArnott PTAL

tmat · 2018-03-08T00:21:33Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets

+          DependsOnTargets="InitializeSourceControlInformation"
+          Condition="'$(IncludeSourceRevisionInInformationalVersion)' == 'true'">
+    <PropertyGroup>
+      <InformationalVersion>$(InformationalVersion) (revision $(SourceRevisionId))</InformationalVersion>


Is there a better syntax to use for the revision? I believe Nerdbank.GitVersioning uses g+ and assume g stands for git. SourceRevisionId could be commit hash, TFVC changeset number, etc.

Almost. NB.GV uses this syntax: 1.2.3+gabc123. Yes, g stands for git.

If we haven't any precedent for $(SourceRevisionId) values yet, perhaps we can indicate they should assume that they are appending some + value to the version, semver 2 style. Just because technically there can be multiple build metadata identifiers specified, they should probably not specify + themselves (we should do that, or add . if there is already a + in the $(InformationalVersion) property).

Is the g an official/standard thing? I've gotten tripped up by it, pasting it as part of the hash. After a minute, I realized g isn't valid hex but I'd rather the Sha appear with better punctuation around it. I'll back away if there's an industry effort to write versions that way.

Yeah, i would not include the g in the version. Just x.y.z+sha seems good enough.

It wasn't my idea, but I can't remember exactly where I'd seen the precedent for it. At the time I figured I'd follow the pattern others had established. It is a little inconvenient, I agree.

AArnott · 2018-03-08T03:01:36Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets

+          DependsOnTargets="InitializeSourceControlInformation"
+          Condition="'$(IncludeSourceRevisionInInformationalVersion)' == 'true'">
+    <PropertyGroup>
+      <InformationalVersion>$(InformationalVersion) (revision $(SourceRevisionId))</InformationalVersion>


Where does $(SourceRevisionId) get set? And if it's empty, shouldn't you avoid appending to $(InformationalVersion) to avoid leaving an obvious hole in the added string?

Ah, yes, forgot to add SourceRevisionId != '' to the condition.
It's set by InitializeSourceControlInformation target that is provided by a source control package.

We should consider doing nothing if informational version already contains revision id. This allows custom formatting and will play better with pre existing techniques like nb.gv, right?

How do we detect that though? Look for + in InformationalVersion?

I was thinking $(InformationalVersion.Contains($(SourceRevisionId))

But that isn't foolproof due to ordering and possible SHA abbreviation.

But that isn't foolproof due to ordering and possible SHA abbreviation.

Yup, that's the problem.

I thought about nb.gv interop as well (thanks, @nguerrera). It won't be a problem because nb.gv already turns generation of this attribute off completely to avoid conflicts with the one it generates itself.

AArnott · 2018-03-08T03:05:03Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets

+          DependsOnTargets="InitializeSourceControlInformation"
+          Condition="'$(IncludeSourceRevisionInInformationalVersion)' == 'true'">
+    <PropertyGroup>
+      <InformationalVersion>$(InformationalVersion) (revision $(SourceRevisionId))</InformationalVersion>


If we haven't any precedent for $(SourceRevisionId) values yet, perhaps we can indicate they should assume that they are appending some + value to the version, semver 2 style. Just because technically there can be multiple build metadata identifiers specified, they should probably not specify + themselves (we should do that, or add . if there is already a + in the $(InformationalVersion) property).

nguerrera · 2018-03-08T17:12:13Z

We should add a test with mock provider targets. Shouldn't be too hard to base it on existing assembly info tests.

tmat · 2018-03-08T22:31:07Z

@nguerrera Yes, I'll definitely add a test

nguerrera · 2018-03-09T22:00:04Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets


+  <Target Name="AddSourceRevisionToInformationalVersion"
+          DependsOnTargets="InitializeSourceControlInformation"
+          Condition="'$(IncludeSourceRevisionInInformationalVersion)' == 'true' and '$(SourceRevisionId)' != ''">


I believe build will break here if msbuild version used does not include your change and SourceRevisionId is set. I'm not sure we want such a hard break. We may need to stub InitializeSourceControlInformation in props.

I think we are saying that 15.7+ is required for 2.1.300, though, so maybe it's fine. We need to think about it carefully and be deliberate. cc @livarcocc

One way to have a 'soft dependency' is change DependsOnTargets to AfterTargets. That doesn't fail if the target is missing. But it also doesn't guarantee that the antecedent runs first -- it just will in the common case.

@AArnott We need the guarantee that InitializeSourceControlInformation runs.

@nguerrera Yes, I agree we need to be careful about the dependency. Let's discuss it next week.

@nguerrera I have an idea how to avoid the break. Let me check with msbuild team.

tmat · 2018-03-15T01:30:10Z

@dsplaisted @nguerrera PTAL

dsplaisted · 2018-03-15T17:38:19Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets

          Condition="'$(GenerateAssemblyInfo)' == 'true'" />

+  <Target Name="AddSourceRevisionToInformationalVersion"
+          DependsOnTargets="InitializeSourceControlInformation"


This should probably also depend on the GetAssemblyVersion, which is where InformationalVersion is set by default. It currently works because of the ordering of DependsOnTargets for GetAssemblyAttributes, but it's probably better to be more explicit.

dsplaisted · 2018-03-15T17:40:35Z

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToControlGeneratedAssemblyInfo.cs

+                    project.Root.Add(
+                        new XElement(ns + "PropertyGroup",
+                            new XElement("SourceControlInformationFeatureSupported", "true"),
+                            new XElement("IncludeSourceRevisionInInformationalVersion", "true")));


Nit: You can add these properties to the AdditionalProperties of the TestProject instead of creating the XML nodes yourself here.

Since the IncludeSourceRevisionInInformationalVersion property is set to true by default in the SDK, is there a reason to set it in this test?

dsplaisted · 2018-03-15T17:43:07Z

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToControlGeneratedAssemblyInfo.cs

+        }
+
+        [Fact]
+        public void It_does_not_include_source_revision_id_if_not_available2()


For tests where the name only differs by a number appended to it, I'd like to see comments summarizing what the tests cover / how they are different.

Changed the names.

dsplaisted · 2018-03-15T17:43:56Z

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToControlGeneratedAssemblyInfo.cs

+        }
+
+        [Fact]
+        public void It_includes_source_revision_id_if_available__version_without_plus()


Nit: Looks like there's an extra underscore between "available" and "version" in the name here.

On the other hand, perhaps this was intentional to separate the different parts of the test name.

You might consider changing these tests to a Theory.

The tests define different XML elements. I'd rather leave them separate.

dsplaisted · 2018-03-15T17:46:25Z

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToControlGeneratedAssemblyInfo.cs

+
+                    project.Root.Add(
+                        new XElement(ns + "PropertyGroup",
+                            new XElement("SourceControlInformationFeatureSupported", "true")));


Please file a follow-up bug for us to remove the SourceControlInformationFeatureSupported property and InitializeSourceControlInformation target from all of these tests once we are using a version of MSBuild that includes them in all places where these tests run (the last one will probably be the full Framework version of MSBuild that we use on CI machines).

That's an interesting point. Can we run tests against older msbuild common targets? Ideally we would test against the current and a fixed previous version that doesn't have these.

I don't think we need to. Generally for customers to be using an SDK with this feature, they will be using an updated version of MSBuild. The only way they wouldn't have an updated version of MSBuild is if they were using full Framework MSBuild and installed the 2.1.300 SDK, but hadn't updated VS/MSBuild to 15.7. That combination might be blocked otherwise anyway.

If that's the case wouldn't we want to remove the condition on SourceControlInformationFeatureSupported from the product as well?

We need it there for now, as the updated version of MSBuild hasn't made its way into the SDK. I would leave it there in general, in order to follow the recommended pattern for hooking into the extension point (since others who use it can't depend on shipping together with MSBuild), and in order to not block the 2.1.300 SDK / MSBuild 15.6 combination just because of this.

Filed #2051

dsplaisted · 2018-03-15T17:47:38Z

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToControlGeneratedAssemblyInfo.cs

+
+            testAsset.Restore(Log, testProject.Name);
+
+            var command = new GetValuesCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name), "netcoreapp2.0", valueName: "InformationalVersion");


Use testProject.TargetFrameworks instead of hardcoding netcoreapp2.0 as the target framework to pass to the GetValuesCommand method (here and elsewhere).

dsplaisted · 2018-03-15T17:53:57Z

@tmat I've reviewed this. Thanks!

tmat · 2018-03-16T01:33:53Z

@dsplaisted Good to merge?

…114.3 (#2028) [main] Update dependencies from dotnet/arcade

tmat changed the title ~~Include SourceRevisionId to InformationalVersion~~ WIP: Include SourceRevisionId to InformationalVersion Mar 8, 2018

tmat commented Mar 8, 2018

View reviewed changes

tmat mentioned this pull request Mar 8, 2018

Add source control information extensibility point dotnet/msbuild#3063

Merged

6 tasks

AArnott reviewed Mar 8, 2018

View reviewed changes

AArnott approved these changes Mar 8, 2018

View reviewed changes

livarcocc added this to the 2.1.3xx milestone Mar 9, 2018

nguerrera reviewed Mar 9, 2018

View reviewed changes

tmat force-pushed the RevisionNumberInAIVA branch from 61fc0cb to 8c0efeb Compare March 14, 2018 19:57

Include SourceRevisionId in InformationalVersion

aee0d72

tmat force-pushed the RevisionNumberInAIVA branch from 8c0efeb to aee0d72 Compare March 15, 2018 01:16

tmat changed the title ~~WIP: Include SourceRevisionId to InformationalVersion~~ Include SourceRevisionId to InformationalVersion Mar 15, 2018

one more test

f527b31

tmat changed the title ~~Include SourceRevisionId to InformationalVersion~~ Include SourceRevisionId in InformationalVersion Mar 15, 2018

another test

890fa04

dsplaisted approved these changes Mar 15, 2018

View reviewed changes

PR feedback

76c24ce

dsplaisted merged commit 2e6c593 into dotnet:master Mar 16, 2018

KalleOlaviNiemitalo mentioned this pull request Nov 1, 2023

Why the product version has a long suffix? #18294

Open

JL03-Yue pushed a commit that referenced this pull request Mar 19, 2024

Update dependencies from https://github.com/dotnet/arcade build 20231…

ae20800

…114.3 (#2028) [main] Update dependencies from dotnet/arcade

tmat deleted the RevisionNumberInAIVA branch November 20, 2025 16:54


		testAsset.Restore(Log, testProject.Name);

		var command = new GetValuesCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name), "netcoreapp2.0", valueName: "InformationalVersion");

Include SourceRevisionId in InformationalVersion #2028

Include SourceRevisionId in InformationalVersion #2028

Uh oh!

Conversation

tmat commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguerrera commented Mar 8, 2018

Uh oh!

tmat commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tmat commented Mar 8, 2018 •

edited

Loading

AArnott Mar 9, 2018 •

edited

Loading