Migrate get packages to prune#54488
Open
AlesProkop wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates GetPackagesToPrune and its targeting-pack helper path handling to MSBuild’s multi-threadable task model, resolving task path inputs through TaskEnvironment instead of process CWD.
Changes:
- Marks
GetPackagesToPruneas multi-threadable and addsTaskEnvironmentpath resolution. - Changes pruning helper methods and
FrameworkPackages.LoadFrameworkPackagesFromPackto consumeAbsolutePath. - Adds focused tests for relative prune data roots, relative targeting pack roots, and empty targeting pack root entries.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Tasks/Microsoft.NET.Build.Tasks/GetPackagesToPrune.cs |
Adds multi-threadable task support and resolves pruning/targeting-pack roots through TaskEnvironment. |
src/Tasks/Microsoft.NET.Build.Tasks/FrameworkPackages/FrameworkPackages.cs |
Updates targeting-pack loading to use AbsolutePath for file I/O while preserving original path text in logs. |
test/Microsoft.NET.Build.Tasks.Tests/GivenAGetPackagesToPrune.cs |
Adds tests covering project-directory resolution and empty targeting-pack root behavior. |
Comment on lines
+146
to
148
| PackagesToPrune = LoadPackagesToPrune(key, targetingPackRoots, prunePackageDataRoot, Log, AllowMissingPrunePackageData); | ||
|
|
||
| BuildEngine4.RegisterTaskObject(key, PackagesToPrune, RegisteredTaskObjectLifetime.Build, true); |
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
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.
Fixes dotnet/msbuild#13892
Migrate
GetPackagesToPruneto the MSBuild multithreaded task modelBrings
GetPackagesToPrune(and itsFrameworkPackages.LoadFrameworkPackagesFromPackhelper) onto the multithreaded task model so the task can safely run under MSBuild multi-threaded execution mode without depending on the process working directory.Changes
src/Tasks/Microsoft.NET.Build.Tasks/GetPackagesToPrune.cs[MSBuildMultiThreadableTask]and now implementsIMultiThreadableTaskwithTaskEnvironment = TaskEnvironment.Fallback.PrunePackageDataRootand non-emptyTargetingPackRootsare absolutized viaTaskEnvironment.GetAbsolutePath(...)before any file I/O.LoadPackagesToPrune,TryLoadPackagesToPruneForVersion,LoadPackagesToPruneFromTargetingPack,LoadPackagesToPruneFromPrunePackageData) now takeAbsolutePath/AbsolutePath[]so the safe-path contract is preserved end-to-end.TargetingPackRootsare filtered out before absolutization to keep the original skip-empty-root behavior (TaskEnvironment.GetAbsolutePath("")would otherwise throw).src/Tasks/Microsoft.NET.Build.Tasks/FrameworkPackages/FrameworkPackages.csLoadFrameworkPackagesFromPacknow acceptsAbsolutePath targetingPackRoot(wasstring). This eliminates the transitive unsafe-call diagnostics that theThreadSafeTaskAnalyzerwas reporting through this helper.AbsolutePath -> stringconversion toValue), while user-facing log messages that mention the root usetargetingPackRoot.OriginalValueso existing log text remains compatible.!string.IsNullOrEmpty(...)totargetingPackRoot.Value is not nullto match thedefault(AbsolutePath)contract.Tests
Added
test/Microsoft.NET.Build.Tasks.Tests/GivenAGetPackagesToPrune.cswith three focused functional tests (no concurrency / no attribute checks):ItResolvesPrunePackageDataRootRelativeToTaskEnvironmentProjectDirectory— verifies a relativePrunePackageDataRootis resolved againstTaskEnvironment.ProjectDirectory, not the process CWD.ItResolvesTargetingPackRootsRelativeToTaskEnvironmentProjectDirectory— verifies the same forTargetingPackRoots, exercising the targeting-pack fallback path inFrameworkPackages.LoadFrameworkPackagesFromPack.ItIgnoresEmptyTargetingPackRootEntries— locks in the empty-root behavior we explicitly preserved during migration.All three tests pass.
Validation
Microsoft.NET.Build.Tasksbuilds clean.Microsoft.NET.Build.Tasks.Testsbuilds clean; new tests pass (3 Passed, 0 Failed).ThreadSafeTaskAnalyzer(fromdotnet/msbuild) no longer reports transitive unsafe-API diagnostics onGetPackagesToPruneafter the helper signature change.