-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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> | ||
|
@@ -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. | ||
|
@@ -83,12 +88,12 @@ public override bool Execute() | |
} | ||
} | ||
|
||
Encoding encode = null; | ||
Encoding encoding = s_defaultEncoding; | ||
if (_encoding != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is if the user specified encoding ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm ... then my follow up suggestion would be to rename |
||
{ | ||
try | ||
{ | ||
encode = System.Text.Encoding.GetEncoding(_encoding); | ||
encoding = System.Text.Encoding.GetEncoding(_encoding); | ||
} | ||
catch (ArgumentException) | ||
{ | ||
|
@@ -97,7 +102,6 @@ public override bool Execute() | |
} | ||
} | ||
|
||
|
||
try | ||
{ | ||
if (Overwrite) | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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("")}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.