Update Package Validation dependency and update Suppression files #60306
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0002</DiagnosticId> | ||
| <Target>M:System.Drawing.Graphics.GetContextInfo(System.Drawing.PointF@,System.Drawing.Region@)</Target> | ||
|
|
||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0001</DiagnosticId> | ||
| <Target>T:System.Drawing.FontConverter</Target> | ||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0001</DiagnosticId> | ||
| <Target>T:System.Drawing.IconConverter</Target> | ||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0001</DiagnosticId> | ||
| <Target>T:System.Drawing.ImageConverter</Target> | ||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0001</DiagnosticId> | ||
| <Target>T:System.Drawing.ImageFormatConverter</Target> | ||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0001</DiagnosticId> | ||
| <Target>T:System.Drawing.Printing.MarginsConverter</Target> | ||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0002</DiagnosticId> | ||
| <Target>F:System.Drawing.Imaging.Encoder.ColorSpace</Target> | ||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0002</DiagnosticId> | ||
| <Target>F:System.Drawing.Imaging.Encoder.ImageItems</Target> | ||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0002</DiagnosticId> | ||
| <Target>F:System.Drawing.Imaging.Encoder.SaveAsCmyk</Target> | ||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| <Suppression> | ||
| <DiagnosticId>CP0002</DiagnosticId> | ||
| <Target>F:System.Drawing.Imaging.EncoderParameterValueType.ValueTypePointer</Target> | ||
| <Left>lib/netstandard2.0/System.Drawing.Common.dll</Left> | ||
| <Right>lib/net461/System.Drawing.Common.dll</Right> | ||
| </Suppression> | ||
| </Suppressions> | ||
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.
Are all of these violations "known" issues with netstandard2.0 vs net461?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eerhardt also note that these changes are already in main for some days.
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.
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.