Remove primary vertical concept from JoinVerticals #47338
Conversation
There was a problem hiding this comment.
PR Overview
This pull request removes the concept of a primary vertical from the JoinVerticals task and updates the YAML pipelines to use a RID-specific publish model, thereby streamlining artifact handling. Key changes include:
- Updated YAML pipeline templates to pass vertical artifact information uniformly.
- Removal of primary vertical handling and artifact download logic in JoinVerticals, replacing them with static asset copy functions.
- Cleanup of obsolete code and configuration files (JoinVerticalsConfig and AzureDevOpsClient) that are no longer required.
Reviewed Changes
| File | Description |
|---|---|
| eng/pipelines/templates/stages/vmr-build-with-join.yml | Updated pipeline parameters and stages to support uniform vertical artifact handling. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/ManifestAssets/BuildAssetsManifest.cs | Adjusted attribute getter methods to perform case-insensitive comparisons. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs | Removed primary vertical logic and replaced artifact download tasks with static asset copying. |
| eng/pipelines/templates/steps/vmr-join-verticals.yml | Modified step inputs and added a step for log file copying post-join. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/ManifestAssets/JoinVerticalsAssetSelector.cs | Removed the deprecated "PriorityVerticals" match type and related configuration. |
| eng/pipelines/templates/variables/vmr-build.yml | Changed a variable value to disable ibcEnabled. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/Models/AzureDevOpsArtifactInformation.cs | Removed the unused artifact model. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/ManifestAssets/JoinVerticalsConfig.cs | Removed obsolete join vertical configuration. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/AzureDevOpsClient.cs | Removed the Azure DevOps client as it is no longer needed. |
Copilot reviewed 59 out of 59 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs:120
- The initial File.Copy appears redundant since a subsequent conditional File.Copy is performed based on the asset type; consider removing the redundant copy to avoid unnecessary I/O and potential file conflicts.
File.Copy(sourceFile, destinationFilePath, true);
There was a problem hiding this comment.
PR Overview
This pull request removes the concept of a primary vertical from JoinVerticals and updates associated pipeline YAML templates to support a unified artifact download process. Key changes include:
- Removal of MainVertical-related properties and download methods in JoinVerticals.cs.
- Update of YAML pipeline templates to leverage the new vertical artifacts model.
- Removal of unused files (AzureDevOpsArtifactInformation.cs, JoinVerticalsConfig.cs, and AzureDevOpsClient.cs).
Reviewed Changes
| File | Description |
|---|---|
| eng/pipelines/templates/stages/vmr-build-with-join.yml | Updates to parameters and stages to support joining verticals without a primary vertical. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs | Removal of primary vertical code and refactoring of asset copying logic to use the new VerticalArtifactsBaseFolder. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/ManifestAssets/BuildAssetsManifest.cs | Updates to property accessors for improved bool comparisons. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/ManifestAssets/JoinVerticalsAssetSelector.cs | Removal of priority vertical matching and simplified asset selection. |
| eng/pipelines/templates/steps/vmr-join-verticals.yml | Updated parameters and steps to align with the new asset download approach. |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/Models/AzureDevOpsArtifactInformation.cs, JoinVerticalsConfig.cs, AzureDevOpsClient.cs | Removal of unused files that relied on the primary vertical concept. |
Copilot reviewed 58 out of 58 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs:121
- In CopyVerticalAssets, 'sourceFile' is a tuple containing both the match result and the fileName. Use the fileName property (e.g. Path.Combine(sourceDirectory, sourceFile.fileName)) as the source path when copying files instead of passing the tuple directly.
File.Copy(sourceFile, destinationFilePath, true);
| - This is not the Rid-agnostic vertical | ||
| - This is a BuildPass1 build | ||
| --> | ||
| <EnableDefaultRidSpecificArtifacts>false</EnableDefaultRidSpecificArtifacts> |
There was a problem hiding this comment.
Do we need to flow this property when it's false? Asking as reducing the number of properties that we flow to the inner build is preferred.
There was a problem hiding this comment.
We need to flow it for runtime, which defaults this to true. Once we get rid of the runtime official build, we can change runtime's infra to not default to true.
There was a problem hiding this comment.
OK. Consider moving this to runtime.proj and add a tracking issue to remove it.
There was a problem hiding this comment.
I understand why you introduced this file and what you are trying to solve with it but I can't say that the additional indirection makes things easier to follow / understand.
There was a problem hiding this comment.
Yeah it's not as clear as I'd like it to be. I'll try to add more comments about what it's doing and why to make it a little clearer.
There was a problem hiding this comment.
I'm not sure how to best review this file. Is this a 1:1 move or did something change in it?
There was a problem hiding this comment.
This is a 1:1 move of the jobs in the Build stage.
…rtifactVisibility. Remove unused yaml logic and add more comments
Fixes dotnet/source-build#4905
Uses the "RID-specific publish" model introduced in dotnet/arcade#15549 to give the VMR orchestrator control over when all assets are published with External visibility and when RID-agnostic assets are Vertical only.
Also updates JoinVerticals to stop doing its own download and instead statically download all artifacts as there are now no duplicates of external artifacts.
To avoid duplication in the YAML, I've updated the templates to enable JoinVerticals to automatically be given all of the different vertical's artifacts (using the same technique as in dotnet/runtime#111934).
I've removed the concept of a primary vertical as it's now unused.
One interesting snag I hit: As the Windows_x86_BuildPass2 leg needs an Arcade build to use, I had to make Arcade publish with "internal" visibility in all BuildPass1 legs to enable Arcade to be present in any BuildPass2 leg no matter what BuildPass1 legs it depends on. Alternatively, we could have Arcade always build in BuildPass2 as well and remove this.
That would also remove our only "Internal" visibility asset, allow us to truly have zero duplicate assets, and allow us to use a 1ES PT Artifact Job to download/upload assets and do the join in a separate job.
Patches into other repos:
dotnet/aspnetcore#60792
dotnet/windowsdesktop#4960
NuGet/NuGet.Client#6306