Include only PackageReferences included at runtime in deps.json

[Forgind]

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.

[dotnet-policy-service]

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]

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]

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]

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:

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]

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.

[dsplaisted]

@ericstj @ViktorHofer @agocke Any feedback on my idea above on removing libraries from the deps.json file if they don't have any assets?

What are the different asset types possible, and how should we handle them and handle dependencies between libraries that don't have any assets?

[ericstj]

@ericstj @ViktorHofer @agocke Any feedback on my idea above on removing libraries from the deps.json file if they don't have any assets?

Yes! this would be great. We've wanted these gone for some time as they also cause false positives for folks scanning the files.

What are the different asset types possible

The runtime cares about runtime, native, runtimeTargets, and resources @elinor-fung @jkoritzinsky

how should we handle them and handle dependencies between libraries that don't have any assets?

I think we just remove the library section and remove references to the removed section. This should ensure that that an entire branch from the package graph is removed. I can imagine if we're sloppy about identifying packages to remove it could lead to "orphaned" references - though I don't think the host would care about that since it doesn't attempt to construct a graph.

[dsplaisted]

@ericstj How should we handle dependencies if we're removing packages without assets? Should we remove a package that has no assets but depends on a package which does? I think that could lead to a different kind of orphaning where we have a dependency of the removed package still in the file but no dependencies to it any more.

[ericstj]

Should we remove a package that has no assets but depends on a package which does?

I'd correct this to say

package that has no assets but is the only package which depends on another package that has assets

This is not so uncommon if you consider a case where someone does ExcludeAssets on one package with a large package graph, but then updates one of that package's dependencies to a newer version with a direct reference (or some other graph) that's not excluded.

If you could constrain the removal in this way efficiently then I think it would be safer, however my hypothesis is that this is unlikely if you are identifying removal based on exclusion/conflict resolution. Package exclusion is specified by graph notation which flows to dependencies and conflict resolution is closure complete. So you could count on these mechanisms being graph complete for a given path.

I can see how if instead we remove packages that have no assets (regardless of how) then it would be safer to also check that they are not the only packages that reference (including indirectly) some other package with assets.