Move in-vertical manifest merging to new arcade package #47881
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Depends on dotnet/arcade#15661 Removes the custom manifest handling in MergeAssetManifests, moving it to use the new manifest package produced by arcade. This reduces code duplication and means more complete manifests out of the each vertical. Add tests for the merging, based on manifests coming out of the build.
ghost
added
There was a problem hiding this comment.
Pull Request Overview
This PR migrates custom manifest merging for in-vertical builds to the new Arcade manifest package, reducing code duplication and improving manifest completeness.
- Added tests (MergeAssetManifestsTests and MockBuildEngine) to validate manifest merging.
- Updated BuildComparer to use the new Arcade package.
- Refactored MergeAssetManifests to leverage dependency injection and new manifest model APIs.
Reviewed Changes
Copilot reviewed 53 out of 68 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/SourceBuild/content/test/Microsoft.DotNet.Tests/MergeAssetManifestsTests.cs | Added new tests for merged manifest validation |
| src/SourceBuild/content/test/Microsoft.DotNet.Tests/MockBuildEngine.cs | Introduced a mock build engine for testing purposes |
| src/SourceBuild/content/eng/tools/BuildComparer/Program.cs | Updated using directive to reference the new Arcade manifest package |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/MergeAssetManifests.cs | Refactored manifest merging to use dependency injection and new Arcade APIs |
| src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/GetKnownArtifactsFromAssetManifests.cs | Changed base class inheritance to align with new task patterns |
Files not reviewed (15)
- src/SourceBuild/content/Directory.Packages.props: Language not supported
- src/SourceBuild/content/eng/Version.Details.xml: Language not supported
- src/SourceBuild/content/eng/Versions.props: Language not supported
- src/SourceBuild/content/eng/merge-asset-manifests.proj: Language not supported
- src/SourceBuild/content/eng/tools/BuildComparer/BuildComparer.csproj: Language not supported
- src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/Microsoft.DotNet.UnifiedBuild.Tasks.csproj: Language not supported
- src/SourceBuild/content/test/Microsoft.DotNet.Tests/Microsoft.DotNet.Tests.csproj: Language not supported
- src/SourceBuild/content/test/Microsoft.DotNet.Tests/assets/MergeAssetManifestsTests/manifests1/expected.xml: Language not supported
- src/SourceBuild/content/test/Microsoft.DotNet.Tests/assets/MergeAssetManifestsTests/manifests1/fsharp.xml: Language not supported
- src/SourceBuild/content/test/Microsoft.DotNet.Tests/assets/MergeAssetManifestsTests/manifests1/xdt.xml: Language not supported
- src/SourceBuild/content/test/Microsoft.DotNet.Tests/assets/MergeAssetManifestsTests/manifests2/arcade.xml: Language not supported
- src/SourceBuild/content/test/Microsoft.DotNet.Tests/assets/MergeAssetManifestsTests/manifests2/aspire.xml: Language not supported
- src/SourceBuild/content/test/Microsoft.DotNet.Tests/assets/MergeAssetManifestsTests/manifests2/aspnetcore.xml: Language not supported
- src/SourceBuild/content/test/Microsoft.DotNet.Tests/assets/MergeAssetManifestsTests/manifests2/cecil.xml: Language not supported
- src/SourceBuild/content/test/Microsoft.DotNet.Tests/assets/MergeAssetManifestsTests/manifests2/command-line-api.xml: Language not supported
Comments suppressed due to low confidence (2)
src/SourceBuild/content/test/Microsoft.DotNet.Tests/MergeAssetManifestsTests.cs:21
- [nitpick] Consider renaming 'manifestsBasePath' to 'ManifestsBasePath' for consistency with C# naming conventions.
private const string manifestsBasePath = "MergeAssetManifestsTests";
src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/MergeAssetManifests.cs:52
- Ensure that 'Log' is registered with an explicit service type if required, to avoid potential misconfigurations during dependency injection.
collection.TryAddSingleton(Log);
|
It also depends on timeline. We do need this for p4. Is anything aside from re-bootstrap blocking #47076? |
|
I believe only bootstrap, but there might be a little more work follow up work from resolving failures and the like from a full VMR run. I'm also fine getting this in first as it's a move in the right direction. I'll do a full review pass tomorrow. |
jkoritzinsky
approved these changes
Mar 28, 2025
src/SourceBuild/content/test/Microsoft.DotNet.Tests/MergeAssetManifestsTests.cs
Outdated
Show resolved
Hide resolved
| @@ -19,11 +19,10 @@ | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <Content Include="assets\**" CopyToOutputDirectory="PreserveNewest" /> | |||
| <Content Include="assets\**" CopyToOutputDirectory="Always" /> | |||
There was a problem hiding this comment.
Why can't this be PreserveNewest?
ViktorHofer
reviewed
Apr 1, 2025
| @@ -11,6 +11,8 @@ | |||
| <PackageReference Include="NuGet.Protocol" IncludeAssets="compile" /> | |||
| <PackageReference Include="NuGet.ProjectModel" IncludeAssets="compile" /> | |||
| <PackageReference Include="Newtonsoft.Json" IncludeAssets="compile" /> | |||
| <PackageReference Include="Microsoft.DotNet.Build.Manifest" /> | |||
| <PackageReference Include="Microsoft.Arcade.Common" /> | |||
There was a problem hiding this comment.
nit: indentation
Suggested change
| <PackageReference Include="Microsoft.Arcade.Common" /> | |
| <PackageReference Include="Microsoft.Arcade.Common" /> |
ViktorHofer
approved these changes
Apr 1, 2025
|
Holding off on this unless @jkoritzinsky thinks #47076 is not viable |
|
I think we should take this one as #47076 isn't ready yet. The other PR can then rebase on top of these changes. |
|
I agree with @ViktorHofer. Let's take this one first and I'll finish up mine afterwards. |
|
@jkoritzinsky @ViktorHofer Alright, dotnet/arcade#15661 needs to go in first then. |
|
Failing because command-line-api builds with IsReleaseOnlyPackageVersion=true, and the manifest merge is more strict on this front. |
This reverts commit 9f45e2f.
kasperk81
reviewed
Apr 11, 2025
Comment on lines
+4
to
+5
| <TargetFramework>$(NetCurrent)</TargetFramework> | ||
| <CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies> |
There was a problem hiding this comment.
Suggested change
| <TargetFramework>$(NetCurrent)</TargetFramework> | |
| <CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies> | |
| <TargetFramework>$(NetCurrent)</TargetFramework> | |
| <CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies> |
Comment on lines
+17
to
+21
| <!-- These are actual dependencies --> | ||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.DotNet.Build.Manifest" /> | ||
| <PackageReference Include="Microsoft.Arcade.Common" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
Suggested change
| <!-- These are actual dependencies --> | |
| <ItemGroup> | |
| <PackageReference Include="Microsoft.DotNet.Build.Manifest" /> | |
| <PackageReference Include="Microsoft.Arcade.Common" /> | |
| </ItemGroup> | |
| <!-- These are actual dependencies --> | |
| <ItemGroup> | |
| <PackageReference Include="Microsoft.DotNet.Build.Manifest" /> | |
| <PackageReference Include="Microsoft.Arcade.Common" /> | |
| </ItemGroup> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on dotnet/arcade#15661 Removes the custom manifest handling in MergeAssetManifests, moving it to use the new manifest package produced by arcade. This reduces code duplication and means more complete manifests out of the each vertical. Add tests for the merging, based on manifests coming out of the build.