Merge pull request #9454 from drewnoakes/fix-9001-duplicate-copy-items-with-ba

Build Acceleration handles duplicate copy items

Author: drewnoakes

docs/build-acceleration.md

@@ -92,9 +92,11 @@ Build acceleration runs with the FUTDC, and outputs details of its operation in
 > Tools | Options | Projects and Solutions | SDK-Style Projects
 
 _Traditional view:_
+
 <img src="repo/images/options.png" width="528" alt="SDK-style project options, in the legacy settings view">
 
 _Unified settings view:_
+
 <img src="repo/images/options-unified.png" width="528" alt="SDK-style project options, in the modern unified settings view">
 
 Setting _Logging Level_ to a value other than `None` results in messages prefixed with `FastUpToDate:` in Visual Studio's build output.
@@ -140,6 +142,12 @@ Looking through the build output with the following points in mind:
 
    Then any project that references the indicated project (directly or transitively) cannot be accelerated. This can happen if the mentioned project uses the legacy `.csproj` format, or for any other project system within Visual Studio that doesn't support build acceleration. Currently only .NET SDK-style projects (loaded with the project system from this GitHub repository) provide the needed data.
 
+- ⛔ If you see:
+
+   > Build acceleration is not available for this project because it copies duplicate files to the output directory: '<path1>', '<path2>'
+
+   Then multiple projects want to copy the same file to the output directory. Currently, Build Acceleration does not attempt to discover which of these source files should win. Instead, when this situation occurs, Build Acceleration is disabled.
+
 - ⛔ If you see:
 
    > This project has enabled build acceleration, but not all referenced projects produce a reference assembly. Ensure projects producing the following outputs have the 'ProduceReferenceAssembly' MSBuild property set to 'true': '&lt;path1&gt;', '&lt;path2&gt;'.

spelling.dic

@@ -1,3 +1,4 @@
+appsettings
 args
 async
 awaiter
@@ -15,6 +16,7 @@ dirs
 docdata
 enum
 enums
+fsproj
 fsscript
 func
 futdcache
@@ -44,6 +46,7 @@ netcoreapp
 nuget
 pbstr
 phier
+pkgdef
 prev
 proj
 projectsystem

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/UpToDate/BuildUpToDateCheck.cs

@@ -955,7 +955,7 @@ private async Task<bool> IsUpToDateInternalAsync(
                 // The subscription object calls GetLatestVersionAsync for project data, which can take a while.
                 // We separate out the wait time from the overall time, so we can more easily identify when the
                 // wait is long, versus the check's actual execution time.
-                var waitTime = sw.Elapsed;
+                TimeSpan waitTime = sw.Elapsed;
 
                 token.ThrowIfCancellationRequested();
 
@@ -1077,7 +1077,7 @@ private async Task<bool> IsUpToDateInternalAsync(
                         // We check timestamps of whatever items we can find, but only perform acceleration when the full set is available.
                         CopyItemsResult copyInfo = _copyItemAggregator.TryGatherCopyItemsForProject(implicitState.ProjectTargetPath, logger);
 
-                        bool? isBuildAccelerationEnabled = await IsBuildAccelerationEnabledAsync(copyInfo.IsComplete, implicitState);
+                        bool? isBuildAccelerationEnabled = await IsBuildAccelerationEnabledAsync(copyInfo.IsComplete, copyInfo.DuplicateCopyItemRelativeTargetPaths, implicitState);
 
                         var configuredFileSystemOperations = new ConfiguredFileSystemOperationAggregator(fileSystemOperations, isBuildAccelerationEnabled, copyInfo.TargetsWithoutReferenceAssemblies);
 
@@ -1150,13 +1150,14 @@ private async Task<bool> IsUpToDateInternalAsync(
                     _lastCopyTargetsFromThisProject = copyItemPaths;
                 }
 
-                async ValueTask<bool?> IsBuildAccelerationEnabledAsync(bool isCopyItemsComplete, UpToDateCheckImplicitConfiguredInput implicitState)
+                async ValueTask<bool?> IsBuildAccelerationEnabledAsync(bool isCopyItemsComplete, IReadOnlyList<string>? duplicateCopyItemRelativeTargetPaths, UpToDateCheckImplicitConfiguredInput implicitState)
                 {
                     // Build acceleration requires:
                     //
                     // 1. being enabled, either in the project or via feature flags, and
                     // 2. having a full set of copy items, and
-                    // 3. not having any project references known to be incompatible with Build Acceleration.
+                    // 3. not having any project references known to be incompatible with Build Acceleration, and
+                    // 4. not having any duplicate copy items that would overwrite one another in the output directory (due to ordering issues).
                     //
                     // Being explicitly disabled in the project overrides any feature flag.
 
@@ -1180,11 +1181,13 @@ private async Task<bool> IsUpToDateInternalAsync(
 
                     bool isEnabled;
 
-                    if (isEnabledInProject is null)
+                    if (isEnabledInProject is bool b)
                     {
-                        // No value has been specified in the project. Look to the feature flag to determine
-                        // the default behavior in this case.
-
+                        isEnabled = b;
+                    }
+                    else
+                    {
+                        // No value has been specified in the project. Query the environment to decide (e.g. feature flag).
                         if (await _projectSystemOptions.IsBuildAccelerationEnabledByDefaultAsync(cancellationToken))
                         {
                             // The user has opted-in via feature flag. Set this to true and carry on with further checks.
@@ -1193,32 +1196,32 @@ private async Task<bool> IsUpToDateInternalAsync(
                         }
                         else
                         {
-                            logger.Verbose(nameof(VSResources.FUTD_BuildAccelerationIsNotEnabledForThisProject));
+                            logger.Info(nameof(VSResources.FUTD_BuildAccelerationIsNotEnabledForThisProject));
                             return null;
                         }
                     }
-                    else
-                    {
-                        isEnabled = isEnabledInProject.Value;
-                    }
 
                     if (isEnabled)
                     {
-                        if (isCopyItemsComplete)
+                        if (!isCopyItemsComplete)
                         {
-                            if (isEnabledInProject is not null)
-                            {
-                                // Don't log if isEnabledInProject is null, as we already log that status above
-                                logger.Info(nameof(VSResources.FUTD_BuildAccelerationEnabledViaProperty));
-                            }
-
-                            return true;
+                            logger.Info(nameof(VSResources.FUTD_AccelerationDisabledCopyItemsIncomplete));
+                            return false;
                         }
-                        else
+
+                        if (duplicateCopyItemRelativeTargetPaths is not null)
                         {
-                            logger.Info(nameof(VSResources.FUTD_AccelerationDisabledCopyItemsIncomplete));
+                            logger.Info(nameof(VSResources.FUTD_AccelerationDisabledDuplicateCopyItemsIncomplete_1), string.Join(", ", duplicateCopyItemRelativeTargetPaths.Select(path => $"'{path}'")));
                             return false;
                         }
+
+                        if (isEnabledInProject is not null)
+                        {
+                            // Don't log if isEnabledInProject is null, as we already log that status above.
+                            logger.Info(nameof(VSResources.FUTD_BuildAccelerationEnabledViaProperty));
+                        }
+
+                        return true;
                     }
                     else
                     {
@@ -1235,13 +1238,9 @@ public Task<bool> IsUpToDateCheckEnabledAsync(CancellationToken cancellationToke
             return _projectSystemOptions.GetIsFastUpToDateCheckEnabledAsync(cancellationToken);
         }
 
-        internal readonly struct TestAccessor
+        internal readonly struct TestAccessor(BuildUpToDateCheck check)
         {
-            private readonly BuildUpToDateCheck _check;
-
-            public TestAccessor(BuildUpToDateCheck check) => _check = check;
-
-            public void SetSubscription(ISubscription subscription) => _check._subscription = subscription;
+            public void SetSubscription(ISubscription subscription) => check._subscription = subscription;
         }
 
         /// <summary>For unit testing only.</summary>

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/UpToDate/CopyItemAggregator.cs

@@ -27,6 +27,8 @@ public CopyItemsResult TryGatherCopyItemsForProject(string targetPath, BuildUpTo
 
         // The queue of projects yet to be visited.
         Queue<string> frontier = new();
+
+        // Start with the requested project.
         frontier.Enqueue(targetPath);
 
         // If we find a reference to a project that did not call SetProjectData then we will not know the
@@ -59,6 +61,10 @@ public CopyItemsResult TryGatherCopyItemsForProject(string targetPath, BuildUpTo
 
                 if (!_projectData.TryGetValue(project, out ProjectCopyData data))
                 {
+                    // We don't have a list of project references for this project, so we cannot continue
+                    // walking the project reference tree. As we might not know all possible copy items,
+                    // we disable build acceleration. Note that we still walk the rest of the tree in order
+                    // to detect copy items, so that we can decide whether the project is up to date.
                     logger.Verbose(nameof(VSResources.FUTDC_AccelerationDataMissingForProject_1), project);
                     isComplete = false;
                     continue;
@@ -67,24 +73,25 @@ public CopyItemsResult TryGatherCopyItemsForProject(string targetPath, BuildUpTo
                 if (!data.ProduceReferenceAssembly && project != targetPath)
                 {
                     // One of the referenced projects does not produce a reference assembly.
-                    referencesNotProducingReferenceAssembly ??= new();
+                    referencesNotProducingReferenceAssembly ??= [];
                     referencesNotProducingReferenceAssembly.Add(data.TargetPath);
                 }
 
+                // Breadth-first traversal of the tree.
                 foreach (string referencedProjectTargetPath in data.ReferencedProjectTargetPaths)
                 {
                     frontier.Enqueue(referencedProjectTargetPath);
                 }
 
                 if (!data.CopyItems.IsEmpty)
                 {
-                    contributingProjects ??= new();
+                    contributingProjects ??= [];
                     contributingProjects.Add(data);
                 }
             }
         }
 
-        return new(GenerateCopyItems(), isComplete, referencesNotProducingReferenceAssembly);
+        return new(isComplete, GenerateCopyItems(), GetDuplicateCopyItems(), referencesNotProducingReferenceAssembly);
 
         IEnumerable<(string Path, ImmutableArray<CopyItem> CopyItems)> GenerateCopyItems()
         {
@@ -98,5 +105,29 @@ public CopyItemsResult TryGatherCopyItemsForProject(string targetPath, BuildUpTo
                 yield return (contributingProject.ProjectFullPath ?? contributingProject.TargetPath, contributingProject.CopyItems);
             }
         }
+
+        IReadOnlyList<string>? GetDuplicateCopyItems()
+        {
+            HashSet<string>? duplicates = null;
+
+            if (contributingProjects is not null)
+            {
+                HashSet<string> targetPaths = new(StringComparers.Paths);
+
+                foreach (ProjectCopyData contributingProject in contributingProjects)
+                {
+                    foreach (CopyItem copyItem in contributingProject.CopyItems)
+                    {
+                        if (!targetPaths.Add(copyItem.RelativeTargetPath))
+                        {
+                            duplicates ??= new(StringComparers.Paths);
+                            duplicates.Add(copyItem.RelativeTargetPath);
+                        }
+                    }
+                }
+            }
+
+            return duplicates?.ToList();
+        }
     }
 }

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/UpToDate/ICopyItemAggregator.cs

@@ -9,19 +9,32 @@ namespace Microsoft.VisualStudio.ProjectSystem.VS.UpToDate;
 internal interface ICopyItemAggregator
 {
     /// <summary>
-    /// Stores the set of copy items produced by the calling project, and modelled
-    /// in <paramref name="projectCopyData"/>.
+    /// Integrates data from a project, for use within <see cref="TryGatherCopyItemsForProject"/>.
     /// </summary>
-    /// <param name="projectCopyData">The set of items to copy, and some details about the provider project.</param>
+    /// <param name="projectCopyData">All necessary data about the project that's providing the data.</param>
     void SetProjectData(ProjectCopyData projectCopyData);
 
     /// <summary>
     /// Walks the project reference graph in order to obtain all reachable copy items
     /// from the project identified by <paramref name="targetPath"/>.
     /// </summary>
     /// <remarks>
+    /// <para>
     /// We use the <c>TargetPath</c> property to identify projects, as that path takes
-    /// other dimensions (such as target framework) into account.
+    /// other dimensions (such as target framework, platform, etc.) into account when projects
+    /// have multiple configurations.
+    /// </para>
+    /// <para>
+    /// When multiple copy items want to write to the same relative output path, the set of duplicate
+    /// items is written to <see cref="CopyItemsResult.DuplicateCopyItemRelativeTargetPaths"/>.
+    /// Build Acceleration disables itself when items exist in this collection, as it does not
+    /// attempt to reproduce the full behaviour of MSBuild for this scenario.
+    /// </para>
+    /// <para>
+    /// If we find a reference to a project that did not call <see cref="SetProjectData"/> then
+    /// the returned <see cref="CopyItemsResult.IsComplete"/> will be <see langword="false"/>.
+    /// Build Acceleration disables itself when copy items from a reachable project is unavailable.
+    /// </para>
     /// </remarks>
     /// <param name="targetPath">The target path of the project to query from.</param>
     /// <param name="logger">An object for writing log messages.</param>
@@ -33,15 +46,23 @@ internal interface ICopyItemAggregator
 /// Results of gathering the items that must be copied as part of a project's build
 /// by <see cref="ICopyItemAggregator.TryGatherCopyItemsForProject(string, BuildUpToDateCheck.Log)"/>.
 /// </summary>
-/// <param name="ItemsByProject">A sequence of items by project, that are reachable from the current project</param>
 /// <param name="IsComplete">Indicates whether we have items from all reachable projects.</param>
+/// <param name="ItemsByProject">
+///     A sequence of items by project, that are reachable from the current project. The path is that
+///     of the project file, such as <c>c:\repos\MyProject\MyProject.csproj</c>.
+/// </param>
+/// <param name="DuplicateCopyItemRelativeTargetPaths">
+///     A list of relative target paths for which more than one project produces an item, or <see langword="null"/> if
+///     no duplicates exists.
+/// </param>
 /// <param name="TargetsWithoutReferenceAssemblies">
 ///     A list of target paths for projects that do not produce reference assemblies, or <see langword="null"/> if
 ///     all reachable projects do in fact produce reference assemblies.
 /// </param>
 internal record struct CopyItemsResult(
-    IEnumerable<(string Path, ImmutableArray<CopyItem> CopyItems)> ItemsByProject,
     bool IsComplete,
+    IEnumerable<(string Path, ImmutableArray<CopyItem> CopyItems)> ItemsByProject,
+    IReadOnlyList<string>? DuplicateCopyItemRelativeTargetPaths,
     IReadOnlyList<string>? TargetsWithoutReferenceAssemblies);
 
 /// <summary>
@@ -59,5 +80,5 @@ internal record struct ProjectCopyData(
     ImmutableArray<CopyItem> CopyItems,
     ImmutableArray<string> ReferencedProjectTargetPaths)
 {
-    public bool IsDefault => CopyItems.IsDefault;
+    public readonly bool IsDefault => CopyItems.IsDefault;
 }

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/VSResources.Designer.cs

@@ -543,6 +543,10 @@ In order to debug this project, add an executable project to this solution which
   <data name="FUTD_AccelerationDisabledCopyItemsIncomplete" xml:space="preserve">
     <value>Build acceleration is not available for this project because not all transitively referenced projects have provided acceleration data.</value>
   </data>
+  <data name="FUTD_AccelerationDisabledDuplicateCopyItemsIncomplete_1" xml:space="preserve">
+    <value>Build acceleration is not available for this project because it copies duplicate files to the output directory: {0}</value>
+    <comment>{0} is a list of one or more relative file paths.</comment>
+  </data>
   <data name="FUTDC_AccelerationDataMissingForProject_1" xml:space="preserve">
     <value>Build acceleration data is unavailable for project with target '{0}'. See https://aka.ms/vs-build-acceleration.</value>
     <comment>{0} is a file path.</comment>