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

Make PlatformConfiguration properties trimmable (fixes tvOS compilation) #2717

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

maxkatz6
Copy link
Contributor

Description of Change

Previously SkiaSharp used old APIs to conditionally execute platform specific APIs. Specifically, RuntimeInformation.IsOSPlatform is known to be non-trimmable.

It caused some Windows PInvokes to be included in the non-windows binaries. For most platform it didn't really cause any problems, but apparently tvOS linker is more sensitive (see #2715)

This PR changes PlatformConfiguration class:

  1. Use newer trimmer friendly OperatingSystem.IsWindows() method instead of non-trimmable RuntimeInformation.IsOSPlatform (OSPlatform.Windows).
  2. Remove static ctor, and instead move these checks to the property getters.

On .NET6+ targets with trimmer enabled these calls will be eliminated and values will be inlined.

Non trimmed SkiaSharp:
Screenshot 2024-01-14 at 10 25 25 PM

Trimmed SkiaSharp with "-r tvos-arm64" (hint for the compiler to replace OperatingSystem.IsWindows with "false").
As we can see, windows specific class is completely removed as unused.
Screenshot 2024-01-14 at 10 25 35 PM

Bugs Fixed

API Changes

None.

Behavioral Changes

None.

Required skia PR

None.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Merged related skia PRs
  • Changes adhere to coding standard
  • Updated documentation

This PR should also be backportable with no conflicts.

@maxkatz6
Copy link
Contributor Author

Alternatively, we can add more preprocessor conditional checks for each target, eliminating code blocks during per specific target framework. In this way SkiaSharp for "net7.0-tvos" would never include windows code to begin with.
Though, it's probably unnecessary. Also, it would complexity.

@maxkatz6
Copy link
Contributor Author

@mattleibow hi! How does this PR look? CI build failing seems to be unrelated.

mattleibow
mattleibow previously approved these changes Jan 22, 2024
@mattleibow mattleibow enabled auto-merge (squash) January 22, 2024 14:01
@mattleibow mattleibow added the backport/release/2.x Backport this PR to release/2.x label Jan 22, 2024
@mattleibow
Copy link
Contributor

Retrying thre build. Sometimes the tests explode for no reason.

@mattleibow
Copy link
Contributor

The macos tests are still crashing... I have retried 5 times and a fail already retries by default...

auto-merge was automatically disabled January 28, 2024 03:16

Head branch was pushed to by a user without write access

@maxkatz6 maxkatz6 force-pushed the platform-config-conditional-fixes branch from cb7c83c to c579dbd Compare January 28, 2024 05:36
@maxkatz6
Copy link
Contributor Author

maxkatz6 commented Jan 28, 2024

@mattleibow that's weird, as with .NET 6+, RuntimeInformation API should simply redirect to OperatingSystem API, making them equivalent (except of trimming capabilities): https://github.com/dotnet/runtime/blob/v6.0.26/src/libraries/System.Runtime.InteropServices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/RuntimeInformation.cs#L52

CI worker only when I tried to revert my changed... I wonder if it's just a consistent fluke or some hidden detail behind CI infra here.
Trying to see if it works with OperatingSystem.IsWindows () only, without changing behavior for other platforms.

@mattleibow
Copy link
Contributor

I see some other PRs are crashing on macOS... I wonder if some of my GC tests are crashing on a newer .net service release. But this is green now, so I am thinking this is intermittant... It can't be the check is crashing the test runner...

@maxkatz6
Copy link
Contributor Author

@mattleibow okay. Let me know if any changes are required from my side. I can rebase this PR back to the state where OperatingSystem was used on every platform, not only Windows.

@mattleibow
Copy link
Contributor

Maybe do that, let's make this code right and I will continue to investigate why CI is crashing. It looks also to just be the public bot.

@maxkatz6 maxkatz6 force-pushed the platform-config-conditional-fixes branch from c579dbd to a5b4987 Compare January 31, 2024 23:27
@maxkatz6
Copy link
Contributor Author

@mattleibow done

@mattleibow mattleibow merged commit 8c296e5 into mono:main Feb 2, 2024
2 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 2, 2024
mattleibow pushed a commit that referenced this pull request Mar 1, 2024
(cherry picked from commit 8c296e5)

Co-authored-by: Max Katz <maxkatz6@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/release/2.x Backport this PR to release/2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Publishing tvOS application with SkiaSharp fails
2 participants