Skip to content

Allow control of AssemblyInformationVersion specifically - #528

Merged
nguerrera merged 1 commit into
dotnet:masterfrom
AArnott:dev/andarno/InformationalVersion
Dec 19, 2016
Merged

nguerrera merged 1 commit into
dotnet:masterfrom
AArnott:dev/andarno/InformationalVersion

Conversation

@AArnott

@AArnott AArnott commented Dec 17, 2016

Copy link
Copy Markdown
Contributor

The Nerdbank.GitVersioning package sets a bunch of assembly version information including the git commit ID in the AssemblyInformationVersion attribute. This fixes the build authoring so that this attribute's value in particular can be customized rather than being set only to the same value as the nuget package.

@AArnott
AArnott force-pushed the dev/andarno/InformationalVersion branch from 28fc3ae to 406aa9e Compare December 17, 2016 17:56

<PropertyGroup>
<FileVersion Condition="'$(FileVersion)' == ''">$(AssemblyVersion)</FileVersion>
<InformationalVersion Condition="'$(InformationalVersion)' == ''">$(AssemblyVersion)</InformationalVersion>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value used to come straight from $(Version), but this is changing the default to use $(AssemblyVersion). Is that intentional?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that looks incorrect. It should be $(Version)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I wasn't sure which would be better. I don't remember my reasoning for going this way so I'll switch to what you agree on.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$(Version) will change build-to-build. But $(AssemblyVersion) usually stays the same for an entire release.

Ex $(Version) = 4.0.0-build123
Ex $(AssemblyVersion = 4.0.0.0

@eerhardt

Copy link
Copy Markdown
Member

/cc @nguerrera @dsplaisted


<Target Name="CoreGenerateAssemblyInfo"
Condition="'$(Language)'=='VB' or '$(Language)'=='C#'"
DependsOnTargets="GetAssemblyVersion"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of having GetAssemblyVersion on GenerateAssemblyInfo and not here was to make it so that the AssemblyVersion is always available to CoreGenerateAssemblyInfo overrides for languages other than C# or VB.

Is moving this related to the feature requested here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's unrelated, please revert.

@nguerrera nguerrera left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I think it's a good change.

See my comment about moving GetAssemblyVersion dependency for the primary reason for my requesting changes status.

Also, I know how you feel about it in general, but please squash. I think it only obscures the simplicity of a +5,-3 change to split it over 4 commits.

@AArnott

AArnott commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

Agreed. I was considering a local squash and forced push myself, but wanted to see if there were concerns with the DependsOnTargets change.

@AArnott
AArnott force-pushed the dev/andarno/InformationalVersion branch from c5340c1 to c1468af Compare December 19, 2016 03:41
@AArnott

AArnott commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

@nguerrera I've made the changes you requested.

@AArnott

AArnott commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

BTW, for NB.GV to leverage this instead of just turning it off, it will have to know whether a project that is using it actually has this .targets imported. How can an imported .targets file determine that this is the new SDK/cross-targeting project? Is there a new property (value) set that I can condition on?

@nguerrera

Copy link
Copy Markdown
Contributor

@srivatsn in case this needs ask mode approval

@nguerrera

Copy link
Copy Markdown
Contributor

I don't think we have a good property set to use for that purpose. We should come up with one.

@nguerrera
nguerrera merged commit 8204b6b into dotnet:master Dec 19, 2016
@AArnott
AArnott deleted the dev/andarno/InformationalVersion branch December 19, 2016 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants