Generate pruning data correctly #50348
Conversation
nkolev92
changed the title
|
cc @dsplaisted |
There was a problem hiding this comment.
Pull Request Overview
This PR modifies how pruning data is generated for multi-targeted projects to ensure compatibility with .NET 10 requirements. The change allows pruning data generation when RestoreEnablePackagePruning is empty (indicating a multi-targeted project) in addition to when it's explicitly set to true.
- Modified the target condition to generate pruning data for multi-targeted projects even when
RestoreEnablePackagePruningis not explicitly set - Updated test cases to verify the new behavior with additional test scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets | Updated condition logic to generate pruning data for multi-targeted projects |
| test/Microsoft.NET.Build.Tests/GivenThatWeWantToResolveConflicts.cs | Added test parameters to verify pruning behavior with different configurations |
...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets
Outdated
Show resolved
Hide resolved
|
That's a good idea. |
|
@nkolev92 Were you targeting .NET 10 RC1 for this? If so, it will need to be ported to the RC1 release branch, and you probably need Tactics approval. |
|
Yes, it needs ported to RC1. |
nkolev92
force-pushed
the
dev-nkolev92-generatePruningDataCorrect
branch
from
bec8595 to
97d1fb3
Compare
nkolev92
force-pushed
the
dev-nkolev92-generatePruningDataCorrect
branch
from
97d1fb3 to
849eae3
Compare
|
/backport to release/10.0.1xx |
|
Started backporting to release/10.0.1xx: https://github.com/dotnet/sdk/actions/runs/17083993349 |
|
@dsplaisted an error occurred while backporting to "release/10.0.1xx", please check the run log for details! The process '/usr/bin/git' failed with exit code 1 |
|
I see conversations here saying this needed to be ported to RC1, but as far as I can tell, this change did not actually make it into the 10.0.100-rc.1.25451.107 SDK that released yesterday. With this change not making it in, it seems pruning is broken for multi-targeted projects. When I manually add this change to my copy of |
|
I think this did make it into RC1 but then it was disabled or backed out due to a perf regression. @nkolev92 can probably confirm. |
|
It was ported yeah: #50377. Perf regression is correct, but it was only disabled in dotnet/dotnet repo: dotnet/dotnet@2a94fd4. That change hasn't flown into the RC1 branch in this repo. Not sure if it will. I'm not 100% versed into the VMR world :D https://github.com/dotnet/dotnet/blob/release/10.0.1xx/src/sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets#L58 seems to have the latest changes. https://github.com/dotnet/dotnet/blob/release/10.0.1xx-rc2-branding-update/src/sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets has old changes, but I'm not sure if that's a shipping branch. cc @mmitche |
Not a shipping branch |
|
So, why did the change not end up the RC1 build that shipped? I've checked again by installing Because of that, when I have a multi-targeted project, pruning is not working unless I manually enable it.
Is there a plan to re-introduce this in some way before RTM? Is there an issue I can follow? |
The change was only made in the RC1 branch. |
|
Ok, so it was reverted on the RC1 branch before it shipped, but will be back in RC2? Just curious, is there another change being made to avoid it continuing to cause the perf problem that caused the revert in the first place? |
|
yes, in a different repo: NuGet/NuGet.Client#6753 |
I was hit by the changes to pruning between preview 7 and RC1 in pretty much every way possible. I had all of the following scenarios that broke:
I ended up spending a lot of time trying to figure out what had happened in RC1 to cause all the broken builds. It would have been nice if the changes had been called out in any of the release notes documents. Instead, I spent time comparing targets files, and tracking down issues and PRs that might have been related. That's what led me to this PR and my question about why the changes weren't actually in RC1. 😄 |
|
@bording In particular, we expect that enabling pruning on/off should not lead to a build break, save for some very special cases. |
To provide more context, we do set For the first two scenarios I listed, pruning being disabled meant that we were no longer getting pruning of transitive packages that were flagged as vulnerable. We had gone through those repos already as of preview 7 and removed unnecessary direct package references now that pruning was removing them for us. For the repos that are covered by those scenarios, we had already modified them to build properly when pruning was on, so multi-targeting causing pruning to be disabled in RC1 was a problem for us. The .NET Framework scenario was also related to vulnerable packages showing back up, but when I tried to fix it by turning pruning back on, I actually hit a runtime error on the I know you're already planning on disabling pruning for .NET Framework, and that's also how I solved that problem. One other thing I've observed while investigating all of this, the NU1903 warnings that are getting reported when building from the commandline aren't quite correct for multi-targeted scenarios. For example, in a |
|
Thanks for the explanation, that was super helpful. Sorry for all the changes you've had to do, but based on your experiences, this gives me more confidence we have the right solution for the GA version. The NU1903 issue is not known, so there's probably a bug somewhere. |
|
Customer Impact
This PR is the SDK side of changing the package pruning defaults to be enabled for .NET 10 projects only, including ones that multi-target .NET 10.
Design details at https://github.com/NuGet/Home/blob/dev/accepted/2025/prune-package-reference-rollout-take-2.md.
Without this change multi-targeted projects will not get data.
SDK side of NuGet/NuGet.Client#6696.
Due to the fact that we want pruning (inner build feature) to be enabled for the full project when one of the TFMs is .NET 10, in multitargeted projects we should always generate the pruning data and let NuGet decide whether to use it.
Regression
No, NET 10 is the first time we're enabling the feature by default and this behavior is more restrictive than the P1-P7 behaviors.
Testing
Automation added in this PR.
The equivalent functionality is already tested on NuGet side as well.
Risk
Low, it enables pruning for more scenarios such as .NET 10 multi-targeting, but the P7 default itself is more permissive (it enables for net8.0 and above).