Improve incremental build in presence of globs #1328

AndyGerlicher · 2016-11-09T06:27:16Z

First iteration, looking for feedback. Filename, where I ended up putting it (Common targets vs Roslyn Core targets, etc.).

Fix issue with incremental build with globs.
Add a target that runs before CoreCompile that will record and store the state of the Compile ItemGroup. This file is added to the CustomAdditionalCompileInputs which is an input to CoreCompile. When this file changes, CoreCompile will run again. This fixes the issue of deleting a file or adding a file with an old timestamp to a glob (e.g. *.cs).
Add WriteOnlyWhenDifferent to WriteLinesToFile
Add option to not write the file when the file would not have changed (preserves the timestamp).

The original prototype wrote the file after compile and then subsequent runs could read that before compile and compare the file to the Compile ItemGroup. I went with this approach as it was far simpler and hopefully more elegant, but required a change to WriteLinesToFile to preserve the time stamp when the file doesn't change.

cdmihai · 2016-11-09T17:21:38Z

src/XMakeTasks/Microsoft.Common.CurrentVersion.targets

+    ============================================================
+    -->
+  <Target Name="_GenerateCompilerDependencyFile" BeforeTargets="CoreCompile">
+    <WriteLinesToFile Lines="@(Compile->'%(FullPath)')" File="$(IntermediateOutputPath)Compile.deps" Overwrite="True" WriteOnlyWhenDifferent="True" />


Since we're at this, in addition to Compile I wonder if it would be worth writing out all the properties / items that are not part of the CoreCompile Inputs but sent to CSC as args.

Definitely also references (via ReferencePath and DependsOnTargets="ResolveAssemblyReferences", probably?). Other properties and items should be caught by the Inputs="$(MSBuildAllProjects)" in Microsoft.CSharp.Core.targets. That would be more reliable with #1299 (as is, it might not catch some user-authored files) but is . . . ok now.

rainersigwald

I was preparing to be high and mighty about how this belongs in the core targets in the Roslyn repo, but this looks like a pretty good approach, and fairly general (it doesn't apply to C++ which uses a different item name, but should work for C# and VB both, with no changes on their end).

Would definitely like to run this by @jaredpar and/or @agocke though.

rainersigwald · 2016-11-09T18:52:39Z

src/XMakeTasks/UnitTests/ReadLinesFromFile_Tests.cs

+                    WriteOnlyWhenDifferent = true,
+                    Lines = new ITaskItem[] {new TaskItem("File contents1")}
+                };
+                Assert.True(a2.Execute());


I hate to say this but you need to sleep before running the second task. On HFS+ (and evidently some other *nix systems), timestamps have only a 1-second granularity, so this could rewrite the file but the checks below wouldn't detect it.

rainersigwald · 2016-11-09T20:31:26Z

src/XMakeTasks/Microsoft.Common.CurrentVersion.targets

+    ============================================================
+    -->
+  <Target Name="_GenerateCompilerDependencyFile" BeforeTargets="CoreCompile">
+    <WriteLinesToFile Lines="@(Compile->'%(FullPath)')" File="$(IntermediateOutputPath)Compile.deps" Overwrite="True" WriteOnlyWhenDifferent="True" />


Definitely also references (via ReferencePath and DependsOnTargets="ResolveAssemblyReferences", probably?). Other properties and items should be caught by the Inputs="$(MSBuildAllProjects)" in Microsoft.CSharp.Core.targets. That would be more reliable with #1299 (as is, it might not catch some user-authored files) but is . . . ok now.

rainersigwald · 2016-11-09T20:33:42Z

src/XMakeTasks/Microsoft.Common.CurrentVersion.targets

+    ============================================================
+    -->
+  <Target Name="_GenerateCompilerDependencyFile" BeforeTargets="CoreCompile">
+    <WriteLinesToFile Lines="@(Compile->'%(FullPath)')" File="$(IntermediateOutputPath)Compile.deps" Overwrite="True" WriteOnlyWhenDifferent="True" />


I don't like .deps, especially since there's already the .deps.json files for .NET Core. Maybe .CompilerInputs?

corecompile.cache? makes it clear what build entity it's for

agocke · 2016-11-09T21:14:59Z

Right now we don't have any logic around incremental build in the compiler and that feels appropriate.

Basically, the compiler itself just executes build commands from MSBuild -- so it feels like whether or not to issue the command is the responsibility of the MSBuild core tasks + targets, while figuring out how to execute the build is the responsibility of the Roslyn tasks + targets.

rainersigwald · 2016-11-09T21:26:14Z

@agocke Agreed, that totally meshes with the MSBuild model. But do you have any objection to this approach of writing references + source files to a file and using that as an additional input, so that we can catch the "compile *.cs" + "delete something.cs" + build (and actually re-invoke the compiler)?

agocke · 2016-11-09T22:11:36Z

@rainersigwald I think the general strategy is OK, but you'll need much more than just the references + source files to guarantee deterministic recompilation. @jaredpar Has the list of inputs for the compiler that need to be considered.

Regarding specifically adding files, have you considered looking at the last-modified timestamp of the directories in the glob? That may be simpler if you only care about adding/removing source files.

rainersigwald · 2016-11-09T23:18:46Z

Yeah, this is not a complete did-anything-change implementation, but it addresses a problem that will be more prevalent with the new dotnet SDK templates' use of wildcarding (because you can delete a file without changing the project file if it's not individually listed). The existing did-a-project-file-change check is the main "miscellaneous changes" catch.

jaredpar · 2016-11-10T16:19:28Z

ref/net46/Microsoft.Build.Tasks.Core/Microsoft.Build.Tasks.Core.cs

@@ -1177,6 +1177,7 @@ public partial class WriteLinesToFile : Microsoft.Build.Tasks.TaskExtension
        public Microsoft.Build.Framework.ITaskItem File { get { throw null; } set { } }
        public Microsoft.Build.Framework.ITaskItem[] Lines { get { throw null; } set { } }
        public bool Overwrite { get { throw null; } set { } }
+        public bool WriteOnlyWhenDifferent { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }


Why have the CompilerGeneratedAttribute here? The use doesn't seem to fit.

This is our ref assembly, auto-generated.

If it's auto-generated then why do only half have the attribute?

The property I added was an auto-property, and that crazy compiler we use added the CompilerGeneratedAttribute to the IL. The ref assembly code is just a dump of that, and the other properties all have backing fields so they don't have it.

jaredpar · 2016-11-10T16:19:51Z

src/XMakeTasks/FileIO/WriteLinesToFile.cs

@@ -23,6 +17,7 @@ public class WriteLinesToFile : TaskExtension
        private ITaskItem[] _lines = null;
        private bool _overwrite = false;
        private string _encoding = null;
+        private static Encoding s_defaultEncoding = new UTF8Encoding(false, true);


Recommend using named arguments here. On a casual read no idea what false and true mean here.

Recommend using `readonly here as well.

jaredpar · 2016-11-10T16:21:22Z

src/XMakeTasks/FileIO/WriteLinesToFile.cs

@@ -83,12 +85,12 @@ public override bool Execute()
                    }
                }

-                Encoding encode = null;
+                Encoding encoding = s_defaultEncoding;
                if (_encoding != null)


Making the s_defaultEncoding field with readonly allows you to avoid this logic.

This is if the user specified encoding (_encoding string set). I set it to the default so we could always safely use it rather than checking the parsed value for null because it failed to convert the string to an Encoding object.

Hmm ... then my follow up suggestion would be to rename _encoding or encoding. Makes it easy to mis-read this code 😄

jaredpar · 2016-11-10T16:22:03Z

src/XMakeTasks/FileIO/WriteLinesToFile.cs

                            {
-                                System.IO.File.WriteAllText(File.ItemSpec, buffer.ToString());
+                                var existingContents = System.IO.File.ReadAllText(File.ItemSpec);


What happens when ReadAllText throws? The use of File.Exists above doesn't give an guarantees this will succeed.

Added a low priority message for this case. No reason to fail the whole task if this happens.

jaredpar · 2016-11-10T16:22:45Z

src/XMakeTasks/FileIO/WriteLinesToFile.cs

+                            string contentsAsString = null;
+
+                            // When WriteOnlyWhenDifferent is set, read the file and if they're the same return.
+                            if (WriteOnlyWhenDifferent && System.IO.File.Exists(File.ItemSpec))


Recommend avoiding Exists here. Just try and read the file and deal with errors if they happen.

ReadAllText throws when the file doesn't exist. Since this is a pretty normal case (in the context I'm using it every clean build the file won't be there) I'd rather check first to avoid the allocation/read entirely.

jaredpar · 2016-11-10T16:23:36Z

src/XMakeTasks/FileIO/WriteLinesToFile.cs

+                                var existingContents = System.IO.File.ReadAllText(File.ItemSpec);
+                                if (existingContents.Length == buffer.Length)
+                                {
+                                    contentsAsString = buffer.ToString();


Isn't this going to be causing a lot more allocations to occur in MSBuild as a result? You're basically bringing all these files into memory as a contiguous string object.

This is true and it could be optimized. We ended up going with just the hash so the effect here is very minimal and not worth optimizing. I will file an issue to improve it.

cdmihai · 2016-11-14T17:24:24Z

src/XMakeTasks/UnitTests/Hash_Tests.cs

+            System.Diagnostics.Debugger.Break();
+
+            // Hash should be valid for empty item
+            var emptyItemHash = ExecuteHashTask(new ITaskItem[] {new TaskItem("")});


There could be another test for an empty array: ExecuteHashTask(new ITaskItem[0]);

AndyGerlicher · 2016-11-14T22:04:07Z

@dotnet-bot test Windows_NT Build for Desktop please

rainersigwald

Looks pretty good overall to me.

rainersigwald · 2016-11-14T22:06:37Z

src/XMakeTasks/FileIO/WriteLinesToFile.cs

+                                        contentsAsString = buffer.ToString();
+                                        if (existingContents.Equals(contentsAsString))
+                                        {
+                                            return true;


I think this is probably worth an info message.

rainersigwald · 2016-11-15T23:16:46Z

src/XMakeTasks/Hash.cs

+{
+    /// <summary>
+    /// Generates a hash of a given ItemGroup items. Metadata is not considered in the hash.
+    /// <remarks>Currently uses SHA1. Implementation subject to change between MSBuild versions.</remarks>


Maybe some sort of "Not intended as a cryptographic security measure" type caveat?

rainersigwald · 2016-11-15T23:17:08Z

src/XMakeTasks/Hash.cs

+        /// <summary>
+        /// Execute the task.
+        /// </summary>
+        /// <returns></returns>


nit: remove empty <returns>

rainersigwald · 2016-11-15T23:18:37Z

src/XMakeTasks/Hash.cs

+
+                using (var sha1 = SHA1.Create())
+                {
+                    var hash = sha1.ComputeHash(Encoding.UTF8.GetBytes(hashInput.ToString()));


I wish we didn't have to materialize the full string but I don't see a better way.

rainersigwald · 2016-11-15T23:20:02Z

src/XMakeTasks/Microsoft.Build.Tasks.csproj

@@ -400,6 +400,7 @@
    <Compile Include="FindAppConfigFile.cs">
      <ExcludeFromStyleCop>true</ExcludeFromStyleCop>
    </Compile>
+    <Compile Include="Hash.cs" />


big nit: alphabetize?

rainersigwald · 2016-11-15T23:20:52Z

src/XMakeTasks/Microsoft.Common.CurrentVersion.targets

@@ -3166,6 +3167,31 @@ Copyright (C) Microsoft Corporation. All rights reserved.

  <!--
    ============================================================
+                                        _GenerateCompilerDependencyCache


I'd name this GenerateCompileDependencyCache (no r), because it's around the Compile target. But I could go a different way.

rainersigwald · 2016-11-15T23:22:05Z

src/XMakeTasks/Microsoft.Common.CurrentVersion.targets

@@ -3010,6 +3010,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
      _GenerateCompileInputs;
      BeforeCompile;
      _TimeStampBeforeCompile;
+      _GenerateCompilerDependencyCache;


Why add this here instead of doing a BeforeTargets="CoreCompile"? Just for relative ordering?

rainersigwald · 2016-11-15T23:23:42Z

src/XMakeTasks/Resources/Strings.resx

@@ -2085,6 +2085,10 @@
    <value>MSB3491: Could not write lines to file "{0}". {1}</value>
    <comment>{StrBegin="MSB3491: "}</comment>
  </data>
+  <data name="WriteLinesToFile.ErrorReadingFile">
+    <value>MSB3492: Could not read existing file "{0}" to compare contents against. Will attempt to overwrite only.</value>


Could not read existing file "{0}" to determine whether its contents are up to date. Overwriting it. ?

Add option to not write the file when the file would not have changed (preserves the timestamp).

Add a task to perform a quick, in-memory, hash of a set of items..

Add a target that runs before CoreCompile that will record and store the state of the Compile ItemGroup. This file is added to the CustomAdditionalCompileInputs which is an input to CoreCompile. When this file changes, CoreCompile will run again. This fixes the issue of deleting a file or adding a file with an old timestamp to a glob (e.g. *.cs). Fixes dotnet#1327

dnfclas added the cla-already-signed label Nov 9, 2016

Sarabeth-Jaffe-Microsoft added the Needs Review label Nov 9, 2016

cdmihai reviewed Nov 9, 2016

View reviewed changes

rainersigwald reviewed Nov 9, 2016

View reviewed changes

jaredpar reviewed Nov 10, 2016

View reviewed changes

AndyGerlicher force-pushed the cscincrementalglob branch from add61d1 to c0d02df Compare November 11, 2016 21:57

cdmihai reviewed Nov 14, 2016

View reviewed changes

AndyGerlicher force-pushed the cscincrementalglob branch 4 times, most recently from c439ebe to fa39f89 Compare November 14, 2016 20:16

rainersigwald approved these changes Nov 15, 2016

View reviewed changes

AndyGerlicher added 3 commits November 16, 2016 09:58

Add WriteOnlyWhenDifferent to WriteLinesToFile

56348d5

Add option to not write the file when the file would not have changed (preserves the timestamp).

Add built-in Hash task

443dd47

Add a task to perform a quick, in-memory, hash of a set of items..

AndyGerlicher force-pushed the cscincrementalglob branch from fa39f89 to e56d8fb Compare November 16, 2016 17:58

AndyGerlicher merged commit 74f1a45 into dotnet:xplat Nov 16, 2016

Sarabeth-Jaffe-Microsoft removed the Needs Review label Nov 16, 2016

AndyGerlicher deleted the cscincrementalglob branch November 16, 2016 18:25

rainersigwald mentioned this pull request Mar 9, 2017

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

Closed

dasMulli mentioned this pull request Mar 18, 2017

Incremental build improvements for generated AssemblyInfo.cs dotnet/sdk#1007

Closed

rainersigwald mentioned this pull request Jun 1, 2017

Adding existing cs files or empty code files doesn't trigger FastUpdate dotnet/project-system#2347

Closed

rainersigwald mentioned this pull request Aug 24, 2017

Incremental builds: Respect inputs closure changes as well as timestamps #2470

Open

rainersigwald mentioned this pull request Dec 9, 2019

Ensure we update the response files when the targets are run. microsoft/cppwinrt#445

Merged

rainersigwald mentioned this pull request Nov 17, 2021

Consider replacing CoreCompileCache with Roslyn DeterministicKey #7043

Open

Improve incremental build in presence of globs #1328

Improve incremental build in presence of globs #1328

Conversation

AndyGerlicher commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Nov 9, 2016 • edited

Choose a reason for hiding this comment

agocke commented Nov 9, 2016 • edited

rainersigwald commented Nov 9, 2016

agocke commented Nov 9, 2016

rainersigwald commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyGerlicher commented Nov 14, 2016

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Nov 9, 2016 •

edited

agocke commented Nov 9, 2016 •

edited