Populate unproxied targets to avoid dropping requested targets (dotnet#9130)

Fixes dotnet#9117

For project cache plugins to only partially handle a build request, it makes sense for it to only proxy some targets and not others. For example, in VS the build request has:

"Build"
"BuiltProjectOutputGroup"
"BuiltProjectOutputGroupDependencies"
"DebugSymbolsProjectOutputGroup"
"DebugSymbolsProjectOutputGroupDependencies"
"DocumentationProjectOutputGroup"
"DocumentationProjectOutputGroupDependencies"
"SatelliteDllsProjectOutputGroup"
"SatelliteDllsProjectOutputGroupDependencies"
"SGenFilesOutputGroup"
"SGenFilesOutputGroupDependencies"
"Build" is the only relevant one that a plugin would want to handle, and would likely redirect to "GetTargetPath", while the rest are "information gathering" targets which should just be "passed through" and allowed to execute as-is.

Today if the plugin does this, the targets it does not proxy are dropped entire, which would make the caller confused about not getting results for a target they specifically requested. This change instead just fills in the remaining targets which were not proxied.

Example:
Original request: Build, BuiltProjectOutputGroup, BuiltProjectOutputGroupDependencies, ...
Cache plugin returns proxy targets Build -> GetTargetPath, and nothing more.
New request: GetTargetPath, BuiltProjectOutputGroup, BuiltProjectOutputGroupDependencies, ...

This fixes dotnet#9117 indirectly by not requiring a plugin to handle targets it wants to pass through at all, allowing it to just ignore them.

Author: dfederm

src/Build.UnitTests/BackEnd/Scheduler_Tests.cs

@@ -4,13 +4,15 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using System.Xml;
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.Evaluation;
 using Microsoft.Build.Execution;
 using Microsoft.Build.Experimental.ProjectCache;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
+using Microsoft.Build.Unittest;
 using Shouldly;
 using Xunit;
 using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem;
@@ -19,8 +21,6 @@
 
 namespace Microsoft.Build.UnitTests.BackEnd
 {
-    using Microsoft.Build.Unittest;
-
     /// <summary>
     /// Tests of the scheduler.
     /// </summary>
@@ -544,7 +544,13 @@ public void TestProxyAffinityIsInProc()
 
             CreateConfiguration(1, "foo.csproj");
 
-            BuildRequest request1 = CreateProxyBuildRequest(1, 1, new ProxyTargets(new Dictionary<string, string> { { "foo", "bar" } }), null);
+            BuildRequest request1 = CreateBuildRequest(
+                nodeRequestId: 1,
+                configId: 1,
+                targets: new[] { "foo" },
+                NodeAffinity.Any,
+                parentRequest: null,
+                new ProxyTargets(new Dictionary<string, string> { { "foo", "bar" } }));
 
             BuildRequestBlocker blocker = new BuildRequestBlocker(-1, Array.Empty<string>(), new[] { request1 });
             List<ScheduleResponse> response = new List<ScheduleResponse>(_scheduler.ReportRequestBlocked(1, blocker));
@@ -806,8 +812,6 @@ private BuildRequest CreateBuildRequest(int nodeRequestId, int configId, string[
         /// </summary>
         private BuildRequest CreateBuildRequest(int nodeRequestId, int configId, string[] targets, NodeAffinity nodeAffinity, BuildRequest parentRequest, ProxyTargets proxyTargets = null)
         {
-            (targets == null ^ proxyTargets == null).ShouldBeTrue();
-
             HostServices hostServices = null;
 
             if (nodeAffinity != NodeAffinity.Any)
@@ -816,36 +820,26 @@ private BuildRequest CreateBuildRequest(int nodeRequestId, int configId, string[
                 hostServices.SetNodeAffinity(String.Empty, nodeAffinity);
             }
 
-            if (targets != null)
+            if (proxyTargets != null)
             {
+                parentRequest.ShouldBeNull();
                 return new BuildRequest(
                     submissionId: 1,
                     nodeRequestId,
                     configId,
-                    targets,
-                    hostServices,
-                    BuildEventContext.Invalid,
-                    parentRequest);
+                    proxyTargets,
+                    targets.ToList(),
+                    hostServices);
             }
 
-            parentRequest.ShouldBeNull();
             return new BuildRequest(
                 submissionId: 1,
                 nodeRequestId,
                 configId,
-                proxyTargets,
-                hostServices);
-        }
-
-        private BuildRequest CreateProxyBuildRequest(int nodeRequestId, int configId, ProxyTargets proxyTargets, BuildRequest parentRequest)
-        {
-            return CreateBuildRequest(
-                nodeRequestId,
-                configId,
-                null,
-                NodeAffinity.Any,
-                parentRequest,
-                proxyTargets);
+                targets,
+                hostServices,
+                BuildEventContext.Invalid,
+                parentRequest);
         }
 
         /// <summary>

src/Build.UnitTests/ProjectCache/ProjectCacheTests.cs

@@ -507,7 +507,6 @@ public void ProjectCacheByBuildParametersAndBottomUpBuildWorks(GraphCacheRespons
                 }
             }
 
-
             AssertCacheBuild(graph, testData, mockCache, logger, nodesToBuildResults, targets: null);
         }
 
@@ -766,6 +765,59 @@ public void RunningProxyBuildsOnOutOfProcNodesShouldIssueWarning(bool disableInp
             logger.AssertMessageCount("MSB4274", 1);
         }
 
+        // A common scenario is to get a request for N targets, but only some of them can be handled by the cache.
+        // In this case, missing targets should be passed through.
+        [Fact]
+        public async Task PartialProxyTargets()
+        {
+            const string ProjectContent = """
+                <Project>
+                  <Target Name="SomeTarget">
+                    <Message Text="SomeTarget running" />
+                  </Target>
+                  <Target Name="ProxyTarget">
+                    <Message Text="ProxyTarget running" />
+                  </Target>
+                  <Target Name="SomeOtherTarget">
+                    <Message Text="SomeOtherTarget running" />
+                  </Target>
+                </Project>
+                """;
+            TransientTestFile project = _env.CreateFile($"project.proj", ProjectContent);
+
+            BuildParameters buildParameters = new()
+            {
+                ProjectCacheDescriptor = ProjectCacheDescriptor.FromInstance(
+                    new ConfigurableMockCache
+                    {
+                        GetCacheResultImplementation = (_, _, _) =>
+                        {
+                            return Task.FromResult(
+                                CacheResult.IndicateCacheHit(
+                                    new ProxyTargets(
+                                        new Dictionary<string, string>
+                                        {
+                                            { "ProxyTarget", "SomeTarget" },
+                                        })));
+                        }
+                    }),
+            };
+
+            MockLogger logger;
+            using (Helpers.BuildManagerSession buildSession = new(_env, buildParameters))
+            {
+                logger = buildSession.Logger;
+                BuildResult buildResult = await buildSession.BuildProjectFileAsync(project.Path, new[] { "SomeTarget", "SomeOtherTarget" });
+
+                buildResult.Exception.ShouldBeNull();
+                buildResult.ShouldHaveSucceeded();
+            }
+
+            logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("SomeTarget running");
+            logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("ProxyTarget running");
+            logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("SomeOtherTarget running");
+        }
+
         private void AssertCacheBuild(
             ProjectGraph graph,
             GraphCacheResponse testData,

src/Build/BackEnd/BuildManager/BuildManager.cs

@@ -1783,15 +1783,30 @@ private static void AddBuildRequestToSubmission(BuildSubmission submission, int
 
         private static void AddProxyBuildRequestToSubmission(
             BuildSubmission submission,
-            int configurationId,
+            BuildRequestConfiguration configuration,
             ProxyTargets proxyTargets,
             int projectContextId)
         {
+            IReadOnlyDictionary<string, string> realTargetsToProxyTargets = proxyTargets.RealTargetToProxyTargetMap;
+
+            ICollection<string> requestedTargets = submission.BuildRequestData.TargetNames.Count > 0
+                ? submission.BuildRequestData.TargetNames
+                : configuration.Project.DefaultTargets;
+            List<string> targets = new(requestedTargets.Count);
+            foreach (string requestedTarget in requestedTargets)
+            {
+                string effectiveTarget = realTargetsToProxyTargets.TryGetValue(requestedTarget, out string proxyTarget)
+                    ? proxyTarget
+                    : requestedTarget;
+                targets.Add(effectiveTarget);
+            }
+
             submission.BuildRequest = new BuildRequest(
                 submission.SubmissionId,
                 BackEnd.BuildRequest.InvalidNodeRequestId,
-                configurationId,
+                configuration.ConfigurationId,
                 proxyTargets,
+                targets,
                 submission.BuildRequestData.HostServices,
                 submission.BuildRequestData.Flags,
                 submission.BuildRequestData.RequestedProjectState,
@@ -2294,7 +2309,7 @@ void HandleCacheResult()
                         {
                             // Setup submission.BuildRequest with proxy targets. The proxy request is built on the inproc node (to avoid
                             // ProjectInstance serialization). The proxy target results are used as results for the real targets.
-                            AddProxyBuildRequestToSubmission(submission, configuration.ConfigurationId, cacheResult.ProxyTargets, projectContextId);
+                            AddProxyBuildRequestToSubmission(submission, configuration, cacheResult.ProxyTargets, projectContextId);
                             IssueBuildRequestForBuildSubmission(submission, configuration, allowMainThreadBuild: false);
                         }
                         else if (cacheResult.ResultType == CacheResultType.CacheHit && cacheResult.BuildResult != null)

src/Build/BackEnd/Components/ProjectCache/ProxyTargets.cs

@@ -27,6 +27,23 @@ public class ProxyTargets : ITranslatable
         /// </summary>
         public IReadOnlyDictionary<string, string> ProxyTargetToRealTargetMap => _proxyTargetToRealTargetMap;
 
+        internal IReadOnlyDictionary<string, string> RealTargetToProxyTargetMap
+        {
+            get
+            {
+                // The ProxyTargetToRealTargetMap is "backwards" from how most users would want to use it and doesn't provide as much flexibility as it could if reversed.
+                // Unfortunately this is part of a public API so cannot easily change at this point.
+                Dictionary<string, string> realTargetsToProxyTargets = new(ProxyTargetToRealTargetMap.Count, StringComparer.OrdinalIgnoreCase);
+                foreach (KeyValuePair<string, string> kvp in ProxyTargetToRealTargetMap)
+                {
+                    // In the case of multiple proxy targets pointing to the same real target, the last one wins. Another awkwardness of ProxyTargetToRealTargetMap being "backwards".
+                    realTargetsToProxyTargets[kvp.Value] = kvp.Key;
+                }
+
+                return realTargetsToProxyTargets;
+            }
+        }
+
         private ProxyTargets()
         {
         }

src/Build/BackEnd/Shared/BuildRequest.cs

@@ -128,6 +128,7 @@ private BuildRequest(
         /// <param name="nodeRequestId">The id of the node issuing the request</param>
         /// <param name="configurationId">The configuration id to use.</param>
         /// <param name="proxyTargets"><see cref="ProxyTargets"/></param>
+        /// <param name="targets">The set of targets to execute</param>
         /// <param name="hostServices">Host services if any. May be null.</param>
         /// <param name="buildRequestDataFlags">Additional flags for the request.</param>
         /// <param name="requestedProjectState">Filter for desired build results.</param>
@@ -137,14 +138,15 @@ public BuildRequest(
             int nodeRequestId,
             int configurationId,
             ProxyTargets proxyTargets,
+            List<string> targets,
             HostServices hostServices,
             BuildRequestDataFlags buildRequestDataFlags = BuildRequestDataFlags.None,
             RequestedProjectState requestedProjectState = null,
             int projectContextId = BuildEventContext.InvalidProjectContextId)
             : this(submissionId, nodeRequestId, configurationId, hostServices, buildRequestDataFlags, requestedProjectState, projectContextId)
         {
             _proxyTargets = proxyTargets;
-            _targets = proxyTargets.ProxyTargetToRealTargetMap.Keys.ToList();
+            _targets = targets;
 
             // Only root requests can have proxy targets.
             _parentGlobalRequestId = InvalidGlobalRequestId;

src/Build/BackEnd/Shared/BuildRequestConfiguration.cs

@@ -745,13 +745,6 @@ public List<string> GetTargetsUsedToBuildRequest(BuildRequest request)
             ErrorUtilities.VerifyThrow(_projectInitialTargets != null, "Initial targets have not been set.");
             ErrorUtilities.VerifyThrow(_projectDefaultTargets != null, "Default targets have not been set.");
 
-            if (request.ProxyTargets != null)
-            {
-                ErrorUtilities.VerifyThrow(
-                    CollectionHelpers.SetEquivalent(request.Targets, request.ProxyTargets.ProxyTargetToRealTargetMap.Keys),
-                    "Targets must be same as proxy targets");
-            }
-
             List<string> initialTargets = _projectInitialTargets;
             List<string> nonInitialTargets = (request.Targets.Count == 0) ? _projectDefaultTargets : request.Targets;