Don't publish assets with Vertical visibility as part of the merged manifest for a vertical. (#45553)

jkoritzinsky · web-flow · commit 388e0a81ed3a · 2024-12-20T00:42:14.000Z
diff --git a/src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/GetKnownArtifactsFromAssetManifests.cs b/src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/GetKnownArtifactsFromAssetManifests.cs
@@ -24,6 +24,8 @@ public sealed class GetKnownArtifactsFromAssetManifests : Build.Utilities.Task
     private const string RepoOriginAttributeName = "RepoOrigin";
     private const string NonShippingAttributeName = "NonShipping";
     private const string DotNetReleaseShippingAttributeName = "DotNetReleaseShipping";
+    private const string VisibilityAttributeName = "Visibility";
+    private const string DefaultVisibility = "External";
 
     // Package metadata
     private const string PackageElementName = "Package";
@@ -69,7 +71,8 @@ public override bool Execute()
                 { PackageVersionAttributeName, package.Attribute(PackageVersionAttributeName)!.Value },
                 { RepoOriginAttributeName, package.Attribute(RepoOriginAttributeName)?.Value ?? string.Empty },
                 { NonShippingAttributeName, package.Attribute(NonShippingAttributeName)?.Value ?? string.Empty },
-                { DotNetReleaseShippingAttributeName, package.Attribute(DotNetReleaseShippingAttributeName)?.Value ?? string.Empty }
+                { DotNetReleaseShippingAttributeName, package.Attribute(DotNetReleaseShippingAttributeName)?.Value ?? string.Empty },
+                { VisibilityAttributeName, package.Attribute(VisibilityAttributeName)?.Value ?? DefaultVisibility },
             }))
             .Distinct(TaskItemManifestEqualityComparer.Instance)
             .ToArray();
@@ -81,7 +84,8 @@ public override bool Execute()
             {
                 { RepoOriginAttributeName, blob.Attribute(RepoOriginAttributeName)?.Value ?? string.Empty },
                 { NonShippingAttributeName, blob.Attribute(NonShippingAttributeName)?.Value ?? string.Empty },
-                { DotNetReleaseShippingAttributeName, blob.Attribute(DotNetReleaseShippingAttributeName)?.Value ?? string.Empty }
+                { DotNetReleaseShippingAttributeName, blob.Attribute(DotNetReleaseShippingAttributeName)?.Value ?? string.Empty },
+                { VisibilityAttributeName, blob.Attribute(VisibilityAttributeName)?.Value ?? DefaultVisibility },
             }))
             .Distinct(TaskItemManifestEqualityComparer.Instance)
             .ToArray();
@@ -110,4 +114,4 @@ public bool Equals(TaskItem? x, TaskItem? y)
 
         public int GetHashCode([DisallowNull] TaskItem obj) => HashCode.Combine(obj.ItemSpec, obj.GetMetadata(PackageVersionAttributeName));
     }
-}
+}
diff --git a/src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs b/src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs
@@ -68,6 +68,10 @@ public class JoinVerticals : Microsoft.Build.Utilities.Task
     private const string _packageElementName = "Package";
     private const string _blobElementName = "Blob";
     private const string _idAttribute = "Id";
+    private const string _visibilityAttribute = "Visibility";
+    private const string _externalVisibility = "External";
+    private const string _internalVisibility = "Internal";
+    private const string _verticalVisibility = "Vertical";
     private const string _verticalNameAttribute = "VerticalName";
     private const string _artifactNameSuffix = "_Artifacts";
     private const string _assetsFolderName = "assets";
@@ -110,7 +114,7 @@ private async Task ExecuteAsync()
 
         using var clientThrottle = new SemaphoreSlim(16, 16);
         List<Task> downloadTasks = new();
-        
+
         foreach (XDocument verticalManifest in verticalManifests)
         {
             string verticalName = GetRequiredRootAttribute(verticalManifest, _verticalNameAttribute);
@@ -188,28 +192,28 @@ private async Task DownloadArtifactFiles(
 
         await Task.WhenAll(fileNamesToDownload.Select(async fileName =>
             await DownloadFileFromArtifact(
-                filesInformation, 
-                artifactName, 
-                azureDevOpsClient, 
-                buildId, 
-                fileName, 
-                outputDirectory, 
+                filesInformation,
+                artifactName,
+                azureDevOpsClient,
+                buildId,
+                fileName,
+                outputDirectory,
                 clientThrottle)));
     }
 
     private async Task DownloadFileFromArtifact(
-        ArtifactFiles artifactFilesMetadata, 
-        string azureDevOpsArtifact, 
-        AzureDevOpsClient azureDevOpsClient, 
-        string buildId, 
-        string manifestFile, 
+        ArtifactFiles artifactFilesMetadata,
+        string azureDevOpsArtifact,
+        AzureDevOpsClient azureDevOpsClient,
+        string buildId,
+        string manifestFile,
         string destinationDirectory,
         SemaphoreSlim clientThrottle)
     {
         try
         {
             await clientThrottle.WaitAsync();
-            
+
             ArtifactItem fileItem;
 
             var matchingFilePaths = artifactFilesMetadata.Items.Where(f => Path.GetFileName(f.Path) == Path.GetFileName(manifestFile));
@@ -260,7 +264,7 @@ private async Task DownloadFileFromArtifact(
     }
 
     /// <summary>
-    /// Copy all files from the source directory to the destination directory, 
+    /// Copy all files from the source directory to the destination directory,
     /// in a flat layout
     /// </summary>
     private void CopyMainVerticalAssets(string sourceDirectory, string destinationDirectory)
@@ -272,7 +276,7 @@ private void CopyMainVerticalAssets(string sourceDirectory, string destinationDi
             Directory.CreateDirectory(destinationDirectory);
         }
 
-        foreach (var sourceFile in sourceFiles) 
+        foreach (var sourceFile in sourceFiles)
         {
             string destinationFilePath = Path.Combine(destinationDirectory, Path.GetFileName(sourceFile));
 
@@ -291,11 +295,35 @@ private List<string> AddMissingElements(Dictionary<string, AddedElement> addedAr
 
         string verticalName = verticalManifest.Root!.Attribute(_verticalNameAttribute)!.Value;
 
-        foreach (XElement artifactElement in verticalManifest.Descendants(elementName)) 
+        foreach (XElement artifactElement in verticalManifest.Descendants(elementName))
         {
-            string elementId = artifactElement.Attribute(_idAttribute)?.Value 
+            string elementId = artifactElement.Attribute(_idAttribute)?.Value
                 ?? throw new ArgumentException($"Required attribute '{_idAttribute}' not found in {elementName} element.");
 
+            // Filter out artifacts that are not "External" visibility.
+            // Artifacts of "Vertical" visibility should have been filtered out in each individual vertical,
+            // but artifacts of "Internal" visibility would have been included in each vertical's manifest (to enable feeding into join verticals).
+            // We need to remove them here so they don't get included in the final merged manifest.
+            // As we're in the final join, there should be no jobs after us. Therefore, we can also skip uploading them to the final artifacts folders
+            // as no job should run after this job that would consume them.
+            string? visibility = artifactElement.Attribute(_visibilityAttribute)?.Value;
+            
+            if (visibility == _verticalVisibility)
+            {
+                Log.LogError($"Artifact {elementId} has 'Vertical' visibility and should not be present in a vertical manifest.");
+                continue;
+            }
+            else if (visibility == _internalVisibility)
+            {
+                Log.LogMessage(MessageImportance.High, $"Artifact {elementId} has 'Internal' visibility and will not be included in the final merged manifest.");
+                continue;
+            }
+            else if (visibility is not (null or "" or _externalVisibility))
+            {
+                Log.LogError($"Artifact {elementId} has unknown visibility: '{visibility}'");
+                continue;
+            }
+
             if (addedArtifacts.TryAdd(elementId, new AddedElement(verticalName, artifactElement)))
             {
                 if (elementName == _packageElementName)
@@ -327,7 +355,7 @@ private List<string> AddMissingElements(Dictionary<string, AddedElement> addedAr
 
     private static string GetRequiredRootAttribute(XDocument document, string attributeName)
     {
-        return document.Root?.Attribute(attributeName)?.Value 
+        return document.Root?.Attribute(attributeName)?.Value
             ?? throw new ArgumentException($"Required attribute '{attributeName}' not found in root element.");
     }
 
diff --git a/src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/MergeAssetManifests.cs b/src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/MergeAssetManifests.cs
@@ -54,7 +54,7 @@ public override bool Execute()
 
             VerifyAssetManifests(assetManifestXmls);
 
-            XElement mergedManifestRoot = assetManifestXmls.First().Root 
+            XElement mergedManifestRoot = assetManifestXmls.First().Root
                 ?? throw new ArgumentException("The root element of the asset manifest is null.");
 
             // Set the BuildId and AzureDevOpsBuildNumber attributes to the value of VmrBuildNumber
@@ -67,10 +67,14 @@ public override bool Execute()
 
             foreach (var assetManifestXml in assetManifestXmls)
             {
-                packageElements.AddRange(assetManifestXml.Descendants("Package"));
-                blobElements.AddRange(assetManifestXml.Descendants("Blob"));
+                // We may encounter assets here with "Vertical", "Internal", or "External" visibility here.
+                // We filter out "Vertical" visibility assets here, as they are not needed in the merged manifest.
+                // We leave in "Internal" assets so they can be used in later build passes.
+                // We leave in "External" assets as we will eventually ship them.
+                packageElements.AddRange(assetManifestXml.Descendants("Package").Where(package => package.Attribute("Visibility")?.Value != "Vertical"));
+                blobElements.AddRange(assetManifestXml.Descendants("Blob").Where(blob => blob.Attribute("Visibility")?.Value != "Vertical"));
             }
-            
+
             packageElements = packageElements.OrderBy(packageElement => packageElement.Attribute("Id")?.Value).ToList();
             blobElements = blobElements.OrderBy(blobElement => blobElement.Attribute("Id")?.Value).ToList();
 
@@ -97,18 +101,18 @@ private static void VerifyAssetManifests(IReadOnlyList<XDocument> assetManifestX
                 .Root?
                 .Attributes()
                 .Select(attribute => attribute.ToString())
-                .ToHashSet() 
+                .ToHashSet()
                 ?? throw new ArgumentException("The root element of the asset manifest is null.");
 
             if (assetManifestXmls.Skip(1).Any(manifest => manifest.Root?.Attributes().Count() != rootAttributes.Count))
             {
                 throw new ArgumentException("The asset manifests do not have the same number of root attributes.");
             }
 
-            if (assetManifestXmls.Skip(1).Any(assetManifestXml => 
+            if (assetManifestXmls.Skip(1).Any(assetManifestXml =>
                     !assetManifestXml.Root?.Attributes().Select(attribute => attribute.ToString())
                         .All(attribute =>
-                            // Ignore BuildId and AzureDevOpsBuildNumber attributes, they're different for different repos, 
+                            // Ignore BuildId and AzureDevOpsBuildNumber attributes, they're different for different repos,
                             // TODO this should be fixed with https://github.com/dotnet/source-build/issues/3934
                             _ignoredAttributes.Any(ignoredAttribute => attribute.StartsWith(ignoredAttribute)) || rootAttributes.Contains(attribute))
                         ?? false))
diff --git a/src/SourceBuild/content/repo-projects/Directory.Build.targets b/src/SourceBuild/content/repo-projects/Directory.Build.targets
@@ -554,9 +554,13 @@
         <ShippingFolder Condition="%(ProducedPackage.NonShipping) != 'true'">Shipping</ShippingFolder>
       </ProducedPackage>
 
-      <!-- When building from source, the Private.SourceBuilt.Artifacts archive already contains the nuget packages. -->
-      <BinPlaceFile Include="@(ProducedPackage->'$(ArtifactsPackagesDir)%(ShippingFolder)/$(RepositoryName)/%(Identity).%(Version).nupkg')" Condition="'$(DotNetBuildSourceOnly)' != 'true'" />
-      <BinPlaceFile Include="@(ProducedAsset->'$(ArtifactsAssetsDir)%(Identity)')" />
+      <!--
+        When building from source, the Private.SourceBuilt.Artifacts archive already contains the nuget packages so no need to binplace.
+        In addition, don't binplace any packages or blobs with vertical visibility. These are only used for building other repos within this vertical
+        and shouldn't be staged for upload.
+      -->
+      <BinPlaceFile Include="@(ProducedPackage->'$(ArtifactsPackagesDir)%(ShippingFolder)/$(RepositoryName)/%(Identity).%(Version).nupkg')" Condition="'$(DotNetBuildSourceOnly)' != 'true' and '%(Visibility)' != 'Vertical'" />
+      <BinPlaceFile Include="@(ProducedAsset->'$(ArtifactsAssetsDir)%(Identity)')" Condition="'%(Visibility)' != 'Vertical'"/>
     </ItemGroup>
 
     <!-- Add manifests from dependent verticals. -->
