Make System.Drawing.Common throw on Unix unless a config switch is set #55962
Conversation
safern
requested review from
stephentoub,
terrajobst,
danmoseley,
jkotas and
ericstj
|
Tagging subscribers to this area: @safern, @tarekgh Issue DetailsI choose the GDIP startup process to throw the PNSE as that is executed via an static constructor and from the scan I did there is not any interesting types left in System.Drawing.Common that don't use GDI+. Most of the ones that don't use it are data holders to represent data gathered from GDI+, or to pass in to another class as a parameter and then go down to the native layer. But doing it from the GDIP startup process it means that some of these types that don't use or reference GDI+ directly can be used/created (they are not interesting to use), but those would not throw PNSE. Are we OK with that and then in .NET 7+ make every type on the assembly just throw or do we want to follow a different approach? Relates to: dotnet/designs#234
|
Co-authored-by: Immo Landwerth <immo@landwerth.net>
src/libraries/System.Drawing.Common/src/System/Drawing/GdiplusNative.Unix.cs
Outdated
Show resolved
Hide resolved
Yes, I think so - same as what we do for other OS-specific libraries. |
|
I think this should be marked as breaking change and have breaking change template filled. |
safern
added
the
breaking-change
ghost
added
the
needs-breaking-change-doc-created
|
Tagging @dotnet/compat for awareness of the breaking change. |
src/libraries/System.Drawing.Common/src/System/Drawing/GdiplusNative.Unix.cs
Outdated
Show resolved
Hide resolved
|
@ericstj some resources tests are failing on non-windows: Do we care about that support? Or should I just disable those tests cases on non-windows? |
Those tests are using Drawing. We can have them set the flag as well https://github.com/dotnet/runtime/pull/55962/files#diff-8409367c188e7be18ecf8735b3d8016c9ca5f9c2ec610eb2ec122bef2a4d94feR3. If we want we can also add a test to validate the failing scenario as you did here. |
|
Thanks. I didn't know if that scenario was important for Resources on Unix. I'll update the tests accordingly. |
|
I've created the breaking change issue: dotnet/docs#25257 |
| { | ||
| RemoteExecutor.Invoke(() => | ||
| { | ||
| AppContext.SetSwitch("System.Drawing.EnableUnixSupport", false); |
There was a problem hiding this comment.
Why set it if it's false by default? Seems we should test default, and also test when explicitly enabled.
There was a problem hiding this comment.
We set it to false here as the runtimeconfig is setting it to true and the remote executor uses that same runtimeconfig file when creating the new process.
There was a problem hiding this comment.
If we accidentally changed the default to true, would any test fail?
There was a problem hiding this comment.
Nope. I could however try to add a test to ensure the default is false.
There was a problem hiding this comment.
In order to achieve this we would need to either create our own process or a separate assembly that doesn't set the value on the runtimeconfig.json, so I believe it is pretty complicated and not worth it, as in order to change the default value we would have to hardcode it in LocalAppContextSwitches like here:
Given that this is a one time release thing and already late on the release I think we are safe to not test that default value.
src/libraries/System.Resources.Extensions/tests/runtimeconfig.template.json
Show resolved
Hide resolved
|
Http3_MsQuic test failures are tracked in #56090. Should be fixed and merged shortly. Sorry for the inconvenience! |
|
Test failures are unrelated. Merging. |
|
/backport to release/6.0-preview7 |
|
Started backporting to release/6.0-preview7: https://github.com/dotnet/runtime/actions/runs/1054284714 |
|
The breaking change for this is already published. https://docs.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/system-drawing-common-windows-only |
ericstj
removed
the
needs-breaking-change-doc-created
|
Thanks, removed the label to indicate that. |
I choose the GDIP startup process to throw the PNSE as that is executed via an static constructor and from the scan I did there is not any interesting types left in System.Drawing.Common that don't use GDI+. Most of the ones that don't use it are data holders to represent data gathered from GDI+, or to pass in to another class as a parameter and then go down to the native layer.
But doing it from the GDIP startup process it means that some of these types that don't use or reference GDI+ directly can be used/created (they are not interesting to use), but those would not throw PNSE. Are we OK with that and then in .NET 7+ make every type on the assembly just throw or do we want to follow a different approach?
Relates to: dotnet/designs#234