Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Dec 20, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b84ab1
Don't publish assets with Vertical visibility as part of the merged m…
jkoritzinsky Dec 18, 2024
f68a748
Don't binplace for upload any "Vertical" visibility assets. They shou…
jkoritzinsky Dec 18, 2024
7227d01
PR feedback and filter out internal assets in the final join.
jkoritzinsky Dec 19, 2024
20e9987
Add logging and validation for different visibilities.
jkoritzinsky Dec 19, 2024
3123ff3
Add missing continue statements.
jkoritzinsky Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions ...ng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/GetKnownArtifactsFromAssetManifests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -110,4 +114,4 @@ public bool Equals(TaskItem? x, TaskItem? y)

public int GetHashCode([DisallowNull] TaskItem obj) => HashCode.Combine(obj.ItemSpec, obj.GetMetadata(PackageVersionAttributeName));
}
}
}
64 changes: 46 additions & 18 deletions src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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)
Expand All @@ -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));

Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert on that here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add some validation

// 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)
Expand Down Expand Up @@ -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.");
}

Expand Down
18 changes: 11 additions & 7 deletions ...eBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/MergeAssetManifests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"));
mmitche marked this conversation as resolved.
Show resolved Hide resolved
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();

Expand All @@ -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))
Expand Down
10 changes: 7 additions & 3 deletions src/SourceBuild/content/repo-projects/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. -->
Expand Down
Loading