search

dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.66k stars 1.06k forks source link

PackageValidation task should not filter package assets. #18165

Open joperezr opened 3 years ago

joperezr commented 3 years ago

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:

https://github.com/dotnet/sdk/blob/2347c7209d6974b0622b52bd6968bb6aae00fc07/src/Compatibility/Microsoft.DotNet.PackageValidation/NupkgParser.cs#L45

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[bot] commented 3 years ago

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.

joperezr commented 3 years ago

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.

ViktorHofer commented 2 years ago

https://github.com/dotnet/aspnetcore/issues/744#issuecomment-145582934 has more details around what a placeholder file means.