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

PackageValidation task should not filter package assets. #18165

Open
joperezr opened this issue Jun 9, 2021 · 3 comments
Open

PackageValidation task should not filter package assets. #18165

joperezr opened this issue Jun 9, 2021 · 3 comments
Assignees
Milestone

Comments

@joperezr
Copy link
Member

joperezr commented Jun 9, 2021

cc: @Anipik @safern @ericstj

While testing package validation I noticed that apparently we are filtering the assets that we consider when parsing a package here:

return new Package(packageId, version, packageReader.GetFiles()?.Where(t => t.EndsWith(packageId + ".dll")), packageDependencies, runtimeGraph);

The problem with this, is that we will only consider those filtered files when calculating the best compile and runtime asset to verify APICompat for, but that is not always accurate, as a package could have an asset like lib/net461/_._ which should win as a runtime asset over a lib/netstandard2.0/Foo.dll when evaluating for net461, and that is not happening today. This is important as the previously mentioned example, means that effectively I would be intentionally dropping support for net461, but package validation won't catch it as it would evaluate APICompat against the netstandard2.0 asset.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Jun 9, 2021
@joperezr
Copy link
Member Author

joperezr commented Jun 9, 2021

We should remove that filtering, and then refactor the code so that if the ref or runtime asset picked for a specific framework is not a .dll (like in the case of a _._ getting picked) then we should not try to read into the assembly and instead just provide the appropriate validation warning saying that the support was dropped. It would also be good to test against packages that produce .winmds instead of .dlls, as I'm not so sure that our current logic would work with them.

@Anipik Anipik added Area-ApiCompat and removed Area-NetSDK untriaged Request triage from a team member labels Jun 9, 2021
@Anipik Anipik modified the milestones: 6.0.1xx, 6.0.0 Jun 9, 2021
@marcpopMSFT marcpopMSFT modified the milestones: 6.0.0, 6.0.1xx Jun 10, 2021
@ericstj ericstj modified the milestones: 6.0.1xx, 7.0.1xx Jan 26, 2022
@ViktorHofer ViktorHofer self-assigned this Aug 18, 2022
@ViktorHofer
Copy link
Member

dotnet/aspnetcore#744 (comment) has more details around what a placeholder file means.

ViktorHofer added a commit to ViktorHofer/sdk that referenced this issue Sep 12, 2022
Fixes dotnet#18165

Instead of manually filtering package assets, let NuGet calculate the
runtime and compile time assets and handle placeholder files which are
returned by NuGet.
ViktorHofer added a commit to ViktorHofer/sdk that referenced this issue Sep 13, 2022
Fixes dotnet#18165

Instead of manually filtering package assets, let NuGet calculate the
runtime and compile time assets and handle placeholder files which are
returned by NuGet.
@smasher164 smasher164 modified the milestones: 7.0.1xx, 7.0.2xx Oct 19, 2022
@marcpopMSFT marcpopMSFT modified the milestones: 7.0.2xx, 8.0.1xx Jan 25, 2023
@ViktorHofer ViktorHofer modified the milestones: 8.0.1xx, 8.0.2xx Jul 20, 2023
@marcpopMSFT marcpopMSFT modified the milestones: 8.0.2xx, 8.0.4xx Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants