Update CompatibilitySuppressions after updating APICompat that uses references #58208
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsUpdating the Compatibility package used in dotnet/runtime repo and updating the CompatibilitySuppresion.xml files now that ApiCompat is able to follow type-forwards. This now allow us to remove some suppressions that were just due to not finding the types to the forwarded assembly, as well as add new suppressions for breaking changes to types that are forwarded.
|
| <DiagnosticId>CP1002</DiagnosticId> | ||
| <Target>System.ServiceModel.Internals.dll</Target> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP1002</DiagnosticId> | ||
| <Target>SMDiagnostics.dll</Target> |
There was a problem hiding this comment.
These appear in multiple suppression files now. What is that?
There was a problem hiding this comment.
Right, I chatted briefly with @ericstj and @Anipik about these. Basically some APICompat rules now might need a full closure of references when loading the assemblies, and for some projects that target net461, these two assemblies represent part of the full closure and are part of the GAC, just are not part of the targeting pack for whatever reason. The way ApiCompat works today, is that if you elected to load references and APICompat is unable to get the full closure, it emits a warning (CP1002) for each assembly in the closure that it couldn't find. We could opt for special-casing these two when targeting net461, but I think a better solution is for APICompat to be smarter and only emit CP1002 warnings when the assembly that it couldn't find was actually required by some rule that was doing some dependency walking. I have an idea on how to do this, and plan to add it for RC2, so my suggestion for now would be to check in these suppressions as-is, and once we fix APICompat to not warn about these then come back and remove these suppressions.
There was a problem hiding this comment.
Thanks for the explanation Joe.
just are not part of the targeting pack for whatever reason
Presumably because only assemblies that are meant to be directly referenced are part of the targeting pack, which System.ServiceModel.Internals.dll isn't. Just my guess.
I think a better solution is for APICompat to be smarter and only emit CP1002 warnings when the assembly that it couldn't find was actually required by some rule that was doing some dependency walking. I have an idea on how to do this, and plan to add it for RC2, so my suggestion for now would be to check in these suppressions as-is, and once we fix APICompat to not warn about these then come back and remove these suppressions.
Fully agree on that CP1002 should only be raised for references that are actually required to satisfy the graph. Presumably when you re-generated the suppression files with the newer version of PackageValidation, you already got a ton of value as you could test your change on dotnet/runtime.
Based on the diff and the amount of suppressions being added in comparison to what's being deleted, what do we gain by merging this in now instead of waiting until your mentioned change is merged and consumable?
There was a problem hiding this comment.
Based on the diff and the amount of suppressions being added in comparison to what's being deleted, what do we gain by merging this in now instead of waiting until your mentioned change is merged and consumable?
By enabling this, we first get coverage of a current blind spot (testing facades) and we also get us closer to our goal of getting all APICompat warnings with a very small cost (which is just temporary) which is these CP1002 errors which may or may not be relevant. We already saw in other places (like your AccessControl PR) that PackageValidation has helped catch interesting issues on the build, so I'd rather check this in and have some progress made as opposed to waiting for the full solution and miss a) feedback on this feature since we are shipping it in 6.0, b) coverage for full facades, c) coverage of other potential APICompat issues with our build.
There was a problem hiding this comment.
That sounds like the right set of reasons to get this in 👍
joperezr
force-pushed
the
UpdateApiCompat
branch
from
8d991a5
to
bb67c02
Compare
|
Hello @joperezr! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Errors in CI are unrelated. Merging to unblock upcoming maestro consumption PRs |
src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/CompatibilitySuppressions.xml
Show resolved
Hide resolved
ghost
locked as resolved and limited conversation to collaborators
Updating the Compatibility package used in dotnet/runtime repo and updating the CompatibilitySuppresion.xml files now that ApiCompat is able to follow type-forwards. This now allow us to remove some suppressions that were just due to not finding the types to the forwarded assembly, as well as add new suppressions for breaking changes to types that are forwarded.