Dump the fingerprint if the targets at least match for read scenarios (#124)

Author: dfederm

README.md

@@ -72,7 +72,7 @@ These settings are common across all plugins, although different implementations
 | `$(MSBuildCacheAllowProcessCloseAfterProjectFinishProcessPatterns)` | `Glob[]` | `\**\mspdbsrv.exe` | Processes to allow to exit after the project which launched it completes, ie detached processes. |
 | `$(MSBuildCacheGlobalPropertiesToIgnore)` | `string[]` | `CurrentSolutionConfigurationContents; ShouldUnsetParentConfigurationAndPlatform; BuildingInsideVisualStudio; BuildingSolutionFile; SolutionDir; SolutionExt; SolutionFileName; SolutionName; SolutionPath; _MSDeployUserAgent`, as well as all proeprties related to plugin settings | The list of global properties to exclude from consideration by the cache |
 | `$(MSBuildCacheGetResultsForUnqueriedDependencies)` | `bool` | false | Whether to try and query the cache for dependencies if they have not previously been requested. This option can help in cases where the build isn't done in graph order, or if some projects are skipped. |
-| `$(MSBuildCacheTargetsToIgnore)` | `string[]` | `GetTargetFrameworks;GetNativeManifest;GetCopyToOutputDirectoryItems;GetTargetFrameworksWithPlatformForSingleTargetFramework` | The list of targets to ignore when determining if a build request matches a cache entry. This is intended for "information gathering" targets which do not have side-effect. eg. a build with `/t:Build` and `/t:Build;GetTargetFrameworks` should be considered to have equivalent results. Note: This only works "one-way" in that the build request is allowed to have missing targets, while the cache entry is not. This is to avoid a situation where a build request recieves a cache hit with missing target results, where a cache hit with extra target results is acceptable. |
+| `$(MSBuildCacheTargetsToIgnore)` | `string[]` | `GetTargetFrameworks;GetNativeManifest;GetCopyToOutputDirectoryItems;GetTargetFrameworksWithPlatformForSingleTargetFramework` | The list of targets to ignore when determining if a build request matches a cache entry. This is intended for "information gathering" targets which do not have side-effect. eg. a build with `/t:Build` and `/t:Build;GetTargetFrameworks` should be considered to have equivalent results. Note: This only works "one-way" in that the build request is allowed to have missing targets, while the cache entry is not. This is to avoid a situation where a build request receives a cache hit with missing target results, where a cache hit with extra target results is acceptable. |
 | `$(MSBuildCacheSkipUnchangedOutputFiles)` | `bool` | false | Whether to avoid writing output files on cache hit if the file is unchanged, which can improve performance for incremental builds. A file is considered unchanged if it exists, the previously placed file and file to be placed have the same hash, and the the previously placed file and current file on disk have the same timestamp and file size. |
 | `$(MSBuildCacheTouchOutputFiles)` | `bool` | false | Whether to update the last write time for output files on cache hit. All files for a given cache entry will have the same timestamp. Note that outputs which skip materialization via `MSBuildCacheSkipUnchangedOutputFiles` are still touched. |
 | `$(MSBuildCacheIgnoreDotNetSdkPatchVersion)` | `bool` | false | Whether to ignore the patch version when doing cache lookups. This trades off some correctness for the sake of getting cache hits when the SDK version isn't exactly the same. The default behavior is to consider the exact SDK version, eg. "8.0.404". With this setting set to true, it will instead use something like "8.0.4XX". Note that the major version, minor version, and feature bands are still considered. |

src/Common/MSBuildCachePluginBase.cs

@@ -393,7 +393,7 @@ private async Task<CacheResult> GetCacheResultInnerAsync(BuildRequestData buildR
             return CacheResult.IndicateNonCacheHit(CacheResultType.CacheNotApplicable);
         }
 
-        if (!DoTargetsMatch(nodeContext.TargetNames, buildRequest.TargetNames))
+        if (!DoTargetsMatchForRead(nodeContext.TargetNames, buildRequest.TargetNames))
         {
             logger.LogMessage($"`TargetNames` does not match for {nodeContext.Id}. `{string.Join(";", nodeContext.TargetNames)}` vs `{string.Join(";", buildRequest.TargetNames)}`.");
             return CacheResult.IndicateNonCacheHit(CacheResultType.CacheNotApplicable);
@@ -497,7 +497,12 @@ private void HandleFileAccessInner(FileAccessContext fileAccessContext, FileAcce
         _hasHadFileAccessReport = true;
 
         NodeContext? nodeContext = GetNodeContext(fileAccessContext);
-        if (nodeContext == null)
+        if (nodeContext is null)
+        {
+            return;
+        }
+
+        if (!DoTargetsMatchForWrite(nodeContext.TargetNames, fileAccessContext.Targets))
         {
             return;
         }
@@ -516,7 +521,12 @@ public override void HandleProcess(FileAccessContext fileAccessContext, ProcessD
     private void HandleProcessInner(FileAccessContext fileAccessContext, ProcessData processData)
     {
         NodeContext? nodeContext = GetNodeContext(fileAccessContext);
-        if (nodeContext == null)
+        if (nodeContext is null)
+        {
+            return;
+        }
+
+        if (!DoTargetsMatchForWrite(nodeContext.TargetNames, fileAccessContext.Targets))
         {
             return;
         }
@@ -539,11 +549,46 @@ private async Task HandleProjectFinishedInnerAsync(FileAccessContext fileAccessC
         }
 
         NodeContext? nodeContext = GetNodeContext(fileAccessContext);
-        if (nodeContext == null)
+        if (nodeContext is null)
         {
             return;
         }
 
+        // If the target do not match for write, the result is not sufficient to cache as it's missing targets.
+        if (!DoTargetsMatchForWrite(nodeContext.TargetNames, fileAccessContext.Targets))
+        {
+            // Remove the initial targets, if any, from the reported targets to guess at the originally requested targets.
+            IReadOnlyList<string> requestedTargets;
+            if (nodeContext.ProjectInstance.InitialTargets.Count > 0)
+            {
+                List<string> requestedTargetsList = new(fileAccessContext.Targets.Count - nodeContext.ProjectInstance.InitialTargets.Count);
+                foreach (string reportedTarget in fileAccessContext.Targets)
+                {
+                    if (!nodeContext.ProjectInstance.InitialTargets.Contains(reportedTarget, StringComparer.OrdinalIgnoreCase))
+                    {
+                        requestedTargetsList.Add(reportedTarget);
+                    }
+                }
+
+                requestedTargets = requestedTargetsList;
+            }
+            else
+            {
+                requestedTargets = fileAccessContext.Targets;
+            }
+
+            // If the targets match for read, it is very unlikely that we will later get a result that does match for write as the
+            // targets ignored for reads should be information-gathering only. To help debug cache misses dump the fingerprint.
+            if (DoTargetsMatchForRead(nodeContext.TargetNames, requestedTargets))
+            {
+                // Because we do not have the file accesses, we cannot know the PathSet.
+                // This means that any cache miss analysis will need to take this into account.
+                await DumpFingerprintLogAsync(logger, nodeContext, null);
+            }
+
+            return;
+        }
+
         nodeContext.SetEndTime();
 
         // In niche cases, eg traversal projects, the build may be successful despite a dependency failing. Ignore these cases since we can't properly fingerprint failed dependencies.
@@ -556,18 +601,14 @@ private async Task HandleProjectFinishedInnerAsync(FileAccessContext fileAccessC
             }
         }
 
-        FileAccesses fileAccesses = _fileAccessRepository.FinishProject(nodeContext);
-
         // If file access reports are disabled in MSBuild we can't cache anything as we don't know what to cache.
         if (!_hasHadFileAccessReport)
         {
-            // We still want to dump the fingerprint to help debug cache misses.
-            // However, note that because we do not have the file accesses, we cannot know the PathSet.
-            // This means that any cache miss analysis will need to take this into account.
-            await DumpFingerprintLogAsync(logger, nodeContext, null);
             return;
         }
 
+        FileAccesses fileAccesses = _fileAccessRepository.FinishProject(nodeContext);
+
         // Package files are commonly just copied as outputs, so track the package inputs to compare with package outputs to avoid caching them.
         Dictionary<ContentHash, string> hashesToPackageFiles = new();
         List<Task<(byte[]?, string)>> packageFileHashingTasks = new();
@@ -743,22 +784,10 @@ await Task.WhenAll(
         }
 
         NodeDescriptor nodeDescriptor = _nodeDescriptorFactory.Create(fileAccessContext.ProjectFullPath, fileAccessContext.GlobalProperties);
-        if (!_nodeContexts.TryGetValue(nodeDescriptor, out NodeContext? nodeContext))
-        {
-            return null;
-        }
-
-        // Note: Checking if the targets we expect is a subset of the targets we got. InitialTargets in particular may cause extra targets to be executed.
-        // We will end up caching these extra results, but this is also intended as they do end up executing with the original request.
-        if (!nodeContext.TargetNames.IsSubsetOf(fileAccessContext.Targets))
-        {
-            return null;
-        }
-
-        return nodeContext;
+        return _nodeContexts.TryGetValue(nodeDescriptor, out NodeContext? nodeContext) ? nodeContext : null;
     }
 
-    private bool DoTargetsMatch(HashSet<string> expectedTargets, IEnumerable<string> requestedTargets)
+    private bool DoTargetsMatchForRead(HashSet<string> expectedTargets, IEnumerable<string> requestedTargets)
     {
         if (Settings is null || Settings.TargetsToIgnore.Count == 0)
         {
@@ -781,6 +810,13 @@ private bool DoTargetsMatch(HashSet<string> expectedTargets, IEnumerable<string>
         return extraTargets.Count == 0;
     }
 
+    private static bool DoTargetsMatchForWrite(HashSet<string> expectedTargets, IEnumerable<string> reportedTargets)
+    {
+        // Check if the targets we expect is a subset of the targets we got. InitialTargets in particular may cause extra targets to be executed.
+        // We will end up caching these extra results, but this is also intended as they do end up executing with the original request.
+        return expectedTargets.IsSubsetOf(reportedTargets);
+    }
+
     private async Task DumpNodeContextsAsync(PluginLoggerBase logger, Dictionary<NodeDescriptor, NodeContext> nodeContexts)
     {
         if (_nodeContexts is null)
@@ -899,15 +935,24 @@ private async Task DumpFingerprintLogAsync(
             return;
         }
 
+        Fingerprint? weakFingerprint = await FingerprintFactory.GetWeakFingerprintAsync(nodeContext);
+        if (weakFingerprint is null)
+        {
+            // Nothing to dump. Likely dependencies failed or cache missed.
+            return;
+        }
+
+        Fingerprint? strongFingerprint = await FingerprintFactory.GetStrongFingerprintAsync(pathSet);
+
         string filePath = Path.Combine(nodeContext.LogDirectory, "fingerprint.json");
         try
         {
             using FileStream fileStream = File.Create(filePath);
             await using var jsonWriter = new Utf8JsonWriter(fileStream, SerializationHelper.WriterOptions);
 
             jsonWriter.WriteStartObject();
-            WriteFingerprintJson(jsonWriter, "weak", await FingerprintFactory.GetWeakFingerprintAsync(nodeContext));
-            WriteFingerprintJson(jsonWriter, "strong", await FingerprintFactory.GetStrongFingerprintAsync(pathSet));
+            WriteFingerprintJson(jsonWriter, "weak", weakFingerprint);
+            WriteFingerprintJson(jsonWriter, "strong", strongFingerprint);
             jsonWriter.WriteEndObject();
         }
         catch (Exception ex)