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

Include only PackageReferences included at runtime in deps.json #46218

Open
wants to merge 2 commits into
base: release/9.0.3xx
Choose a base branch
from

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Jan 22, 2025

Fixes dotnet/roslyn#76797

PrivateAssets isn't what we should be looking for to determine that we don't need an assembly in the deps.json file. This instead looks for ExcludeAssets and IncludeAssets to see if it will be in the output directory and includes it in the deps.json if so.

Copy link
Contributor

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@ericstj
Copy link
Member

ericstj commented Jan 23, 2025

I think this is the right change and better than the PrivateAssets approach. I do want to mark this as "breaking" though because it's possible for folks to notice. I would expect that in all the cases where folks notice it's really a no-op since I would expect all cases the SDK was just writing an empty entry.

The sort of things that folks might see - an entry goes away for a particular package, there's a package reference listed to a package but no definition. For the latter perhaps the SDK can erase that reference too.

One thing that would be great to include with this is to also remove the entries that were dropped by conflict resolution, which is effectively another "automated" way that the SDK applies the equivalent of ExcludeAssets.

@Forgind
Copy link
Member Author

Forgind commented Jan 24, 2025

One thing that would be great to include with this is to also remove the entries that were dropped by conflict resolution, which is effectively another "automated" way that the SDK applies the equivalent of ExcludeAssets.

Is the _ConflictPackageFiles item (filtered to just References) right? That seems to be created by the ResolvePackageFileConflicts task, and it only includes the 'losers.' I haven't found anything else that looks good yet.

@dsplaisted
Copy link
Member

I think it might be better if instead of, or in addition to this, we did the following: Remove any Libraries from the deps.json that have no assets under them. From the example in #39400:

image

NerdBank.GitVersioning is listed as a library under the .NET 7 target, but has no runtime assets or anything else under it. So maybe we could remove it regardless of whether ExcludeAssets was specified, since whatever the cause it doesn't contribute anything at runtime.

This would require diving into the DependencyContextBuilder code, understanding how it works, and updating it. We also might need to consider cases such as if a library has no assets but does list dependencies... do we need to check if it transitively brought in any assets?

Thoughts?

@baronfel
Copy link
Member

One thing I like about your addition @dsplaisted is that it makes the deps.json more purpose-built - the file only cares about libraries and assets that impact the runtime experience.

For users that need accurate recounting of compile-time library dependencies we should be promoting SBOMs, which are a separate artifact specifically for book-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure breaking-change Using this label will notify dotnet/compat and trigger a request to file a compat bug needs-breaking-change-doc-created untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants