Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supplied by platform design #13634

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from
Open

Conversation

nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Jul 17, 2024

Prototype in https://github.com/NuGet/NuGet.Client/tree/dev-nkolev92-suppliedByPlatform
Related: NuGet/NuGet.Client#5919

To get dummy values from prototype, set AddDummyPrunedPackageReferences to true.

Copy link

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like the spec explicitly states whether pruned packages will be downloaded at all. It would be good to explicitly never download pruned packages. Some vulnerability scanners look at the global packages folder, so this would ensure that those scanners wouldn't have false positives for pruned packages.

accepted/2024/supplied-by-platform.md Outdated Show resolved Hide resolved
@nkolev92
Copy link
Member Author

It doesn't look like the spec explicitly states whether pruned packages will be downloaded at all. It would be good to explicitly never download pruned packages. Some vulnerability scanners look at the global packages folder, so this would ensure that those scanners wouldn't have false positives for pruned packages.

Apparently I only had that in the summary.

This helps avoid downloading unnecessary reference and implementation packages

I'll add that explicitly in the rest of the spec as well.

@nkolev92
Copy link
Member Author

Pushed some more changes to the spec based on the feedback.
There is a limitation with transitive shared frameworks. Not sure if we have an automatic solution for that.


See [Prior art](#net-sdk-package-pruning) for more details on how the .NET SDK prunes packages at runtime today.
The particular pruning is based on the resulting list of shared frameworks.
The .NET SDK will provide the list at the beginning of the restore operation, as such the .NET SDK *must* only consider direct shared frameworks, and not transitive ones.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I've only ever seen folks complain when they get transitive shared frameworks. It's often accidental and it breaks deployment -- @dsplaisted what if the SDK encouraged people to make them direct?

IOW if the result of a restore or resolving project references changed the shared framework of the project the SDK would warn the user and ask them explicitly reference that framework.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wary of changing that behavior, because we don't know how many people we aren't hearing from who are relying on it (knowingly or not).

With respect to the supplied by platform design, I think it would probably be OK to only consider the direct framework references.

Copy link

@ericstj ericstj Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we made it just a warning:

  1. When transitive framework was brought in after restore.
  2. We can see that the project has packages that overlap with that framework.
  3. Warn that those packages could be dropped by directly referencing the framework.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea.
I added it in future possibilities.

@nkolev92 nkolev92 marked this pull request as ready for review October 15, 2024 23:19
@nkolev92 nkolev92 requested a review from a team as a code owner October 15, 2024 23:19
@nkolev92 nkolev92 requested a review from ericstj October 15, 2024 23:19

### Functional explanation

A list of package id/versions to be pruned will be provided by the .NET SDK. The version will signify the highest version to be pruned.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericstj would these be specified by the individual targeting packs or the SDK as a whole?

Copy link

@ericstj ericstj Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today this information already ships in the individual targeting packs. @dsplaisted doesn't think that NuGet nor the SDK can directly consume it from here since they will not be available prior to restore, so the SDK build would need to capture them and store locally - similar to what it does with "KnownFramework" information.

However nothing should prevent other components specifying these, so long as they can specify them before restore. IOW they can come from user files and SDKs but not packages.

accepted/2024/supplied-by-platform.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants