-
Notifications
You must be signed in to change notification settings - Fork 5k
Update previous netcoreapp version to run API Compat against 6.0 ref pack #61437
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
Conversation
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsWe are still running API Compat for the ref pack against the 5.0 ref pack rather than the 6.0 one on main. Now that the 6.0 is released we can do that.
|
It seems like we are hitting some package validation now that the baseline versions changed. I will look into them and see why. |
fa0944c
to
1354b39
Compare
<Right>lib/netstandard2.0/System.Data.Odbc.dll</Right> | ||
<IsBaselineSuppression>true</IsBaselineSuppression> | ||
</Suppression> | ||
<Suppression> | ||
<DiagnosticId>CP0002</DiagnosticId> | ||
<Target>M:System.Data.Odbc.OdbcParameter.get_Offset</Target> | ||
<Left>runtimes/freebsd/lib/netcoreapp2.0/System.Data.Odbc.dll</Left> | ||
<Left>runtimes/freebsd/lib/net6.0/System.Data.Odbc.dll</Left> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to check these runtime net6.0 assets vs lib assets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I was able to figure these out. The package doesn't have a ref
folder, and this is comparing runtimes/*
as the contract, VS the ridless lib
(the compile asset), and it seems like the ref is not exposing these APIs. It seems like that is by design. As we even have this: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Data.Odbc/src/MatchingRefApiCompatBaseline.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple look like real failures due to TFMs being removed.
Do we have a way to suppress the removed TFMs and avoid any member-specific noise about them, while still getting coverage for newer TFMs?
src/libraries/System.ServiceProcess.ServiceController/src/CompatibilitySuppressions.xml
Show resolved
Hide resolved
Not yet, this is what I was mentioning here: #61437 (comment) We need a way to specify tmfs to ignore via an item or something like that. |
@ericstj I've replied to your feedback, could you take another look to get this unblocked? |
src/libraries/System.Security.Cryptography.Pkcs/src/CompatibilitySuppressions.xml
Show resolved
Hide resolved
src/libraries/System.Windows.Extensions/src/CompatibilitySuppressions.xml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good in general. It would be good to investigate those new ODBC ones as I wouldn't expect those either. Also, a NIT, but it might be worth adding a comment on all of the suppressions that are expected due to a drop of a TFM from the current package, that is what we did before 6.0 shipped so we should probably do that as well in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So long as we're certain those suppressions aren't real issues that will be hit for supported frameworks I'm OK with this.
1354b39
to
099c709
Compare
I've validated the diffs and they all make sense. |
We are still running API Compat for the ref pack against the 5.0 ref pack rather than the 6.0 one on main. Now that the 6.0 is released we can do that.