Provide PrunePackageReference data for NuGet #45874
Conversation
…k and framework references
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets
Outdated
Show resolved
Hide resolved
...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This doesn't have how the PrunePackageReference item (maybe tweak the name? sounds like a boolean value to me) is used. It looks like it should hold the highest value of each of the framework references, so if that's what nuget wants, then this looks right to me.
| @@ -50,6 +51,21 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| </CreateWindowsSdkKnownFrameworkReferences> | |||
| </Target> | |||
|
|
|||
| <PropertyGroup Condition="'$(PrunePlatformPackageReferences)' == ''"> | |||
| <PrunePlatformPackageReferences>false</PrunePlatformPackageReferences> | |||
There was a problem hiding this comment.
Why do you need this PropertyGroup?
There was a problem hiding this comment.
It was to set a default value, and even if it wasn't technically needed it made it easier to change the default value. But now I'm using NuGet's flag instead so this has been removed.
| <PrunePlatformPackageReferences>false</PrunePlatformPackageReferences> | ||
| </PropertyGroup> | ||
|
|
||
| <Target Name="AddPrunePackageReferences" BeforeTargets="CollectPrunePackageReferences" |
There was a problem hiding this comment.
Nit:
_AddPrunePackageReferences
?
There was a problem hiding this comment.
We have so many targets that don't start with the underscore, I'm not sure there's much value in adding it here.
| @@ -0,0 +1,197 @@ | |||
| namespace Microsoft.ComponentDetection.Detectors.NuGet; | |||
There was a problem hiding this comment.
Do we need these when they're all out-of-support?
There was a problem hiding this comment.
It doesn't hurt, it may help a bit, and it's easier to just copy all of the code from the folder in the component-detection repo rather than to pick and choose.
There was a problem hiding this comment.
Yes it helps for out of support frameworks. Less noise that folks need to deal with -- helps them see the key message
-- framework is out of support.
| using System.Threading.Tasks; | ||
| using Microsoft.Build.Framework; | ||
| using Microsoft.Build.Utilities; | ||
| using Microsoft.ComponentDetection.Detectors.NuGet; |
There was a problem hiding this comment.
Will Microsoft.ComponentDetection.Detectors.NuGet trip up RPS module loads?
There was a problem hiding this comment.
No, this isn't adding another DLL reference, there's just code that I've copied in from elsewhere that has a different namespace.
src/Tasks/Microsoft.NET.Build.Tasks/FrameworkPackages/FrameworkPackages.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| PackagesToPrune = packagesToPrune.Select(p => |
There was a problem hiding this comment.
I wonder if we'll want a cache of these per framework? In case there is cost in constructing the items and value in sharing them for a given framework references & version.
There was a problem hiding this comment.
I've added a cache in the latest commit, though I'm not sure it's really worth the risk right now, since this is an opt in feature and the tests won't really exercise the caching, so it's pretty much untested.
src/Tasks/Microsoft.NET.Build.Tasks/FrameworkPackages/FrameworkPackages.cs
Show resolved
Hide resolved
It's consumed by NuGet / the NuGet targets, so that's not part of this PR (or this repo, really). The spec covers how it works. The item name also needs to be the same as NuGet is using, so we can't just change that. |
test/Microsoft.NET.Build.Tests/GivenThatWeWantToResolveConflicts.cs
Outdated
Show resolved
Hide resolved
|
Testing note: I built the SDK repo with this change and package pruning enabled. Spot-diffing some assets files showed packages were pruned correctly. Perf-wise, the AddPrunePackageReferences target took 600 ms out of a1 minute 42 second build. That seems fine, and the overall build time might even be less since fewer packages are downloaded, passed to the compiler, etc. |
…design-time builds, and add tests
|
@dsplaisted do you plan to dogfood this in our own stack, i.e. by turning in on in the Arcade SDK? Source-build would immediately benefit from it as it still needs to carry netstandard1.x packages around (in SBRP). cc @MichaelSimons |
Yes, we should do that. We need to make sure that the SDK used by our repos is updated to a version that supports this too. |
|
We will probably update the SDK in Arcade and the VMR sometime next week. I can ping you when that's done. |
|
@dsplaisted I'm guessing #44694 should now be closed? |
Provide PrunePackageReference data to NuGet (see the spec).
This implementation copies generated code from https://github.com/microsoft/component-detection/tree/main/src/Microsoft.ComponentDetection.Detectors/nuget/FrameworkPackages which encapsulates the data about which packages are included for each target framework and framework reference. This is a quick and easy way to get the PrunePackageReference support working, though it doesn't provide the best way for keeping this data up-to-date.
We will continue to work on designing a mechanism for this information to flow into the SDK and be updated with each release.