-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Don't include windows PInvokes on Unix for System.Drawing #49405
Conversation
src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj
Outdated
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.
LGTM otherwise. Thanks!
src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs
Outdated
Show resolved
Hide resolved
I made some changes to conform with Interop guidelines so that There is more cleanup to do, it would be nice if SafeNativeMethods followed the Interop guidelines as well but that would be a very large change. I would like to do that in a separate PR if that's OK. |
Yes, that's ok. Thanks! |
FYI: @JeremyKuhne |
<Compile Include="misc\ExternDll.Unix.cs" /> | ||
<Compile Include="$(CommonPath)Interop\Unix\Interop.Libraries.cs" | ||
Link="Common\Interop\Unix\Interop.Libraries.cs" /> | ||
<Compile Include="Interop\Unix\Interop.Libraries.cs" /> |
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.
Could you also please update to not target Unix? I think we effectively support this library on Linux and macOS only not on any Unix* RIDs like tvOS or solaris.
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.
This library is cross compiled for windows and Unix. IIRC Unix is a parent of Linux, Solaris and macOS, so I'm just following the convention we have in other libraries here. I think whenever we start cross targeting for macOS or other targets if we ever do then we can adjust the paths for that.
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent);netcoreapp3.0-windows;netcoreapp3.0-Unix;netcoreapp3.0</TargetFrameworks> |
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.
Yep, which is not wrong because the library is not supported on "Unix". I'll open another issue if you don't want to mix the fix with this change
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.
Yep, which is not wrong because the library is not supported on "Unix".
You mean that cause Unix would imply iOS/tvOS and others we don't support right?
I'll open another issue if you don't want to mix the fix with this change
Sounds good. I rather keep this PR as is and have another issue to fix this.
Failures are unrelated and following up with dnceng. |
Fixes: #49275