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.

Public/Src/Engine/Scheduler/Fingerprints/PipFingerprinter.cs

@@ -57,6 +57,7 @@ namespace BuildXL.Scheduler.Fingerprints
         private readonly ExpandedPathFileArtifactComparer m_expandedPathFileArtifactComparer;
         private readonly Comparer<FileArtifactWithAttributes> m_expandedPathFileArtifactWithAttributesComparer;
         private readonly Comparer<EnvironmentVariable> m_environmentVariableComparer;
+        private readonly PipFragmentRenderer m_pipFragmentRenderer;
 
         /// <summary>
         /// Directory comparer.
@@ -67,7 +68,7 @@ namespace BuildXL.Scheduler.Fingerprints
         /// The tokenizer used to handle path roots
         /// </summary>
         public readonly PathExpander PathExpander;
-        
+
         /// <summary>
         /// Gets or sets whether fingerprint text is returned when computing fingerprints.
         /// </summary>
@@ -111,6 +112,14 @@ namespace BuildXL.Scheduler.Fingerprints
             DirectoryComparer = Comparer<DirectoryArtifact>.Create((d1, d2) => m_pathTable.ExpandedPathComparer.Compare(d1.Path, d2.Path));
             m_environmentVariableComparer = Comparer<EnvironmentVariable>.Create((ev1, ev2) => { return ev1.Name.ToString(pathTable.StringTable).CompareTo(ev2.Name.ToString(pathTable.StringTable)); });
             m_expandedPathFileArtifactWithAttributesComparer = Comparer<FileArtifactWithAttributes>.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);
         }
 
         /// <summary>
@@ -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<FileArtifactWithAttributes, ReadOnlyArray<FileArtifactWithAttributes>>("Outputs", process.FileOutputs, (fp, f) => AddFileOutput(fp, f), m_expandedPathFileArtifactWithAttributesComparer);
             fingerprinter.AddOrderIndependentCollection<DirectoryArtifact, ReadOnlyArray<DirectoryArtifact>>("DirectoryOutputs", process.DirectoryOutputs, (h, p) => h.Add(p.Path), DirectoryComparer);
-                         
+
             fingerprinter.AddOrderIndependentCollection<AbsolutePath, ReadOnlyArray<AbsolutePath>>("UntrackedPaths", process.UntrackedPaths, (h, p) => h.Add(p), m_pathTable.ExpandedPathComparer);
             fingerprinter.AddOrderIndependentCollection<AbsolutePath, ReadOnlyArray<AbsolutePath>>("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));
         }
 
         /// <summary>

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
         /// </remarks>
-        TwoPhaseV2 = 65,
+        TwoPhaseV2 = 66,
     }
 }

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<FileArtifact>() { 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())

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
         /// <nodoc/>
         public static readonly Func<string, string> MaxMonikerRenderer = _ => s_longestIpcMoniker;
 
+        // Assumed max length of a rendered VSO hash.
+        private static readonly FileContentInfo s_longestContentHash = new FileContentInfo(ContentHashingUtilities.ZeroHash, FileContentInfo.LengthAndExistence.MaxSupportedLength);
+
         /// <summary>
         /// Gets the default invalid pip data
         /// </summary>
@@ -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