Incremental build improvements for generated AssemblyInfo.cs #1007

dasMulli · 2017-03-18T14:37:51Z

Addresses #967 by:

Splitting CoreGenerateAssemblyInfo into:

_CalculateAssemblyAttributes which produces AssemblyAttribute items.
_CalculateGeneratedAssemblyInfoFileName which uses the present AssemblyAttribute items to product the name of the generated assembly info file, using the Hash task introduced in Improve incremental build in presence of globs msbuild#1328:
- VersionPrefix=1.0.0 => obj/Debug/netstandard2.0/lib1.AssemblyInfo.e32ef0.cs
- VersionPrefix=1.0.1 => obj/Debug/netstandard2.0/lib1.AssemblyInfo.5be252.cs
- All Generate*AssemblyAttribute set to false => obj/Debug/netstandard2.0/lib1.AssemblyInfo.cs

CoreGenerateAssemblyInfo then uses the generated file name as Outputs and is executed on incremental builds only when any of the AssemblyAttribute items change (even custom ones not generated by the SDK).

nguerrera · 2017-03-20T19:43:37Z

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


+    <PropertyGroup>
+      <GeneratedAssemblyInfoFile Condition=" '$(_AssemblyAttributeContentHash)' != '' ">
+        $(IntermediateOutputPath)$(MSBuildProjectName).AssemblyInfo.$(_AssemblyAttributeContentHash.SubString(0,6))$(DefaultLanguageSourceExtension)


While I appreciate the effort to keep line lengths down, the whitespace is going to be part of the property value, which will affect log readability, etc.

Is using only 6 chars of hash to avoid long path issues?

No, I was trying to figure out how much is actually needed to have a high probability of changing but not get too long and ugly. Since this is autogenerated and purely aesthetic (like short git commit hashes), i might was well use the full string.
Any preference?

rainersigwald · 2017-03-20T13:52:40Z

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

+    <PropertyGroup>
+      <GeneratedAssemblyInfoFile Condition=" '$(_AssemblyAttributeContentHash)' != '' ">
+        $(IntermediateOutputPath)$(MSBuildProjectName).AssemblyInfo.$(_AssemblyAttributeContentHash.SubString(0,6))$(DefaultLanguageSourceExtension)
+      </GeneratedAssemblyInfoFile>


I guess everything chomps input strings that are file paths, but I'd probably rather see this on one long awful line than have the newline+whitespace in the property.

rainersigwald · 2017-03-20T13:57:44Z

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

+            var assemblyPath = Path.Combine(incrementalBuildCommand.GetOutputDirectory(targetFramework).FullName, "HelloWorld.dll");
+            var info = AssemblyInfo.Get(assemblyPath);
+
+            info["AssemblyVersionAttribute"].Should().Be("1.2.4.0");


I'd also like to see a check that the previous hashed-name generated AssemblyInfo file is deleted (by IncrementalClean but that is an implementation detail) after changing it.

@rainersigwald thx, restructured the test completely.

…builds.

dasMulli · 2017-03-20T22:33:50Z

@dotnet-bot test Ubuntu16.04 Release please

Unable to obtain lock file access on '/tmp/NuGetScratch/lock/f031cb549b4aa7cbc2977507b297c62627fd4faf'

nguerrera · 2017-03-21T16:25:52Z

Regarding hash length, I'd prefer if we included all of it just to avoid any question of truncating too aggressively. I am a little concerned about the path length, though. I was thinking we could drop the project name since the file name is already unique for its contents. AssemblyInfo.[hash].cs would be shorter than TemporaryGeneratedFile_[GUID].cs that is dropped in the same directory. But then in the edge case where two libs building in parallel have the same assembly info and the same obj dir (currently blocked by project.assets.json conflict but there is a bug tracking fixing that), one might read it before the other is done writing it. Perhaps we drop AssemblyInfo or shorten it to Info instead, or include the project name (or even full path while we're at it) in the data hashed?

dasMulli · 2017-03-21T16:39:14Z

@nguerrera i see your concern. If it doesn't have to be human "understandable" at all, $(MSBuildProjectFullPath) could be added to the hash. Then i can also drop the condition for when there are no items to hash - in which case it falls back to the version without hash which would then collide with other projects using the same name and intermediate output path.

dasMulli · 2017-04-12T20:17:27Z

@nguerrera Seeing that the file name pops up in the compile error when colliding with attributes defined in user code (e.g. trying to add a custom assembly info file or not using the right Generate*Attribute properties), I think the project name should still show up in the file name.
Since the file name shouldn't grow too much, some truncation of the hash will be necessary.
Luckily, the possible entropy shrinks proportionally to how much of the hash is truncated. Since the purpose is not to avoid collisions, but to have some difference in between two inputs, I think the required entropy is extremely low. I haven't done any math on this though.

TL;DR After some thinking I believe the current form of this PR is what should be done, maybe add some more characters to the hash...
Any thoughts?

AArnott · 2017-04-22T17:09:41Z

Why do all the complex hashing stuff? Isn't it faster to just regenerate the file on every build? Sort of like package restore does for project.lock.json where it generates it at an alternate location and copies it over the one actually used to compile if its contents are different, thereby preserving a skip-build feature when the file doesn't actually need to change?

nguerrera · 2017-05-04T05:57:25Z

Isn't it faster to just regenerate the file on every build

That's not obvious to me. I'd like to see measurements. The I/O followed by a full comparison could be slower. @davkean was very concerned about this hitting the disk too much on every design-time build when it was first implemented.

nguerrera · 2017-05-04T06:03:56Z

On further thought, if we stick with hashing, we should not include a full source file path. The source file name will go in to the PDB and break PDB determinism if it changes only because the local path to the repo is different. (It can also break the assembly determinism if the PDB is embedded.) $(MSBuildProjectFile) is good enough because that's how other things in obj disambiguate themselves. It has never worked to have to projects with the same file name and the same obj directory.

nguerrera · 2017-05-04T06:09:01Z

Since the purpose is not to avoid collisions, but to have some difference in between two inputs

I don't follow. If there is no difference in the hash fragment between any two inputs, then those two inputs have colliding hash fragments. A collision of the hash fragments means that the wrong assembly info will be used.

davkean · 2017-05-04T09:12:47Z

A while back when I was looking at UI delays, I was seeing a non-trivial amount of tracers where I/O was extremely slooow, probably due to devs opening projects on network shares and the like. You'd hope that %TEMP% was fast (if that's where we'd generate the throwaway) - but if you can avoid hitting the disk that would be best.

dasMulli · 2017-05-04T09:40:35Z

I don't follow.

Sry that explanation of mine was sh*.
I meant to say that the chance of hitting a collision when changing the inputs is much lower than the chance of hitting a collision between two arbitrary inputs. That's why cutting the hash is (probably) appropriate to some degree.
Using the full hash will certainly result in maxpath exceptions for someone - i guess even if it's just [hash].cs - and i don't really like removing the project name from the generated file name, since the compiler may display errors for the file and i think that errors like Duplicate XX in [some-hash].cs are not helpful.. But i'm not very attached to this so @nguerrera it's your call :)

AArnott · 2017-05-08T22:51:08Z

probably due to devs opening projects on network shares and the like. You'd hope that %TEMP% was fast (if that's where we'd generate the throwaway) - but if you can avoid hitting the disk that would be best.

Devs who have projects on networks shares are doomed to a meaningless life of drudgery and boredom. Avoiding I/O during a build to cater to them is ... I mean come on, build is all about I/O.

Making design-time builds fast is goodness, of course.

dsplaisted

LGTM. @nguerrera are we ready to merge this or do we need to think more about the hash length vs path length limits?

nguerrera · 2017-05-19T17:08:34Z

How about using Base64 to encode the hash? Is that short enough to alleviate file length concerns? I don't want to be in the business of picking how many bits are good enough.

dasMulli · 2017-05-19T17:27:19Z

Base64 is a good idea. It would need 28 characters for a SHA1 hash.
Is there any telemetry about project file paths available?

Another alternative:
Leave output file name alone, but add a stub file that only has the hash as name (where the project name is part of the has).

nguerrera · 2017-05-19T17:28:28Z

I like "another alternative" a lot. :)

nguerrera · 2017-05-19T17:28:51Z

Then there's no sign of ugly name in PDBs, error messages, just in a stub file for bookkeeping.

dasMulli · 2017-05-19T17:41:22Z

That stub file could even be project.AssemblyInfo.cs.lock containing the hash using <WriteLinesToFile>'s new WriteOnlyWhenDifferent property ✨
(but I'll need to smoke test if that confuses VS design time build)

enricosada · 2017-05-25T08:02:38Z

Dont use lock extension on 'nix is used for lock files (things to not delete).

Maybe project.AssemblyInfo.cs.sha1 like nuget does

nguerrera · 2017-05-25T18:13:19Z

Closing in favor of #1255

…015.2 (dotnet#1007) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19515.2

dnfclas added the cla-already-signed label Mar 18, 2017

nguerrera reviewed Mar 20, 2017

View reviewed changes

rainersigwald reviewed Mar 20, 2017

View reviewed changes

dasMulli force-pushed the feature/incremental-assembly-attributes-using-hash branch from aa528ff to 59b01e7 Compare March 20, 2017 21:37

Make AssemblyInfo generation respect property changes on incremental …

f233bec

…builds.

dasMulli force-pushed the feature/incremental-assembly-attributes-using-hash branch from 59b01e7 to f233bec Compare March 20, 2017 21:41

This was referenced Apr 19, 2017

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

Closed

Generated AssemblyInfo.cs file should be regenerated after package restore #1142

Closed

dsplaisted approved these changes May 15, 2017

View reviewed changes

dsplaisted mentioned this pull request May 15, 2017

AssemblyInfo gets regenerated every design-time build regardless of whether anything has changed #778

Closed

dasMulli mentioned this pull request May 25, 2017

[WIP] add F# WriteCodeFragment dotnet/msbuild#2132

Closed

enricosada mentioned this pull request May 25, 2017

Generate F# assemblyinfo in new fsproj dotnet/fsharp#3113

Closed

dasMulli mentioned this pull request May 25, 2017

Incremental build improvements for generated AssemblyInfo.cs using inputs cache #1255

Merged

nguerrera closed this May 25, 2017

mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020

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

e5a59fc

…015.2 (dotnet#1007) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19515.2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incremental build improvements for generated AssemblyInfo.cs #1007

Incremental build improvements for generated AssemblyInfo.cs #1007

dasMulli commented Mar 18, 2017 •

edited

nguerrera Mar 20, 2017 •

edited

nguerrera Mar 20, 2017

dasMulli Mar 20, 2017

rainersigwald Mar 20, 2017

rainersigwald Mar 20, 2017

dasMulli Mar 20, 2017

dasMulli commented Mar 20, 2017

nguerrera commented Mar 21, 2017

dasMulli commented Mar 21, 2017 •

edited

dasMulli commented Apr 12, 2017

AArnott commented Apr 22, 2017

nguerrera commented May 4, 2017 •

edited

nguerrera commented May 4, 2017

nguerrera commented May 4, 2017

davkean commented May 4, 2017

dasMulli commented May 4, 2017

AArnott commented May 8, 2017

dsplaisted left a comment

nguerrera commented May 19, 2017

dasMulli commented May 19, 2017

nguerrera commented May 19, 2017

nguerrera commented May 19, 2017

dasMulli commented May 19, 2017 •

edited

enricosada commented May 25, 2017

nguerrera commented May 25, 2017

Incremental build improvements for generated AssemblyInfo.cs #1007

Incremental build improvements for generated AssemblyInfo.cs #1007

Conversation

dasMulli commented Mar 18, 2017 • edited

nguerrera Mar 20, 2017 • edited

Choose a reason for hiding this comment

nguerrera Mar 20, 2017

Choose a reason for hiding this comment

dasMulli Mar 20, 2017

Choose a reason for hiding this comment

rainersigwald Mar 20, 2017

Choose a reason for hiding this comment

rainersigwald Mar 20, 2017

Choose a reason for hiding this comment

dasMulli Mar 20, 2017

Choose a reason for hiding this comment

dasMulli commented Mar 20, 2017

nguerrera commented Mar 21, 2017

dasMulli commented Mar 21, 2017 • edited

dasMulli commented Apr 12, 2017

AArnott commented Apr 22, 2017

nguerrera commented May 4, 2017 • edited

nguerrera commented May 4, 2017

nguerrera commented May 4, 2017

davkean commented May 4, 2017

dasMulli commented May 4, 2017

AArnott commented May 8, 2017

dsplaisted left a comment

Choose a reason for hiding this comment

nguerrera commented May 19, 2017

dasMulli commented May 19, 2017

nguerrera commented May 19, 2017

nguerrera commented May 19, 2017

dasMulli commented May 19, 2017 • edited

enricosada commented May 25, 2017

nguerrera commented May 25, 2017

dasMulli commented Mar 18, 2017 •

edited

nguerrera Mar 20, 2017 •

edited

dasMulli commented Mar 21, 2017 •

edited

nguerrera commented May 4, 2017 •

edited

dasMulli commented May 19, 2017 •

edited