Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AssemblyInfo generation skipped on incremental build even if Version/VersionSuffix changes #967

Closed
dasMulli opened this issue Mar 9, 2017 · 10 comments
Milestone

Comments

@dasMulli
Copy link
Contributor

@dasMulli dasMulli commented Mar 9, 2017

Steps to reproduce:

  • dotnet new lib
  • dotnet restore
  • dotnet build
  • dotnet build /p:VersionSuffix=alpha
  • dotnet publish /p:Version=1.2.3
  • dotnet pack /p:VersionSuffix=beta

Expected behaviour:

The bin/Debug/netstandard1.4/tmp.AssemblyInfo.cs should be updated on every subsequent build/publish/pack command and the resulting assembly shall contain the requested attributes.

Actual behaviour:

detailed log contains:

Skipping target "CoreGenerateAssemblyInfo" because all output files are up-to-date with respect to the input files.

The assembly info is generated only once and only contains 1.0.0 / 1.0.0.0 versions. All published dlls contain this version (even the assembly contained in the npkg - the package is versioned correctly).

System:

C:\Users\martin.ullrich\Documents\tmp>dotnet --info
.NET Command Line Tools (1.0.0)

Product Information:
 Version:            1.0.0
 Commit SHA-1 hash:  e53429feb4

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.14393
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\1.0.0

Rationale

Since the inputs of CoreGenerateAssemblyInfo is limited to $(MSBuildAllProjects) it is skipped on incremental build when only the command line arguments change.
Maybe the file name can be changed to include the Version property so msbuild will see that the output tmp.1.0.0-beta.AssemblyInfo.cs is missing and run the target?

@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Mar 9, 2017

Hmm, this is tricky because there are other properties that impact the file. I think a complete solution would involve hashing all of those properties and putting it in file name or something.

cc @rainersigwald

@rainersigwald

This comment has been minimized.

Copy link
Contributor

@rainersigwald rainersigwald commented Mar 9, 2017

I think you're thinking in the right direction, though there's no easy way to deal with property changes from the command line in MSBuild in general. We added some things for MSBuild 15 that could probably be assembled in a reasonable way, mostly in microsoft/msbuild#1328.

@dasMulli

This comment has been minimized.

Copy link
Contributor Author

@dasMulli dasMulli commented Mar 10, 2017

Some context: this is nasty when building twice per CI build (one "CI drop" with a suffix and one "potentially releasable" package without suffix so a chained release definition might publish the suffix-less build).
Also, this is impacting tools that set the version(suffix/prefix) properties. (e.g. https://github.com/dasMulli/SimpleGitVersion)

Sadly, WriteCodeFragment doesn't have a WriteOnlyWhenDifferent parameter 😢 , so one could either do the hashing or reimplement the assembly info generation using WriteLinesToFile by generating line items for c# and vb 😱

I'm wondering if String.GetHashCode() is actually enough for this - instead of using a proper HashAlgorithm implementation...

@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Mar 10, 2017

String.GetHashCode is not good enough, https://msdn.microsoft.com/en-us/library/jj152924(v=vs.110).aspx is on by default in CoreCLR.

In general, an incremental build with different properties is risky, every target is potentially vulnerable to bugs like this.

We should fix this, but I would still caution against a CI setup as described where RTM packages (and only RTM packages!) are shipped from incremental builds.

@dasMulli

This comment has been minimized.

Copy link
Contributor Author

@dasMulli dasMulli commented Mar 11, 2017

[UseRandomizedStringHashAlgorithm] is on by default in CoreCLR.

Silly me.

I've had some success prototyping the hash approach by creating a Sha256Hash task and using it like this:
https://github.com/dasMulli/sdk/blob/feature/incremental-assembly-attributes/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.GenerateAssemblyInfo.targets#L90-L99

If that is the way forward, I'd be happy to contribute it in a proper PR. Just have the following design questions:

  • The Sha256Hash task is quite specific. More users could benefit from it being available in all sdk based projects by naming it ~HashString and taking an Algorithm parameter (that has a default).
  • Should any of the new target names be "public" (=> not prefixed with an underscore) - _CalculateGeneratedAssemblyInfoFileName, _CalculateAssemblyAttributes
@dasMulli

This comment has been minimized.

Copy link
Contributor Author

@dasMulli dasMulli commented Mar 18, 2017

Went a different route using msbuild's builtin Hash task..

@livarcocc

This comment has been minimized.

Copy link
Member

@livarcocc livarcocc commented Apr 19, 2017

@nguerrera @dsplaisted any concerns with @dasMulli suggest approach? I would love to have a PR for this if possible. This has come up quite a few times.

@livarcocc livarcocc modified the milestones: 1.1, 2.0 Apr 19, 2017
@dasMulli

This comment has been minimized.

Copy link
Contributor Author

@dasMulli dasMulli commented Apr 19, 2017

@livarcocc I've already opened a PR (#1007) but it looks like some discussion is still needed.

@dasMulli

This comment has been minimized.

Copy link
Contributor Author

@dasMulli dasMulli commented Apr 19, 2017

Having microsoft/msbuild#1949 would probably also help quite a few users.

@dasMulli

This comment has been minimized.

Copy link
Contributor Author

@dasMulli dasMulli commented May 26, 2017

Fixed by #1255

@dasMulli dasMulli closed this May 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.