Incremental build improvements for generated AssemblyInfo.cs using inputs cache

[dasMulli]

Alternative to #1007 (fixes #967) using a mechanism similar to CoreCompileInputs.cache:

Splits CoreGenerateAssemblyInfo into:

1. GetAssemblyAttributes which creates all AssemblyAttribute items.
2. CreateGeneratedAssemblyInfoInputsCacheFile creates an obj/tfm/projectname.AssemblyInfoInputs.cache file containing the hash of all AssemblyAttribute items. This filename is available as $(GeneratedAssemblyInfoInputsCacheFile). The target uses msbuild 15's WriteOnlyWhenDifferent="True" feature for WriteLinesToFile so the file doesn't change unnecessarily.
3. CoreGenerateAssemblyInfo which generates the projectname.AssemblyInfo.cs file, using the cache file as input for incremental build.

cc @nguerrera @dsplaisted @rainersigwald

Also cc @enricosada @KevinRansom who want to use the refactor to overwrite CoreGenerateAssemblyInfo in F# specific targets (dotnet/fsharp#3113)

[nguerrera]

Much better. Thanks!

[rainersigwald]

Opinion nit: since "ownership" of both of these targets is in the same place, I'd prefer seeing the dependency expressed as a DependsOnTargets on CoreGenerateAssemblyInfo. Reading the dependencies in that direction "feels right" to me, in the absence of other factors.

[dasMulli]

The idea was that CoreGenerateAssemblyInfo can be overwritten in language-specific targets. Having ppl override and use a "private" target seemed a bit weird to me. And a CoreGenerateAssemblyInfoDependsOn property seemed a bit.. overblown.. - but would still be an option. Alternative would be to make the targets public. What do you think is better here?

[rainersigwald]

That's an interesting idea. I'm convinced!

[nguerrera]

Another option that's arguably more consistent with what we have now, but not necessarily better is to slot this in to the dependencies of outer GenerateAssemblyInfo. The target can also be public and called GetAssemblyAttributes matching GetAssemblyVersion.

[dasMulli]

Does the DependsOnTargets="A;B;C" set the build dependency of "b depends on a"?

Option: Merge _CreateGeneratedAssemblyInfoInputsCacheFile and _CalculateAssemblyAttributes into GetAssemblyAttributes , add DependsOnTargets="GetAssemblyAttributes" to CoreGenerateAssemblyInfo .

That would merge both concerns of emitting the items and writing the cache but leave nicely named public targets. @nguerrera your call

[rainersigwald]

@dasMulli

Does the DependsOnTargets="A;B;C" set the build dependency of "b depends on a"?

Depends on how precise you're being with your language 😁

DependsOnTargets="a;b;c" does guarantee an order, unless there's another factor like a DependsOnTargets="c". That happens because MSBuild uses a stack to order target execution, and encountering a target's DependsOnTargets pushes onto the stack in the given order. This is (almost) documented at https://docs.microsoft.com/en-us/visualstudio/msbuild/target-build-order.

There's another internal concept of dependency, though--when MSBuild considers a target and push prerequisites onto the stack, they get annotated with a "Parent". Then, if the target fails, the engine can additionally mark its parent as failed (and skip other targets that were building because of that parent). dotnet/msbuild#2133 is currently tweaking this for a corner case.

The latter is not done by an ordered DependsOnTargets--but that rarely matters.

[nguerrera]

Does the DependsOnTargets="A;B;C" set the build dependency of "b depends on a"?

No, but it ensures A,B,C ordering when it triggers them to run. IIRC, there is precedent in Common targets to slot CoreXxx in via DependsOnTargets of Xxx order without explicitly saying that CoreXxx depends on everything before it in the DependsOnTargets of Xxx. That was the idiom I had in mind when I first wrote this, but I think you're on to something nicer. We should make sure that GetAssemblyVersion is also sequenced by the same mechanism:

<Target Name="GenerateAssemblyInfo" 
        DependsOnTargets="PrepareForBuild;CoreGenerateAssemblyInfo"
        BeforeTargets="CoreCompile" ... />

<Target Name="GetAssemblyAttributes"
        DependsOnTargets="GetAssemblyVersion" .. />

<Target Name="_CreateGeneratedAssemblyInfoInputsCacheFile"
        DependsOnTargets="GetAssemblyAttributes"
        BeforeTargets="CoreGenerateAssemblyInfo"  ... />

[dasMulli]

I'm asking because a huge

<Target Name="GenerateAssemblyInfo" … DependsOnTargets="PrepareForBuild;GetAssemblyVersion;GetAssemblyAttributes;_CreateGeneratedAssemblyInfoInputsCacheFile;CoreGenerateAssemblyInfo" … />

works as well and I wanted to make sure it doesn't do so by accident. But it somehow feels morally wrong.

[nguerrera]

Nothing accidental, but it removes flexibility to invoke the individual targets via something other than GenerateAssemblyInfo and still have its dependencies satisfied. You've convinced me that the explicit approach is nicer.

Thinking further, given

1. Error handling subtleties of Fix issue where targets are executed after an error. msbuild#2133
2. That we need CoreGenerateAssemblyInfo overrides to re-declare the inputs and outputs. (I had put them on outer GenerateAssemblyInfo and that didn't work.)
3. That one of the inputs is the generated cache file

It makes sense for CoreGenerateAssemblyInfo to explicitly state that it depends on creating the cache file.

So here's my proposal:

<Target Name="GenerateAssemblyInfo" 
        DependsOnTargets="PrepareForBuild;CoreGenerateAssemblyInfo"
        BeforeTargets="CoreCompile" ... />

<Target Name="GetAssemblyAttributes"
        DependsOnTargets="GetAssemblyVersion" .. />

<Target Name="CreateGeneratedAssemblyInfoInputsCacheFile"
        DependsOnTargets="GetAssemblyAttributes" />

<Target Name="CoreGenerateAssemblyInfo" 
        DependsOnTargets="CreateGeneratedAssemblyInfoInputsCacheFile"
        Inputs="$(GeneratedAssemblyInfoCacheFile)"
        Outputs="$(GeneratedAssemblyInfoFile)" ... >

It's one more line to repeat in the CoreGenerateAssemblyInfo override, but it feels right next to the declared inputs.

[dasMulli]

@dotnet-bot test OSX10.12 Debug
@dotnet-bot test OSX10.12 Release

[dsplaisted]

@livarcocc @nguerrera Should we take this into release/2.0.0?

[nguerrera]

@dsplaisted Yes. We've gotten several different reports about the incremental build problem and F# needs the target refactoring.

[dsplaisted]

@dasMulli I've updated this PR to target the release/2.0.0 branch

[nguerrera]

You need to rebase this out.

[nguerrera]

Looks great. Thanks again, @dasMulli for your hard work on this and working through the complexities discussed on both PRs!

[nguerrera]

@MattGertz for approval

Customer scenario

Changing assembly version or other assembly metadata via CLI switches or msbuild global properties. Incremental builds would silently use the assembly metadata from a prior build.

Assembly metadata generation for F#. F# targets need to override a target for this and without the refactoring that was a prereq for this fix, they would have to duplicate all of the assembly attribute computation and not just implement writing them out in F# syntax. This would be fragile because F# would have to be kept in sync if any new attributes were added, etc.

Bugs this fixes:

#967

Workarounds, if any

Always rebuild when changing assembly metadata via command line. That assumes you're aware of the issue (since it's silently bad) and never forget about it. It also slows you down.

Duplicate a bunch of code from SDK targets in to F# targets.

Risk

Low.

Performance impact

Slight. There may be some overhead for the computation to do a correct up-to-date check, but it's needed for correctness.

Is this a regression from a previous update?

No

Root cause analysis:

Lack of test coverage for the scenario. Test gap is closed by the PR.

How was the bug found?

Reported (and fixed 😄) by customer.

[MattGertz]

Approved & thanks!

[dasMulli]

@dotnet-bot test Windows_NT_FullFramework Debug