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

Convert to only SwiftPM, use XCFramework on Apple systems #23

Merged
merged 18 commits into from
May 1, 2023
Merged

Convert to only SwiftPM, use XCFramework on Apple systems #23

merged 18 commits into from
May 1, 2023

Conversation

mbullington
Copy link
Contributor

@mbullington mbullington commented Mar 6, 2021

Hey Miguel!

Thanks for being super awesome about patches and working with me on this.

Summary of the changes:

  • Removed old XCode project and switched entirely to SwiftPM.
  • Changed how payloads are downloaded:
  • If on Linux, download SkiaSharp.NativeAssets.Linux and extract the .so that supports these distros: More Pre-Built Linux Libraries mono/SkiaSharp#453
  • If on macOS, download SkiaSharp and extract a bunch of .frameworks, lipo them separate architectures (.xcframework limitation), then make a new SkiaSharp.xcframework that can be used by SwiftPM/XCode.
  • Moved around (not sure if will stick) headers and "shared" code so it was included when you list SkiaKit as a dependency in Swift Package Manager. The XCode sample app uses the XCode 11+ SwiftPM integration and works great.
  • The Apple components that interact with UIView and CALayer I switched to use #if canImport(UIKit) and #if canImport(QuartzCore). They'll just not be included on Linux/other but I think this'll work out of the box with macOS Catalyst.

How to use:

I added a GH action that downloads both payloads and then 'publishes' them to a branch named generated. SwiftPM/XCode projects can use this branch directly and not need to build anything. On Linux I think you'd still need to ship the .so alongside the project.

Unfortunately, SwiftPM integration in XCode requires a git repository and cannot do relative/absolute file paths. To fit this model I had to split out the samples into a separate repo: https://github.com/bloomos/SkiaKitSamplesiOS

Below is the process required to add SkiaKit to a SwiftUI project (in the video, my sunset app Twilight!) and a video of the example app running.

Screen.Recording.2021-09-06.at.4.32.33.PM.mov
Screen.Recording.2021-09-06.at.4.29.38.PM.mov

Thanks again!

@mbullington mbullington marked this pull request as draft March 6, 2021 20:50
@migueldeicaza
Copy link
Owner

Hello!

Thank you for this contribution! Apologies, I am usually spammed with notifications, and I guess this did not show up on my dashboard.

I will review and see if I can merge it, thank you so much for doing that work!

@migueldeicaza
Copy link
Owner

I tried using the pull request, but I am not quite sure how to produce a binary artifact that can be reused.

The download script certainly produces a newer version of the xcframework, but I do not know how I am supposed to use SkiaKit itself after this change.

Lastly, I do not mind dropping Xcode support for the build that I had, and move entirely to SwiftPM, as it has never been a pleasant experience. I would want to ship some working samples for the assorted platforms, but would love some guidance as to how I am supposed to reference the Swift package

@mbullington mbullington changed the title (work-in-progress) Use XCFrameworks to distribute SkiaSharp on apple*OS systems Convert to only SwiftPM, use XCFramework on Apple systems Sep 6, 2021
@mbullington
Copy link
Contributor Author

mbullington commented Sep 6, 2021

Hey @migueldeicaza ! Thanks for your patience here, I've been moving and doing a lot of personal tasks to get settled.

I hope I answered most of your questions in the Description, I'm going to update the READMEs soon but feel this is ready for review.

I also made some more bindings for my Wayland project, but one change at a time. :)

Best,

@mbullington mbullington marked this pull request as ready for review September 6, 2021 21:05
@mbullington
Copy link
Contributor Author

Last thing I think this PR needs fixed is this, I'm sure there's a really easy solution but I'm not super versed in Linux shared library linking.

https://github.com/bloomos/SkiaKit/runs/3528061024?check_suite_focus=true

@mbullington
Copy link
Contributor Author

Also there was an issue that was crashing my real iPhone in SkiaView.swift and SkiaCanvasLayer.swift. I was able to fix by persisting the bitmap data until next render or deinit, the same functionality as SkiaSharp.

https://github.com/mono/SkiaSharp/blob/main/source/SkiaSharp.Views/SkiaSharp.Views.Apple/SKCGSurfaceFactory.cs

@migueldeicaza
Copy link
Owner

Hello Michael,

This looks like an amazing patch! I will review it and hopefully merge it soon, and will try to address the couple of issues that you brought up on this thread.

I have myself just moved, exactly 14 days ago, and we have been slowly unpacking, and have been generally exhausted after a long day of unpacking, and today was my first evening checking Github. It might take me a couple more days, but fear not, I am on the case.

Thanks once again!

@mbullington
Copy link
Contributor Author

Hello Michael,

This looks like an amazing patch! I will review it and hopefully merge it soon, and will try to address the couple of issues that you brought up on this thread.

I have myself just moved, exactly 14 days ago, and we have been slowly unpacking, and have been generally exhausted after a long day of unpacking, and today was my first evening checking Github. It might take me a couple more days, but fear not, I am on the case.

Congrats on the move! Take as much time as you need :)

Thanks once again!

@migueldeicaza
Copy link
Owner

Hello,

I have a question about this:

Unfortunately, SwiftPM integration in XCode requires a git repository and cannot do relative/absolute file paths. To fit this model I had to split out the samples into a separate repo: https://github.com/bloomos/SkiaKitSamplesiOS

Does the challenge happen when you try to reference this repository from another project, and any samples conflict, or it means that Xcode is unable to open a Package.swift that contains iOS samples as well?

@migueldeicaza
Copy link
Owner

@migueldeicaza
Copy link
Owner

I love the patch, and I think I can make the samples work with the above approach.

That said, one thing that is worrying me a bit, is the use of the "generated" branch to deploy the artifacts.

The generated branch will bloat the GitHub repo with binaries, but most importantly, they will override other versions over time, making historically different versions of SkiaKit not work, or break behind the scenes of people that end up using that branch over time.

I wonder if there are other options that we could use to get the binary artifacts that does not rely on that. branch.

@migueldeicaza
Copy link
Owner

Additional thoughts: I wonder if rather than publishing to a branch generated if this can publish an official release, which would have the advantage of not bloating the repository, and could be versioned. The secondary question is whether this can publish the full "tree" as expected by SwiftPM, or if it can only publish the binary artifacts.

@migueldeicaza
Copy link
Owner

Additional Research:

  • Perhaps we could publish the XCFramework artifact as a release through some other way, which would cover the Apple platforms.
  • For other platforms, the Package.swift could run a function that would download the asset before the targets are defined.

@migueldeicaza
Copy link
Owner

Ok did a quick script that will download+publish binary artifacts here:

https://github.com/migueldeicaza/SkiaKitPayloads/releases

(The source for this is there as well). Will try tomorrow to switch the binary dependecy to this on MacOS, and do the unsavory hack on Linux.

@Allan121
Copy link

Were there any updates on this one? We're just about to start a project that would love to make use of this.

@migueldeicaza
Copy link
Owner

Apologies, I forgot where I was at.

I would need to resume the work :-)

@migueldeicaza
Copy link
Owner

Additional details, we could use this for SwiftPM 5.6:

https://github.com/apple/swift-evolution/blob/main/proposals/0305-swiftpm-binary-target-improvements.md

@dempfi
Copy link

dempfi commented Apr 12, 2023

This is a great PR to get Skia runnable in a cross-platform Swift application. Would love to see traction on this initiative. I recognize you folks have work/life/priorities so how can we, onlookers, help you drive this to a finish line?

@migueldeicaza
Copy link
Owner

I have a couple of thoughts:

  1. There is a brand new way of shipping xcframeworks that we should use to download the runtime payloads on Apple systems, and we could still ship the binaries on the side for other platforms.
  2. It would be easier to do this in incremental steps, this patch as it stands is too large, and hard to see what happened (it was years since I last read it, and I dread going through it again, since so much was file moving).

Would love to get small patches to move us towards there.

@dempfi
Copy link

dempfi commented Apr 18, 2023

I've tried to run this PR and it works without issues with the framework from https://www.nuget.org/packages/SkiaSharp.NativeAssets.macOS. So all in all this PR is good.

@mbullington
Copy link
Contributor Author

This was quite a patch!

I’m with @migueldeicaza as it worked well at the time, but I’ve lost context on the scope of it.

I’m also not super tuned into any Swift changes since then!

But would be happy to review any more incremental ways of pulling this out, or just testing on my devices. :)

@migueldeicaza
Copy link
Owner

I am glad that the patch still works!

There is still the issue that I do not think we should be checking-in the artifacts on a generated branch, that just seems like it will bloat over time the git repo.

I think an ideal solution is to either have a script that given a published release of the Sharp bindings, can produce a binary artifact in the form of an .xcframework, and then we can publish those manually as a release (which is what GitHub encourages you to do for this).

So we would need something like:

  1. Script that produces a binary payload, which we would run manually, and produces a binary XCFramrwork
  2. Have the SwiftPM package use the new .binaryTarget to reference the XCFramework published.

So it would not bloat the repo and anger our GitHub overloads, but also, we would not keep a history of bloated and old binaries that people need to download, and we would be aligned with both GitHub and Swift.

I am willing to blindly merge almost everything else at this point to move to this new world.

@migueldeicaza migueldeicaza merged commit 1a7e142 into migueldeicaza:main May 1, 2023
@migueldeicaza
Copy link
Owner

Ok, I have manually merged this, and rather than committing the binaries to a branch, they are now binary payloads that come directly from GitHub.

I tested it with both a standalone app, and the iOS sample, and they work.

I also brought back the iOS sample and checked it into a subdirectory of this module.

@migueldeicaza
Copy link
Owner

@mbullington Thank you so much for getting this patch in place, and I am terribly sorry to you and everyone else for having taken so long to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants