Update Package Validation dependency and update Suppression files #60306
Conversation
|
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue Detailscc: @eerhardt @safern @Anipik @MichaelSimons Updating Package Validation dependency along with the new generated suppression files.
|
|
Do we need to seek Tactics approval for this? |
|
I just asked the same question to @ericstj 😄. I didn't add this in the description but we need to get this in as it is currently blocking source-build. |
| @@ -1,7 +1,38 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema"> | |||
| <Suppression> | |||
| <DiagnosticId>CP0001</DiagnosticId> | |||
| <DiagnosticId>CP0002</DiagnosticId> | |||
| <Target>M:System.Configuration.SettingsAttributeDictionary.#ctor(System.Runtime.Serialization.SerializationInfo,System.Runtime.Serialization.StreamingContext)</Target> | |||
There was a problem hiding this comment.
Are all of these violations "known" issues with netstandard2.0 vs net461?
There was a problem hiding this comment.
yeah. Most of these are due to the library being a facade to some .NET Framework assembly, and then having the type in .NET Framework missing API. This particular one typeforwards to System.dll, which doesn't have the serialization constructor for this type.
There was a problem hiding this comment.
Before we weren't following type forwards, which is why we basically were blindly just suppressing all CP001 (missing types) as the net461 facade "wouldn't have the type". Now with the improved package validation, we follow the type forwards and find diffs.
There was a problem hiding this comment.
And the idea being: "netstandard2.0 and net461 are both locked and won't take any more API changes ever, so we just baseline the diffs to make the tool happy" is that about right? We don't need to verify that these are expected/known diffs, because even if they aren't, we can't fix them.
There was a problem hiding this comment.
@eerhardt also note that these changes are already in main for some days.
There was a problem hiding this comment.
The problem is that we can't go back and remove and API from ns2.0 or add it to net461, but these are definitely mistakes when we authored these packages, but we never ran API Compat in between ref vs compatible implementations, which is why they are showing up now that we actually validate the structure of the package. Which is sad that we can't do anything but suppress them, but at least shows the tool is going to bring value in the future.
|
no we dont need tactics approval for this. |
| <Left>lib/netstandard2.0/System.Data.Odbc.dll</Left> | ||
| <Right>lib/net461/System.Data.Odbc.dll</Right> | ||
| <DiagnosticId>CP0002</DiagnosticId> | ||
| <Target>M:System.Data.Odbc.OdbcParameter.get_Offset</Target> |
There was a problem hiding this comment.
This one seems interesting ....
| @@ -1,20 +1,69 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema"> | |||
| <Suppression> | |||
| <DiagnosticId>CP0002</DiagnosticId> | |||
| <Target>M:System.Drawing.Graphics.GetContextInfo(System.Drawing.PointF@)</Target> | |||
There was a problem hiding this comment.
This one we can definitely fixit as this is a new APIs that were added to the 6.0 package, IMHO I think we should just expose these new APIs on the net5.0 or net6.0 ref assembly rather than in the netstandard2.0 one.
@ericstj @JeremyKuhne thoughts?
There was a problem hiding this comment.
@ericstj @danmoseley is that a change we can still do this late in the release?
There was a problem hiding this comment.
can you explain the customer impact? I see it in the ref
There was a problem hiding this comment.
The customer impact is that we are exposing it in a netstandard2.0 ref but there is no implementation for this API in the net472 implementation, cause it is just a facade to System.Drawing.dll, which of course doesn't contain it. So customers that target ns2.0 and then run on net472 would get this API at compile time but fail with MethodNotFoundException at runtime.
There was a problem hiding this comment.
Just to add to @safern's great explanation, these problems especially affect class libraries, which usually target netstandard2.0, so if one takes a dependency on one of these APIs, and then it gest consumed by a net461+ project, it will hit the exception at runtime as Santi explained.
There was a problem hiding this comment.
Also, because they are new APIs, this is basically our last chance of fixing the issue, since after shipping, we will be effectively on the same boat as the rest of them where API surface won't be able to reduce.
There was a problem hiding this comment.
this seems worth trying to take this week, unless there's risks/complications I'm not aware of. we added an API in the wrong place, in the same release we wrote an analyzer to find such mistakes, it finds it , ...
There was a problem hiding this comment.
Sounds good. I'll work on a fix.
There was a problem hiding this comment.
Thanks. Please let me know when you have a template to look at.
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0002</DiagnosticId> | ||
| <Target>M:System.Drawing.Graphics.GetContextInfo(System.Drawing.PointF@,System.Drawing.Region@)</Target> |
|
I'll go ahead and merge this one in order to unblock source-build, and will log separate issues to investigate potential APIs we may want to investigate/fix before releasing. |
|
On second thought, it looks like I'm not authorized to merge into release branch 😆, @Anipik would you mind? |
|
I have the power! |
ghost
locked as resolved and limited conversation to collaborators
cc: @eerhardt @safern @Anipik @MichaelSimons
Updating Package Validation dependency along with the new generated suppression files.