Pruning roll-out take 2 #14442
Pruning roll-out take 2 #14442
Conversation
| - Customers with builds that have customization may run into build failures if they are: | ||
| - Manually referencing an assembly or props/targets from a package from the global packages folder. Note that pruning is not allowed for direct dependencies, so this would need to be a transitive package. | ||
| - Manually copying assembly during a build - at runtime, the platform version will be preferred anyways, but it could lead to the build failing if the package cannot be found on disk anymore. | ||
| - We don't expect this to be a common scenario given that the packages being pruned are within the framework and customers do not need do to anything custom to make things work. |
There was a problem hiding this comment.
I agree with that sentiment. We might need to react to this in the few repositories that build the shared frameworks: aspnetcore, runtime and windowsdesktop but I don't expect any other components to be significantly impacted.
There was a problem hiding this comment.
Yeah, our repos are definitely the most complex ones from this perspective.
ViktorHofer
left a comment
ViktorHofer
left a comment
There was a problem hiding this comment.
Great summary of the current state. I read through it and still believe that the option that we initially chose "> All .NET & .NET Standard 2.0 >=" is the right one.
I agree that we should fix the entries for frameworks that don't have a targeting pack and use the meta package concept: .NETCoreApp2.x and .NETStandard2.0. The latter might not be affected.
| 1. Reduces false positives by NuGet Audit, Component Governance and other dependency auditing tools. | ||
| Our data investigations show that up 30% of transitive vulnerabilities are within packages that may be removed by pruning. | ||
| This improves the confidence in NuGetAudit and reduces unnecessary alerts by scanners that do not ship as part of the .NET SDK tooling. | ||
| 1. Improves performance, by virtue of having smaller dependency graphs and fewer packages downloaded. |
There was a problem hiding this comment.
Isn't savings in size of deployments also a motiviation?
There was a problem hiding this comment.
No, build time conflict resolution was already taking care of that.
There was a problem hiding this comment.
For the most part - as a side-effect of the pruning feature we changed/made-consistent the data used for conflict resolution which will help drop all major-version-matching packages. dotnet/sdk#44566
Frulfump
left a comment
Frulfump
left a comment
There was a problem hiding this comment.
I liked the overall pros/cons and learned a bit more about pruning while reading it as well.
| - Enabling pruning for all .NET versions + .NET Standard 2.0+ (.NET 10 Preview 1 behavior) | ||
| - Enabling pruning for .NET 8+ and .NET Standard 2.0+ (proposed .NET 10 Preview 7 behavior) | ||
| - Enabling pruning for .NET 10 only (new) | ||
| - Enabling pruning when NuGetAuditMode is all. (new) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks, added feature tangling causing confusion in the cons.
| Pros: | ||
|
|
||
| - Enables pruning for all in support frameworks. Providing benefits to projects staying current. | ||
| - Does not address false positives for every customer currently getting false positive warnings (see example: <https://github.com/dotnet/sdk/issues/49226>). |
There was a problem hiding this comment.
Hoping this will get addressed, false positives are highly annoying.
| - NuGetAuditMode is gonna be direct for a large majority of existing projects (Only 1% of restores have NuGetAuditMode=all) | ||
| We will see reduction in downloads of these packages and reduction of these packages being part of newly published packages, but we won't necessarily reduce the warnings count (overall we do want to eliminate vulnerable package usage). | ||
| The primary benefit is prunable packages not appearing in newly published packages due to automatic PrivateAssets=all. | ||
| - We are making a TFM based enablement on the runtime supported, but that logic really only applies for applications, but pruning and audit are enabled for both apps and libraries. Library authors will often target lower version of the framework because that's all they need. |
There was a problem hiding this comment.
How come the logic only applies for apps in this case?
There was a problem hiding this comment.
Apps care about the runtime they're targeting and when a runtime goes out of support, they're likely to move their project to a newer runtime since they'll get a warning.
For libraries, the tfm targeted just defines the APIs is has available, but the questions around vulnerabilities are super relevant.
See some context in dotnet/sdk#28518.
Frulfump
left a comment
Frulfump
left a comment
There was a problem hiding this comment.
I didn't mention this prior but the different stats being provided is very interesting (and also very relevant)
Co-authored-by: Frulfump <[email protected]>
Co-authored-by: Frulfump <[email protected]>
| - Potential of a breaking change for pre-existing projects. However, the benefits for these customers are more obvious, since they are likely to see fewer vulnerability warnings. They made an explicit decision to adopt NuGetAudit, and they will likely be receptive of a feature improve NuGetAudit itself. (see example: <https://github.com/dotnet/sdk/issues/49226>) | ||
| - Features get a bit tangled and enabling one feature opting you into another one could cause confusion. | ||
|
|
||
| ### >= .NET 10 enables pruning for all frameworks |
There was a problem hiding this comment.
We should have explicitly called out .NETFramework. I thought we had previously decided that pruning on .NETFramework wasn't a good idea (which is why it was excluded in both past iterations), however it looks like the current implementation enables this.
Design for #14345.