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

Improve incremental build in presence of globs #1328

Merged
merged 3 commits into from Nov 16, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions build/NuGetPackages/Microsoft.Build.Tasks.Core.nuspec
Expand Up @@ -47,6 +47,8 @@
<dependency id="System.Runtime.InteropServices.RuntimeInformation" version="4.0.0" />
<dependency id="System.Runtime.Serialization.Primitives" version="4.1.1" />
<dependency id="System.Runtime.Serialization.Xml" version="4.1.1" />
<dependency id="System.Security.Cryptography.Algorithms" version="4.2.0" />
<dependency id="System.Security.Cryptography.Primitives" version="4.0.0" />
<dependency id="System.Security.Cryptography.X509Certificates" version="4.1.0" />
<dependency id="System.Text.Encoding" version="4.0.11" />
<dependency id="System.Text.Encoding.Extensions" version="4.0.11" />
Expand Down
10 changes: 10 additions & 0 deletions ref/net46/Microsoft.Build.Tasks.Core/Microsoft.Build.Tasks.Core.cs
Expand Up @@ -655,6 +655,15 @@ public partial class GetSDKReferenceFiles : Microsoft.Build.Tasks.TaskExtension
public string TargetSDKVersion { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public override bool Execute() { throw null; }
}
public partial class Hash : Microsoft.Build.Tasks.TaskExtension
{
public Hash() { }
[Microsoft.Build.Framework.OutputAttribute]
public string HashResult { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
[Microsoft.Build.Framework.RequiredAttribute]
public Microsoft.Build.Framework.ITaskItem[] ItemsToHash { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public override bool Execute() { throw null; }
}
[System.Runtime.InteropServices.GuidAttribute("00020401-0000-0000-C000-000000000046")]
[System.Runtime.InteropServices.InterfaceTypeAttribute((System.Runtime.InteropServices.ComInterfaceType)(1))]
public partial interface IFixedTypeInfo
Expand Down Expand Up @@ -1177,6 +1186,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 { } }
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is our ref assembly, auto-generated.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

public override bool Execute() { throw null; }
}
public partial class XamlTaskFactory : Microsoft.Build.Framework.ITaskFactory
Expand Down
Expand Up @@ -370,6 +370,15 @@ public partial class GetReferenceAssemblyPaths : Microsoft.Build.Tasks.TaskExten
public string TargetFrameworkMonikerDisplayName { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public override bool Execute() { throw null; }
}
public partial class Hash : Microsoft.Build.Tasks.TaskExtension
{
public Hash() { }
[Microsoft.Build.Framework.OutputAttribute]
public string HashResult { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
[Microsoft.Build.Framework.RequiredAttribute]
public Microsoft.Build.Framework.ITaskItem[] ItemsToHash { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public override bool Execute() { throw null; }
}
public partial class MakeDir : Microsoft.Build.Tasks.TaskExtension
{
public MakeDir() { }
Expand Down Expand Up @@ -625,6 +634,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 { } }
public override bool Execute() { throw null; }
}
}
Expand Down
62 changes: 39 additions & 23 deletions src/XMakeTasks/FileIO/WriteLinesToFile.cs
Expand Up @@ -4,12 +4,7 @@
using System;
using System.IO;
using System.Text;
using System.Diagnostics;
using System.Collections;
using System.Globalization;

using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;
using Microsoft.Build.Shared;

namespace Microsoft.Build.Tasks
Expand All @@ -24,6 +19,9 @@ public class WriteLinesToFile : TaskExtension
private bool _overwrite = false;
private string _encoding = null;

// Default encoding taken from System.IO.WriteAllText()
private static readonly Encoding s_defaultEncoding = new UTF8Encoding(false, true);

/// <summary>
/// File to write lines to.
/// </summary>
Expand Down Expand Up @@ -61,6 +59,13 @@ public string Encoding
set { _encoding = value; }
}

/// <summary>
/// If true, the target file specified, if it exists, will be read first to compare against
/// what the task would have written. If identical, the file is not written to disk and the
/// timestamp will be preserved.
/// </summary>
public bool WriteOnlyWhenDifferent { get; set; }


/// <summary>
/// Execute the task.
Expand All @@ -83,12 +88,12 @@ public override bool Execute()
}
}

Encoding encode = null;
Encoding encoding = s_defaultEncoding;
if (_encoding != null)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

{
try
{
encode = System.Text.Encoding.GetEncoding(_encoding);
encoding = System.Text.Encoding.GetEncoding(_encoding);
}
catch (ArgumentException)
{
Expand All @@ -97,7 +102,6 @@ public override bool Execute()
}
}


try
{
if (Overwrite)
Expand All @@ -110,29 +114,41 @@ public override bool Execute()
}
else
{
// Passing a null encoding, or Encoding.Default, to WriteAllText or AppendAllText
// is not the same as calling the overload that does not take encoding!
// Encoding.Default is based on the current codepage, the overload without encoding is UTF8-without-BOM.
if (encode == null)
string contentsAsString = null;

try
{
// When WriteOnlyWhenDifferent is set, read the file and if they're the same return.
if (WriteOnlyWhenDifferent && FileUtilities.FileExistsNoThrow(File.ItemSpec))
{
var existingContents = System.IO.File.ReadAllText(File.ItemSpec);
if (existingContents.Length == buffer.Length)
{
contentsAsString = buffer.ToString();
if (existingContents.Equals(contentsAsString))
{
Log.LogMessageFromResources(MessageImportance.Low, "WriteLinesToFile.SkippingUnchangedFile", File.ItemSpec);
return true;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably worth an info message.

}
}
}
}
catch (IOException)
{
System.IO.File.WriteAllText(File.ItemSpec, buffer.ToString());
Log.LogMessageFromResources(MessageImportance.Low, "WriteLinesToFile.ErrorReadingFile", File.ItemSpec);
}
else

if (contentsAsString == null)
{
System.IO.File.WriteAllText(File.ItemSpec, buffer.ToString(), encode);
contentsAsString = buffer.ToString();
}

System.IO.File.WriteAllText(File.ItemSpec, contentsAsString, encoding);
}
}
else
{
if (encode == null)
{
System.IO.File.AppendAllText(File.ItemSpec, buffer.ToString());
}
else
{
System.IO.File.AppendAllText(File.ItemSpec, buffer.ToString(), encode);
}
System.IO.File.AppendAllText(File.ItemSpec, buffer.ToString(), encoding);
}
}
catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e))
Expand Down
62 changes: 62 additions & 0 deletions src/XMakeTasks/Hash.cs
@@ -0,0 +1,62 @@
using System.Security.Cryptography;
using System.Text;
using Microsoft.Build.Framework;

namespace Microsoft.Build.Tasks
{
/// <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. Not
/// intended as a cryptographic security measure, only uniqueness between build executions.
/// </remarks>
/// </summary>
public class Hash : TaskExtension
{
private const string ItemSeparatorCharacter = "\u2028";

/// <summary>
/// Items from which to generate a hash.
/// </summary>
[Required]
public ITaskItem[] ItemsToHash { get; set; }

/// <summary>
/// Hash of the ItemsToHash ItemSpec.
/// </summary>
[Output]
public string HashResult { get; set; }

/// <summary>
/// Execute the task.
/// </summary>
public override bool Execute()
{
if (ItemsToHash != null && ItemsToHash.Length > 0)
{
StringBuilder hashInput = new StringBuilder();

foreach (var item in ItemsToHash)
{
hashInput.Append(item.ItemSpec);
hashInput.Append(ItemSeparatorCharacter);
}

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

Choose a reason for hiding this comment

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

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

var hashResult = new StringBuilder(hash.Length*2);

foreach (byte b in hash)
{
hashResult.Append(b.ToString("x2"));
}

HashResult = hashResult.ToString();
}
}

return true;
}
}
}
1 change: 1 addition & 0 deletions src/XMakeTasks/Microsoft.Build.Tasks.csproj
Expand Up @@ -404,6 +404,7 @@
<ExcludeFromStyleCop>true</ExcludeFromStyleCop>
</Compile>
<Compile Include="GetReferenceAssemblyPaths.cs" />
<Compile Include="Hash.cs" />
<Compile Include="InstalledSDKResolver.cs" />
<Compile Include="ErrorFromResources.cs" />
<Compile Include="ExtractedClassName.cs">
Expand Down
26 changes: 26 additions & 0 deletions src/XMakeTasks/Microsoft.Common.CurrentVersion.targets
Expand Up @@ -3010,6 +3010,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
_GenerateCompileInputs;
BeforeCompile;
_TimeStampBeforeCompile;
_GenerateCompileDependencyCache;
CoreCompile;
_TimeStampAfterCompile;
AfterCompile;
Expand Down Expand Up @@ -3164,6 +3165,31 @@ Copyright (C) Microsoft Corporation. All rights reserved.

</Target>

<!--
============================================================
_GenerateCompileDependencyCache

Generate a file used to track compiler dependencies between incremental build
executions. This handles cases where items are added or removed from a glob (e.g.
<Compile Include="**\*.cs" />) and can't otherwise be detected with timestamp
comparisons. The file contains a hash of compiler inputs that are known to
contribute to incremental build inconsistencies.
============================================================
-->
<Target Name="_GenerateCompileDependencyCache" DependsOnTargets="ResolveAssemblyReferences">
<ItemGroup>
<CustomAdditionalCompileInputs Include="$(IntermediateOutputPath)CoreCompileInputs.cache" />
<CoreCompileCache Include="@(Compile->'%(FullPath)')" />
<CoreCompileCache Include="@(ReferencePath->'%(FullPath)')" />
</ItemGroup>

<Hash ItemsToHash="@(CoreCompileCache)">
<Output TaskParameter="HashResult" PropertyName="CoreCompileDependencyHash" />
</Hash>

<WriteLinesToFile Lines="$(CoreCompileDependencyHash)" File="$(IntermediateOutputPath)CoreCompileInputs.cache" Overwrite="True" WriteOnlyWhenDifferent="True" />
</Target>

<!--
============================================================
_TimeStampAfterCompile
Expand Down
1 change: 1 addition & 0 deletions src/XMakeTasks/Microsoft.Common.tasks
Expand Up @@ -125,6 +125,7 @@
<UsingTask TaskName="Microsoft.Build.Tasks.GetFrameworkPath" AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition="'$(MSBuildAssemblyVersion)' != ''" />
<UsingTask TaskName="Microsoft.Build.Tasks.GetFrameworkSdkPath" AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition="'$(MSBuildAssemblyVersion)' != ''" />
<UsingTask TaskName="Microsoft.Build.Tasks.GetReferenceAssemblyPaths" AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition="'$(MSBuildAssemblyVersion)' != ''" />
<UsingTask TaskName="Microsoft.Build.Tasks.Hash" AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition="'$(MSBuildAssemblyVersion)' != ''" />
<UsingTask TaskName="Microsoft.Build.Tasks.LC" AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition="'$(MSBuildAssemblyVersion)' != ''" />
<UsingTask TaskName="Microsoft.Build.Tasks.MakeDir" AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition="'$(MSBuildAssemblyVersion)' != ''" />
<UsingTask TaskName="Microsoft.Build.Tasks.Message" AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition="'$(MSBuildAssemblyVersion)' != ''" />
Expand Down
8 changes: 8 additions & 0 deletions src/XMakeTasks/Resources/Strings.resx
Expand Up @@ -2085,6 +2085,14 @@
<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 determine whether its contents are up to date. Overwriting it.</value>
<comment>{StrBegin="MSB3491: "}</comment>
</data>
<data name="WriteLinesToFile.SkippingUnchangedFile">
<value>MSB3493: </value>
<comment>{StrBegin="Skipping write to file "{0}" because content would not change."}</comment>
</data>
<!--
The GetReferenceAssemblyPaths message bucket is: MSB3642 - MSB3646.
If this bucket overflows, pls. contact 'vsppbdev'.
Expand Down
59 changes: 59 additions & 0 deletions src/XMakeTasks/UnitTests/Hash_Tests.cs
@@ -0,0 +1,59 @@
using Microsoft.Build.Framework;
using Microsoft.Build.UnitTests;
using Microsoft.Build.Utilities;
using Xunit;

namespace Microsoft.Build.Tasks.UnitTests
{
public class Hash_Tests
{
[Fact]
public void HashTaskTest()
{
// This hash was pre-computed. If the implementation changes it may need to be adjusted.
var expectedHash = "5593e2db83ac26117cd95ed8917f09b02a02e2a0";

var actualHash = ExecuteHashTask(new ITaskItem[]
{
new TaskItem("Item1"), new TaskItem("Item2"), new TaskItem("Item3")
});
Assert.Equal(expectedHash, actualHash);

// Try again to ensure the same hash
var actualHash2 = ExecuteHashTask(new ITaskItem[]
{
new TaskItem("Item1"), new TaskItem("Item2"), new TaskItem("Item3")
});
Assert.Equal(expectedHash, actualHash2);
}

[Fact]
public void HashTaskEmptyInputTest()
{
// Hash should be valid for empty item
var emptyItemHash = ExecuteHashTask(new ITaskItem[] {new TaskItem("")});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Assert.False(string.IsNullOrWhiteSpace(emptyItemHash));
Assert.NotEmpty(emptyItemHash);

// Hash should be null for null ItemsToHash or array of length 0
var nullItemsHash = ExecuteHashTask(null);
Assert.Null(nullItemsHash);

var zeroLengthItemsHash = ExecuteHashTask(new ITaskItem[0]);
Assert.Null(zeroLengthItemsHash);
}

private string ExecuteHashTask(ITaskItem[] items)
{
var hashTask = new Hash
{
BuildEngine = new MockEngine(),
ItemsToHash = items
};

Assert.True(hashTask.Execute());

return hashTask.HashResult;
}
}
}
Expand Up @@ -62,6 +62,7 @@
<Compile Include="FindInList_Tests.cs" />
<Compile Include="FindUnderPath_Tests.cs" />
<Compile Include="GenerateBindingRedirects_Tests.cs" />
<Compile Include="Hash_Tests.cs" />
<Compile Include="GenerateResource_Tests.cs" />
<Compile Include="GetReferencePaths_Tests.cs" />
<Compile Include="DirectoryBuildProjectImportTestBase.cs" />
Expand Down