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

Address obsolete ContentItemGroup.FindItems in PackageValidation #23301

Closed
ericstj opened this issue Jan 6, 2022 · 4 comments
Closed

Address obsolete ContentItemGroup.FindItems in PackageValidation #23301

ericstj opened this issue Jan 6, 2022 · 4 comments

Comments

@ericstj
Copy link
Member

ericstj commented Jan 6, 2022

We use ContentItemCollection.FindItemGroups in package validation to replicate NuGet's asset selection algorithm:

PackageAssets = _packageAssets.FindItems(_conventions.Patterns.AnyTargettedFile);
RefAssets = _packageAssets.FindItems(_conventions.Patterns.CompileRefAssemblies);
LibAssets = _packageAssets.FindItems(_conventions.Patterns.CompileLibAssemblies);
CompileAssets = RefAssets.Any() ? RefAssets : LibAssets;
RuntimeSpecificAssets = _packageAssets.FindItems(_conventions.Patterns.RuntimeAssemblies).Where(t => t.Path.StartsWith("runtimes"));
RuntimeAssets = _packageAssets.FindItems(_conventions.Patterns.RuntimeAssemblies);

As of NuGet/NuGet.Client@bcd6856 this method is obsolete. The newly introduced method PopulateItemGroups does too much (allocating groups that we'd have to flatten).

For now I've disabled the obsoletion warning. We should decide what to do moving forward, either have NuGet add an API or change how we do asset selection in the package. It may be convenient to solve this while addressing #17364

Originally found here: #23277 (comment)

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-ApiCompat untriaged Request triage from a team member labels Jan 6, 2022
@ericstj
Copy link
Member Author

ericstj commented Jan 6, 2022

cc @jebriede @safern

@jebriede
Copy link

jebriede commented Jan 7, 2022

@ericstj thanks for tagging me on this issue. FindItems was cleaned up and marked as obsolete as part of a memory overallocation fix but looking at this more closely, I believe FindItems should remain and not be obsolete.

PopulateItemGroups replaces FindItemGroups which uses yield returns, and on a hot code path, resulted in the overallocation of enumerator objects. FindItems used to use yield returns as well but I fixed its implementation to no longer use yield returns so it does not need to be obsolete.

I've created a bug NuGet/Home#11487 and submitted a fix to remove the obsolete attribute from FindItems in this PR: NuGet/NuGet.Client#4394. Thanks!

@ericstj
Copy link
Member Author

ericstj commented Jan 7, 2022

Excellent! Thank you @jebriede! We will remove the obsolete suppression once that change is in.

@ViktorHofer
Copy link
Member

Fixed with dbefb9f by removing the pragma as the underlying API isn't obsolete anymore.

akoeplinger added a commit that referenced this issue Aug 19, 2024
…ion" (#42648)

Reverts #42508

It turns out that this was discussed a while ago when the `FindItems` API was made obsolete the first time and we actually concluded that it _should not_ be obsolete: #23301
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants