Avoid materializing outputs for unqueried dependencies (#117)

* Avoid materializing outputs for recursively queried nodes

* Fix multitargeting

Author: dfederm

src/Common/Caching/CacheClient.cs

@@ -357,9 +357,10 @@ await AddNodeAsync(
 
     public async Task<(PathSet?, NodeBuildResult?)> GetNodeAsync(
         NodeContext nodeContext,
+        bool materializeOutputs,
         CancellationToken cancellationToken)
     {
-        (PathSet? PathSet, NodeBuildResult? NodeBuildResult) result = await GetNodeInternalAsync(nodeContext, cancellationToken);
+        (PathSet? PathSet, NodeBuildResult? NodeBuildResult) result = await GetNodeInternalAsync(nodeContext, materializeOutputs, cancellationToken);
 
         // On cache miss ensure all dependencies are materialized before returning to MSBuild so that MSBuild's execution will actually work.
         if (_enableAsyncMaterialization && result.NodeBuildResult == null)
@@ -378,6 +379,7 @@ await AddNodeAsync(
 
     public async Task<(PathSet?, NodeBuildResult?)> GetNodeInternalAsync(
         NodeContext nodeContext,
+        bool materializeOutputs,
         CancellationToken cancellationToken)
     {
         Context context = new(RootContext);
@@ -498,23 +500,26 @@ async Task PlaceFilesAsync(CancellationToken ct)
             }
         }
 
-        if (_enableAsyncMaterialization)
+        if (materializeOutputs)
         {
-            _materializationTasks.TryAdd(
-                nodeContext,
-                // Avoid using a cancellation token since MSBuild will cancel it when it thinks the build is finished and we await these tasks at that point.
-                // Note that this means that we effectively cannot cancel this operation once started and the user will have to wait.
-                Task.Run(
-                    async () =>
-                    {
-                        await PlaceFilesAsync(CancellationToken.None);
-                        _materializationTasks.TryRemove(nodeContext, out _);
-                    },
-                    CancellationToken.None));
-        }
-        else
-        {
-            await PlaceFilesAsync(cancellationToken);
+            if (_enableAsyncMaterialization)
+            {
+                _materializationTasks.TryAdd(
+                    nodeContext,
+                    // Avoid using a cancellation token since MSBuild will cancel it when it thinks the build is finished and we await these tasks at that point.
+                    // Note that this means that we effectively cannot cancel this operation once started and the user will have to wait.
+                    Task.Run(
+                        async () =>
+                        {
+                            await PlaceFilesAsync(CancellationToken.None);
+                            _materializationTasks.TryRemove(nodeContext, out _);
+                        },
+                        CancellationToken.None));
+            }
+            else
+            {
+                await PlaceFilesAsync(cancellationToken);
+            }
         }
 
         return (pathSet, nodeBuildResult);

src/Common/Caching/ICacheClient.cs

@@ -19,7 +19,7 @@ Task<NodeBuildResult> AddNodeAsync(
         Func<IReadOnlyDictionary<string, ContentHash>, NodeBuildResult> nodeBuildResultBuilder,
         CancellationToken cancellationToken);
 
-    Task<(PathSet?, NodeBuildResult?)> GetNodeAsync(NodeContext nodeContext, CancellationToken cancellationToken);
+    Task<(PathSet?, NodeBuildResult?)> GetNodeAsync(NodeContext nodeContext, bool materializeOutputs, CancellationToken cancellationToken);
 
     Task ShutdownAsync(CancellationToken cancellationToken);
 }

src/Common/MSBuildCachePluginBase.cs

@@ -334,7 +334,7 @@ private async Task BeginBuildInnerAsync(CacheContext context, PluginLoggerBase l
         if (Settings.GetResultsForUnqueriedDependencies)
         {
             ConcurrentDictionary<NodeContext, Lazy<Task<CacheResult>>> cacheResults = new(concurrencyLevel: Environment.ProcessorCount, _nodeContexts.Count);
-            _getCacheResultAsync = (nodeContext, logger, cancellationToken) => GetCacheResultRecursivelyAsync(cacheResults, nodeContext, logger, cancellationToken);
+            _getCacheResultAsync = (nodeContext, logger, cancellationToken) => GetCacheResultRecursivelyAsync(cacheResults, nodeContext, materializeOutputs: true, logger, cancellationToken);
         }
         else
         {
@@ -405,6 +405,7 @@ private async Task<CacheResult> GetCacheResultInnerAsync(BuildRequestData buildR
     private async Task<CacheResult> GetCacheResultRecursivelyAsync(
         ConcurrentDictionary<NodeContext, Lazy<Task<CacheResult>>> cacheResults,
         NodeContext nodeContext,
+        bool materializeOutputs,
         PluginLoggerBase logger,
         CancellationToken cancellationToken)
     {
@@ -413,12 +414,21 @@ private async Task<CacheResult> GetCacheResultRecursivelyAsync(
         return await cacheResults.GetOrAdd(nodeContext, new Lazy<Task<CacheResult>>(
             async () =>
             {
+                bool isOuterBuild = nodeContext.ProjectInstance.IsOuterBuild();
+
                 foreach (NodeContext dependency in nodeContext.Dependencies)
                 {
                     if (dependency.BuildResult == null)
                     {
+                        // When querying recursively, avoid materializing the outputs. That node was never directly queried, so its outputs
+                        // are not desired from the caller. Note that there is an assumption that the node won't be queried directly later
+                        // as that would break the expected "bottom-up" build order of graph builds.
+                        // Special-case the outer build of a multitargeting project which dependencies on the inner builds for which we do
+                        // want the outputs.
+                        bool materializeOutputs = isOuterBuild && dependency.ProjectInstance.IsInnerBuild();
+
                         logger.LogMessage($"Querying cache for missing build result for dependency '{dependency.Id}'");
-                        CacheResult dependencyResult = await GetCacheResultRecursivelyAsync(cacheResults, dependency, logger, cancellationToken);
+                        CacheResult dependencyResult = await GetCacheResultRecursivelyAsync(cacheResults, dependency, materializeOutputs, logger, cancellationToken);
                         logger.LogMessage($"Dependency '{dependency.Id}' cache result: '{dependencyResult.ResultType}'");
 
                         if (dependencyResult.ResultType != CacheResultType.CacheHit)
@@ -430,7 +440,7 @@ private async Task<CacheResult> GetCacheResultRecursivelyAsync(
                     }
                 }
 
-                return await GetCacheResultSingleAsync(nodeContext, logger, cancellationToken);
+                return await GetCacheResultSingleAsync(nodeContext, materializeOutputs, logger, cancellationToken);
             })).Value;
     }
 
@@ -446,10 +456,10 @@ private async Task<CacheResult> GetCacheResultNonRecursiveAsync(NodeContext node
             }
         }
 
-        return await GetCacheResultSingleAsync(nodeContext, logger, cancellationToken);
+        return await GetCacheResultSingleAsync(nodeContext, materializeOutputs: true, logger, cancellationToken);
     }
 
-    private async Task<CacheResult> GetCacheResultSingleAsync(NodeContext nodeContext, PluginLoggerBase logger, CancellationToken cancellationToken)
+    private async Task<CacheResult> GetCacheResultSingleAsync(NodeContext nodeContext, bool materializeOutputs, PluginLoggerBase logger, CancellationToken cancellationToken)
     {
         if (!Initialized)
         {
@@ -458,7 +468,7 @@ private async Task<CacheResult> GetCacheResultSingleAsync(NodeContext nodeContex
 
         nodeContext.SetStartTime();
 
-        (PathSet? pathSet, NodeBuildResult? nodeBuildResult) = await _cacheClient.GetNodeAsync(nodeContext, cancellationToken);
+        (PathSet? pathSet, NodeBuildResult? nodeBuildResult) = await _cacheClient.GetNodeAsync(nodeContext, materializeOutputs, cancellationToken);
         if (nodeBuildResult is null)
         {
             Interlocked.Increment(ref _cacheMissCount);

src/Common/ProjectInstanceExtensions.cs

@@ -0,0 +1,57 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System;
+using Microsoft.Build.Execution;
+
+namespace Microsoft.MSBuildCache;
+
+internal static class ProjectInstanceExtensions
+{
+    /// <summary>
+    /// Parses a property that may contain a bool, returning the default value if the property is not
+    /// set, or returns false if the property is set to 'false', otherwise returning true.
+    /// </summary>
+    public static bool GetBoolPropertyValue(this ProjectInstance projectInstance, string propName, bool defaultValue = false)
+    {
+        string prop = projectInstance.GetPropertyValue(propName);
+        if (string.IsNullOrWhiteSpace(prop))
+        {
+            return defaultValue;
+        }
+
+        if (string.Equals(prop, "false", StringComparison.OrdinalIgnoreCase))
+        {
+            return false;
+        }
+
+        return true;
+    }
+
+    public static bool IsPlatformNegotiationBuild(this ProjectInstance projectInstance)
+    {
+        // the fact that there is zero global properties here allows us to determine this was an evaluation done for the sole use of determining a referenced project's compatible platforms. We know that this is a build for this use
+        // because "IsGraphBuild" will be set to true in the global properties of all other project instances.
+        return projectInstance.IsPlatformNegotiationEnabled() && projectInstance.GlobalProperties.Count == 0;
+    }
+
+    public static bool IsPlatformNegotiationEnabled(this ProjectInstance projectInstance)
+    {
+        // Determine whether or not the platform negotiation is turned on in msbuild
+        return !string.IsNullOrWhiteSpace(projectInstance.GetPropertyValue("EnableDynamicPlatformResolution"));
+    }
+
+    public static bool IsInnerBuild(this ProjectInstance projectInstance)
+    {
+        // This follows the logic of MSBuild's ProjectInterpretation.GetProjectType.
+        // See: https://github.com/microsoft/msbuild/blob/master/src/Build/Graph/ProjectInterpretation.cs
+        return !projectInstance.IsPlatformNegotiationBuild() && !string.IsNullOrWhiteSpace(projectInstance.GetPropertyValue(projectInstance.GetPropertyValue("InnerBuildProperty"))) && !string.IsNullOrWhiteSpace(projectInstance.GetPropertyValue(projectInstance.GetPropertyValue("InnerBuildPropertyValues")));
+    }
+
+    public static bool IsOuterBuild(this ProjectInstance projectInstance)
+    {
+        // This follows the logic of MSBuild's ProjectInterpretation.GetProjectType.
+        // See: https://github.com/microsoft/msbuild/blob/master/src/Build/Graph/ProjectInterpretation.cs
+        return !projectInstance.IsPlatformNegotiationBuild() && !projectInstance.IsInnerBuild() && !string.IsNullOrWhiteSpace(projectInstance.GetPropertyValue(projectInstance.GetPropertyValue("InnerBuildPropertyValues")));
+    }
+}