Skip to content
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

Add type-forwarders for Xamarin.Android compatibility to System.Drawing.Common.dll #82839

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

akoeplinger
Copy link
Member

The legacy Xamarin.Android version of System.Drawing.Common.dll contained these types, add forwarders so we don't get a TypeLoadException when using an assembly compiled against that in modern .NET Android.

Fixes #82829

…ng.Common.dll

The legacy Xamarin.Android version of System.Drawing.Common.dll contained these types, add forwarders so we don't get a TypeLoadException when using an assembly compiled against that in modern .NET Android.

Fixes dotnet#82829
@ghost
Copy link

ghost commented Mar 1, 2023

Tagging subscribers to this area: @dotnet/area-system-drawing
See info in area-owners.md if you want to be subscribed.

Issue Details

The legacy Xamarin.Android version of System.Drawing.Common.dll contained these types, add forwarders so we don't get a TypeLoadException when using an assembly compiled against that in modern .NET Android.

Fixes #82829

Author: akoeplinger
Assignees: -
Labels:

area-System.Drawing

Milestone: -

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// This is required for back-compatibility with legacy Xamarin which had these types in System.Drawing.Common.dll
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are adding these to the implementation assembly only. That's intentional, right?

Copy link
Member

@filipnavara filipnavara Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is intentional. We only need the type-forwarders for binary compatibility at runtime. I am not sure how it works with the ApiCompat checks though.

SystemColors and ColorTranslator have their respective TypeForwardedTo in the ref sources. These two types were moved between assemblies between .NET Core 2 and 3 IIRC. Do I understand correctly that the implementation assembly previously got the TypeForwardedTo for these two types thanks to IsPartialFacadeAssembly = true?

Copy link
Member Author

@akoeplinger akoeplinger Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is intentional. I'll suppress the ApiCompat checks since we don't expect having these types in the ref.

Do I understand correctly that the implementation assembly previously got the TypeForwardedTo for these two types thanks to IsPartialFacadeAssembly = true?

Yes. We could remove the IsPartialFacadeAssembly=true given we have SystemColors and ColorTranslator explicitly here, or remove these two from the explicit list and let the partial facade generator cover them. @ViktorHofer any preference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we'd still need the IsPartialFacadeAssembly for the other TFMs so we'd need a conditional. I'll just leave it as is.

Copy link
Member

@ViktorHofer ViktorHofer Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with that approach and it follows what we do in our shims builds (manual type forwards only exposed in src but not ref). Just to get a second pair of eyes, let me cc @ericstj

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from adding a comment to the suppression file.

@akoeplinger akoeplinger merged commit e486f38 into dotnet:main Mar 8, 2023
@akoeplinger akoeplinger deleted the drawing-forwarders branch March 8, 2023 13:29
akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Mar 8, 2023
…ng.Common.dll (dotnet#82839)

The legacy Xamarin.Android version of System.Drawing.Common.dll contained these types, add forwarders so we don't get a TypeLoadException when using an assembly compiled against that in modern .NET Android.

Fixes dotnet#82829

(cherry picked from commit e486f38)
carlossanlop added a commit that referenced this pull request Mar 9, 2023
…ity to mscorlib.dll and System.Drawing.Common.dll (#83137)

* Add more type-forwarders for Xamarin.Android compatibility to mscorlib.dll (#82618)

The legacy Xamarin.Android version of mscorlib.dll differed a bit compared to the .NET Framework mscorlib.dll, mostly because of additions for .NET Standard 2.1 support.

This meant that an assembly which was compiled against that mscorlib expects types there but since we didn't have type-forwarders in our mscorlib.dll shim to point them to the right assembly you'd get a TypeLoadException when running on modern .NET 6 Android.

Fixes #82193

(cherry picked from commit d8203e7)

* Add type-forwarders for Xamarin.Android compatibility to System.Drawing.Common.dll (#82839)

The legacy Xamarin.Android version of System.Drawing.Common.dll contained these types, add forwarders so we don't get a TypeLoadException when using an assembly compiled against that in modern .NET Android.

Fixes #82829

(cherry picked from commit e486f38)

* Use 7.0 version of the ApiCompat suppressions

* Fix diff

---------

Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com>
@ghost ghost locked as resolved and limited conversation to collaborators Apr 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workaround broken Mono System.Drawing facades
3 participants