From f3f6221f85ef761209d8b11d1dbba02e86dcf5ba Mon Sep 17 00:00:00 2001 From: olkononenko <48926876+olkononenko@users.noreply.github.com> Date: Mon, 21 Oct 2019 17:52:15 -0700 Subject: [PATCH] Fix the rendering of pip arguments for cache purposes (#1077) VSO hashes on the command line are incorrectly rendered as sequence of zeros. This PR fixes this. --- .../Fingerprints/PipFingerprinter.cs | 23 +++++-- .../Fingerprints/PipFingerprintingVersion.cs | 3 +- .../Scheduler/PipFingerprinterTests.cs | 68 +++++++++++++++++++ Public/Src/Pips/Dll/Operations/PipData.cs | 12 +++- 4 files changed, 96 insertions(+), 10 deletions(-) diff --git a/Public/Src/Engine/Scheduler/Fingerprints/PipFingerprinter.cs b/Public/Src/Engine/Scheduler/Fingerprints/PipFingerprinter.cs index 60b056822..506d8c373 100644 --- a/Public/Src/Engine/Scheduler/Fingerprints/PipFingerprinter.cs +++ b/Public/Src/Engine/Scheduler/Fingerprints/PipFingerprinter.cs @@ -57,6 +57,7 @@ namespace BuildXL.Scheduler.Fingerprints private readonly ExpandedPathFileArtifactComparer m_expandedPathFileArtifactComparer; private readonly Comparer m_expandedPathFileArtifactWithAttributesComparer; private readonly Comparer m_environmentVariableComparer; + private readonly PipFragmentRenderer m_pipFragmentRenderer; /// /// Directory comparer. @@ -67,7 +68,7 @@ namespace BuildXL.Scheduler.Fingerprints /// The tokenizer used to handle path roots /// public readonly PathExpander PathExpander; - + /// /// Gets or sets whether fingerprint text is returned when computing fingerprints. /// @@ -111,6 +112,14 @@ namespace BuildXL.Scheduler.Fingerprints DirectoryComparer = Comparer.Create((d1, d2) => m_pathTable.ExpandedPathComparer.Compare(d1.Path, d2.Path)); m_environmentVariableComparer = Comparer.Create((ev1, ev2) => { return ev1.Name.ToString(pathTable.StringTable).CompareTo(ev2.Name.ToString(pathTable.StringTable)); }); m_expandedPathFileArtifactWithAttributesComparer = Comparer.Create((f1, f2) => m_pathTable.ExpandedPathComparer.Compare(f1.Path, f2.Path)); + m_pipFragmentRenderer = new PipFragmentRenderer( + pathExpander: path => PathExpander.ExpandPath(pathTable, path).ToUpperInvariant(), + pathTable.StringTable, + // Do not resolve monikers because their values will be different every build. + monikerRenderer: m => m, + // Use the hash lookup delegate that was passed as an argument. + // PipFragmentRenderer can accept a null value here, and it has special logic for such cases. + m_contentHashLookup); } /// @@ -141,8 +150,8 @@ namespace BuildXL.Scheduler.Fingerprints // Bug #681083 include somehow information about process.ShutdownProcessPipId and process.ServicePipDependencies // but make sure it doesn't depend on PipIds (because they are not stable between builds) - fingerprintInputText = FingerprintTextEnabled - ? (m_extraFingerprintSalts.CalculatedSaltsFingerprintText + Environment.NewLine + hashingHelper.FingerprintInputText) + fingerprintInputText = FingerprintTextEnabled + ? (m_extraFingerprintSalts.CalculatedSaltsFingerprintText + Environment.NewLine + hashingHelper.FingerprintInputText) : string.Empty; return new ContentFingerprint(hashingHelper.GenerateHash()); @@ -269,7 +278,7 @@ namespace BuildXL.Scheduler.Fingerprints fingerprinter.AddOrderIndependentCollection>("Outputs", process.FileOutputs, (fp, f) => AddFileOutput(fp, f), m_expandedPathFileArtifactWithAttributesComparer); fingerprinter.AddOrderIndependentCollection>("DirectoryOutputs", process.DirectoryOutputs, (h, p) => h.Add(p.Path), DirectoryComparer); - + fingerprinter.AddOrderIndependentCollection>("UntrackedPaths", process.UntrackedPaths, (h, p) => h.Add(p), m_pathTable.ExpandedPathComparer); fingerprinter.AddOrderIndependentCollection>("UntrackedScopes", process.UntrackedScopes, (h, p) => h.Add(p), m_pathTable.ExpandedPathComparer); @@ -293,9 +302,9 @@ namespace BuildXL.Scheduler.Fingerprints { fingerprinter.Add("RequiresAdmin", 1); } - + fingerprinter.Add("NeedsToRunInContainer", process.NeedsToRunInContainer ? 1 : 0); - fingerprinter.Add("ContainerIsolationLevel", (byte) process.ContainerIsolationLevel); + fingerprinter.Add("ContainerIsolationLevel", (byte)process.ContainerIsolationLevel); AddPipData(fingerprinter, "Arguments", process.Arguments); if (process.ResponseFileData.IsValid) @@ -354,7 +363,7 @@ namespace BuildXL.Scheduler.Fingerprints Contract.Requires(name != null); Contract.Requires(fingerprinter != null); - fingerprinter.Add(name, data.ToString(path => PathExpander.ExpandPath(m_pathTable, path).ToUpperInvariant(), m_pathTable.StringTable)); + fingerprinter.Add(name, data.ToString(m_pipFragmentRenderer)); } /// diff --git a/Public/Src/Engine/Scheduler/Fingerprints/PipFingerprintingVersion.cs b/Public/Src/Engine/Scheduler/Fingerprints/PipFingerprintingVersion.cs index a4502162e..168b4c9a3 100644 --- a/Public/Src/Engine/Scheduler/Fingerprints/PipFingerprintingVersion.cs +++ b/Public/Src/Engine/Scheduler/Fingerprints/PipFingerprintingVersion.cs @@ -37,7 +37,8 @@ namespace BuildXL.Scheduler.Fingerprints /// 62: FileContentInfo - change how length/existence is stored. /// 63: IncrementalTool - change reparsepoint probes and enumeration probes to read. /// 65: 64 is already used since 20190903; change in UnsafeOption serialization (partial preserve outputs) + /// 66: Changed rendering of VSO hashes /// - TwoPhaseV2 = 65, + TwoPhaseV2 = 66, } } \ No newline at end of file diff --git a/Public/Src/Engine/UnitTests/Scheduler/PipFingerprinterTests.cs b/Public/Src/Engine/UnitTests/Scheduler/PipFingerprinterTests.cs index 60aa69ee6..07af37329 100644 --- a/Public/Src/Engine/UnitTests/Scheduler/PipFingerprinterTests.cs +++ b/Public/Src/Engine/UnitTests/Scheduler/PipFingerprinterTests.cs @@ -621,6 +621,74 @@ namespace Test.BuildXL.Scheduler XAssert.IsTrue(ExtraFingerprintSalts.ArePipWarningsPromotedToErrors(config)); } + [Fact] + public void TestVsoHashesInArgumentsRenderedProperly() + { + var executablePath = X("/x/pkgs/tool.exe"); + var pathTable = m_context.PathTable; + var executable = FileArtifact.CreateSourceFile(AbsolutePath.Create(pathTable, executablePath)); + + var dependencies = new HashSet() { executable }; + var pb = GetDefaultProcessBuilder(pathTable, executable) + .WithDependencies(dependencies); + + var inputFilePath = X("/x/pkgs/input.txt"); + var inputFile = FileArtifact.CreateSourceFile(AbsolutePath.Create(pathTable, inputFilePath)); + + PipDataBuilder b = new PipDataBuilder(pathTable.StringTable); + b.AddVsoHash(inputFile); + + pb.WithArguments(b.ToPipData(string.Empty, PipDataFragmentEscaping.NoEscaping)); + var process = pb.Build(); + + MountPathExpander expander = new MountPathExpander(pathTable); + + var inputFileHash = FileContentInfo.CreateWithUnknownLength(ContentHashingUtilities.CreateSpecialValue(1)); + var anotherFileHash = FileContentInfo.CreateWithUnknownLength(ContentHashingUtilities.CreateSpecialValue(3)); + + PipFragmentRenderer.ContentHashLookup hashLookup = file => + { + if (file == inputFile) + { + return inputFileHash; + } + else + { + return anotherFileHash; + } + }; + + var fingerprinter1 = new PipContentFingerprinter( + m_context.PathTable, + hashLookup, + ExtraFingerprintSalts.Default(), + pathExpander: expander) + { + FingerprintTextEnabled = true + }; + + var contentFingerprint1 = fingerprinter1.ComputeWeakFingerprint(process, out string fingerprintText1); + fingerprintText1 = fingerprintText1.ToUpperInvariant(); + + // 'change' the input file => the rendering of an argument should result in a different value => should compute a different fingerprint + inputFileHash = FileContentInfo.CreateWithUnknownLength(ContentHashingUtilities.CreateSpecialValue(2)); + + var fingerprinter2 = new PipContentFingerprinter( + m_context.PathTable, + hashLookup, + ExtraFingerprintSalts.Default(), + pathExpander: expander) + { + FingerprintTextEnabled = true + }; + + var contentFingerprint2 = fingerprinter2.ComputeWeakFingerprint(process, out string fingerprintText2); + fingerprintText2 = fingerprintText2.ToUpperInvariant(); + + XAssert.AreNotEqual(fingerprintText1, fingerprintText2, $"fp1: '{fingerprintText1}'; fp2: '{fingerprintText2}'"); + XAssert.AreNotEqual(contentFingerprint1, contentFingerprint2); + } + private ProcessBuilder GetDefaultProcessBuilder(PathTable pathTable, FileArtifact executable) { return (new ProcessBuilder()) diff --git a/Public/Src/Pips/Dll/Operations/PipData.cs b/Public/Src/Pips/Dll/Operations/PipData.cs index 2953a93f9..f9d3e23aa 100644 --- a/Public/Src/Pips/Dll/Operations/PipData.cs +++ b/Public/Src/Pips/Dll/Operations/PipData.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Diagnostics.ContractsLight; using System.Text; +using BuildXL.Storage; using BuildXL.Utilities; namespace BuildXL.Pips.Operations @@ -38,6 +39,9 @@ namespace BuildXL.Pips.Operations /// public static readonly Func MaxMonikerRenderer = _ => s_longestIpcMoniker; + // Assumed max length of a rendered VSO hash. + private static readonly FileContentInfo s_longestContentHash = new FileContentInfo(ContentHashingUtilities.ZeroHash, FileContentInfo.LengthAndExistence.MaxSupportedLength); + /// /// Gets the default invalid pip data /// @@ -224,12 +228,16 @@ namespace BuildXL.Pips.Operations { Contract.Requires(stringTable != null); - var renderer = new PipFragmentRenderer(s_maxPathExpander, stringTable, MaxMonikerRenderer); + var renderer = new PipFragmentRenderer( + s_maxPathExpander, + stringTable, + MaxMonikerRenderer, + hashLookup: _ => s_longestContentHash); int computedLength = GetMaxLength(this, renderer); #if DEBUG // Expensive check on in DEBUG builds: costs 6% percent of entire runtime - string actualValue = ToString(s_maxPathExpander, stringTable, MaxMonikerRenderer); + string actualValue = ToString(renderer); Contract.Assert(computedLength == actualValue.Length); #endif