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.
…rtifactVisibility. Remove unused yaml logic and add more comments
src/SourceBuild/nuget-client/0001-support-rid-agnostic-publishing.patch
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
/azp run sdk-unified-build-full |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| sbomEnabled: false | ||
| condition: succeededOrFailed() | ||
| steps: | ||
| - ${{ if ne(variables['System.TeamProject'], 'internal') }}: |
There was a problem hiding this comment.
In internal, we use the 1ES PT inputs above.
I'd love to use those everywhere, but 1ES hasn't responded to my feature request for an "OSS 1ES PT with only the syntax niceties and no internal product validation"
There was a problem hiding this comment.
We could consider adding some basic inputs/outputs support to an Arcade template so we could at least unify the many places Arcade and other repos have to write the different styles.
...tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/ManifestAssets/JoinVerticalsAssetSelector.cs
Outdated
Show resolved
Hide resolved
src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs
Outdated
Show resolved
Hide resolved
|
/azp run sdk-unified-build-full |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Patches into other repos:
dotnet/aspnetcore#60792
dotnet/windowsdesktop#4960
NuGet/NuGet.Client#6306