From 4b84ab1b8e6d87521b1b62eda582a7f96388bc7e Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 18 Dec 2024 13:17:22 -0800 Subject: [PATCH 1/5] Don't publish assets with Vertical visibility as part of the merged manifest for a vertical. --- .../MergeAssetManifests.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 index fa06f60c52b0..0190a028bb6d 100644 --- 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,10 @@ public override bool Execute() foreach (var assetManifestXml in assetManifestXmls) { - packageElements.AddRange(assetManifestXml.Descendants("Package")); - blobElements.AddRange(assetManifestXml.Descendants("Blob")); + 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,7 +97,7 @@ private static void VerifyAssetManifests(IReadOnlyList 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)) @@ -105,10 +105,10 @@ private static void VerifyAssetManifests(IReadOnlyList assetManifestX 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)) From f68a748c57d15a7cbc45ae8579d8973454f86078 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 18 Dec 2024 14:20:13 -0800 Subject: [PATCH 2/5] Don't binplace for upload any "Vertical" visibility assets. They shouldn't be uploaded from jobs and they won't be in the asset manifest anyway. --- .../GetKnownArtifactsFromAssetManifests.cs | 10 +++++++--- .../content/repo-projects/Directory.Build.targets | 10 +++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) 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 index 39f45023471a..28aa6f5ed48d 100644 --- 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)); } -} \ No newline at end of file +} diff --git a/src/SourceBuild/content/repo-projects/Directory.Build.targets b/src/SourceBuild/content/repo-projects/Directory.Build.targets index a954493e82c7..f5c60b83ed88 100644 --- a/src/SourceBuild/content/repo-projects/Directory.Build.targets +++ b/src/SourceBuild/content/repo-projects/Directory.Build.targets @@ -554,9 +554,13 @@ Shipping - - - + + + From 7227d011e87faed7b77cbb9c9579a608eb6398bb Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 19 Dec 2024 13:57:20 -0800 Subject: [PATCH 3/5] PR feedback and filter out internal assets in the final join. --- .../JoinVerticals.cs | 49 ++++++++++++------- .../MergeAssetManifests.cs | 4 ++ .../repo-projects/Directory.Build.targets | 4 +- 3 files changed, 37 insertions(+), 20 deletions(-) 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 index 8e2faef5c0b5..e6b5dc89de39 100644 --- 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,8 @@ 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 _verticalNameAttribute = "VerticalName"; private const string _artifactNameSuffix = "_Artifacts"; private const string _assetsFolderName = "assets"; @@ -110,7 +112,7 @@ private async Task ExecuteAsync() using var clientThrottle = new SemaphoreSlim(16, 16); List downloadTasks = new(); - + foreach (XDocument verticalManifest in verticalManifests) { string verticalName = GetRequiredRootAttribute(verticalManifest, _verticalNameAttribute); @@ -188,28 +190,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 +262,7 @@ private async Task DownloadFileFromArtifact( } /// - /// 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 /// private void CopyMainVerticalAssets(string sourceDirectory, string destinationDirectory) @@ -272,7 +274,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,9 +293,20 @@ private List AddMissingElements(Dictionary 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 + // 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. + if (artifactElement.Attribute(_visibilityAttribute)?.Value is not (null or "" or _externalVisibility)) + { + continue; + } + + string elementId = artifactElement.Attribute(_idAttribute)?.Value ?? throw new ArgumentException($"Required attribute '{_idAttribute}' not found in {elementName} element."); if (addedArtifacts.TryAdd(elementId, new AddedElement(verticalName, artifactElement))) @@ -327,7 +340,7 @@ private List AddMissingElements(Dictionary 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 index 0190a028bb6d..eea82cdb3c84 100644 --- 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 @@ -67,6 +67,10 @@ public override bool Execute() foreach (var assetManifestXml in assetManifestXmls) { + // 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")); } diff --git a/src/SourceBuild/content/repo-projects/Directory.Build.targets b/src/SourceBuild/content/repo-projects/Directory.Build.targets index f5c60b83ed88..691beaa00e77 100644 --- a/src/SourceBuild/content/repo-projects/Directory.Build.targets +++ b/src/SourceBuild/content/repo-projects/Directory.Build.targets @@ -555,8 +555,8 @@ From 20e99872b17556e686588a8611499c5b901bad76 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 19 Dec 2024 14:43:13 -0800 Subject: [PATCH 4/5] Add logging and validation for different visibilities. --- .../JoinVerticals.cs | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) 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 index e6b5dc89de39..4b12a26ebe72 100644 --- 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 @@ -70,6 +70,8 @@ public class JoinVerticals : Microsoft.Build.Utilities.Task 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"; @@ -295,20 +297,31 @@ private List AddMissingElements(Dictionary addedAr foreach (XElement artifactElement in verticalManifest.Descendants(elementName)) { + 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. - if (artifactElement.Attribute(_visibilityAttribute)?.Value is not (null or "" or _externalVisibility)) + 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."); + } + else if (visibility == _internalVisibility) { + Log.LogMessage(MessageImportance.High, $"Artifact {elementId} has 'Internal' visibility and will not be included in the final merged manifest."); + } + else if (visibility is not (null or "" or _externalVisibility)) + { + Log.LogError($"Artifact {elementId} has unknown visibility: '{visibility}'"); continue; } - string elementId = artifactElement.Attribute(_idAttribute)?.Value - ?? throw new ArgumentException($"Required attribute '{_idAttribute}' not found in {elementName} element."); - if (addedArtifacts.TryAdd(elementId, new AddedElement(verticalName, artifactElement))) { if (elementName == _packageElementName) From 3123ff39ee13cd374f29857a1eab79c01c507508 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 19 Dec 2024 14:43:53 -0800 Subject: [PATCH 5/5] Add missing continue statements. --- .../tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs | 2 ++ 1 file changed, 2 insertions(+) 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 index 4b12a26ebe72..565ecaa062fc 100644 --- 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 @@ -311,10 +311,12 @@ private List AddMissingElements(Dictionary addedAr 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)) {