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

Cleanup Build Script, Support XCframeworks #69

Merged
merged 5 commits into from
May 3, 2022

Conversation

jurvis
Copy link
Contributor

@jurvis jurvis commented Apr 15, 2022

Reopened of #61. Resolves #59.

Purpose

This PR primarily adds support for running apps that use Libwally-Swift in iOS simulator on M1 Macs. This is achieved by using xcframeworks, which will allow us down the road to support Mac Catalyst targets and publish it on Swift Package Manager.

Implementation

We heavily lean towards more modern approaches to packaging frameworks in this new iteration. Specifically, instead of using lipo which will prevent us from same having the architecture on multiple platforms in a single universal framework, we will turn to xcframeworks. Xcframeworks contain many slices organized by platform, and is platform-aware. This allows us to target more than one platform for the same architecture (e.g. arm64 ios simulator and arm64 ios device)

  1. First, we introduce a new External Build System target called LibWallyCore. The role of this target is to expose environment variables that Xcode passes to us via xcodebuild (either via the IDE or CLI) to any arbitrary build process. This allows us to not have to manage architecture-related logic within our shell script, and only build for the architectures relevant for our users.
  2. In order to back the External Build System with our own build process of compiling libwally-core, build-libwally.sh needed to be reworked so that it only does one thing: produce a C static library. It is the shell script that the aforementioned target calls when you hit "Build" or "Archive".
  3. Framework generation has been moved to build-libwally-swift.sh. This script exists solely for Cocoapods to build xcframeworks for a developer's local dev environment.

You can see the benefit of our new build system in action in build-libwally-swift.sh, where all we need to do is to call Xcode to build the LibWally scheme. And since it lists LibWallyCore as a dependent target, automatically builds the correct framework for the user.

Demo

User Perspective

With SwiftPM:
image

With Cocoapods:
image

Developer's Perspective

image

Testing Notes

Follow-up

- Add Swift Package Manager support (?)

@jurvis jurvis changed the title Cleanup Build Script, Support XCframeworks [wip] Cleanup Build Script, Support XCframeworks Apr 15, 2022
@jurvis
Copy link
Contributor Author

jurvis commented Apr 15, 2022

ack, forgot to mark this as draft. Still need to work on Cocoapods/Carthage support

@jurvis jurvis force-pushed the jurvis/cleanup-build-script-2 branch 8 times, most recently from 06b6492 to 0925fc8 Compare April 15, 2022 03:59
@jurvis
Copy link
Contributor Author

jurvis commented Apr 15, 2022

@Sjors managed to get Swift Package Manager working by following these steps and uploading a zipped version of the xcframework as a Github release with the following updates to Package.swift.

image

image

image

You can try by installing https://github.com/jurvis/libwally-swift as a dependency on any project.
Caveat: SPM seems to not like the v prefix at all. We may have to consider dropping it.

@jurvis
Copy link
Contributor Author

jurvis commented Apr 15, 2022

Still stumped by Cocoapods, opened an issue here CocoaPods/CocoaPods#11309.

We will probably use this build method for Carthage.

@jurvis jurvis mentioned this pull request Apr 15, 2022
@Sjors Sjors marked this pull request as draft April 15, 2022 08:18
@Sjors
Copy link
Owner

Sjors commented Apr 15, 2022

I'll try to get Swift package manager to work for nthKey. I'm tempted to just drop CocoaPods and Carthage if they can't be fixed, but let's try.

SPM seems to not like the v prefix at all. We may have to consider dropping it.

Sad, but possible of course.

Using binary distribution doesn't seem like a good idea, unless they support deterministic builds. Can't Swift packages be distributed as source code? Update I think it does: https://developer.apple.com/documentation/swift_packages/identifying_binary_dependencies

@jurvis
Copy link
Contributor Author

jurvis commented Apr 15, 2022

@Sjors yeah, every time you cut a release using a binary distribution, you will need to run swift package compute-checksum path/to/MyFramework.zip to compute a checksum to be specified in Package.swift.

When installing the package, Xcode checks and throws you a warning/error if they don't match.

@jurvis
Copy link
Contributor Author

jurvis commented Apr 15, 2022

We can look into distributing Swift packages as source, but that will probably involve finding a way to configure the Xcode project to run what we define ./build-libwally.sh to do. Probably a follow-up PR if we want to go down that route.

@Sjors
Copy link
Owner

Sjors commented Apr 15, 2022

When installing the package, Xcode checks and throws you a warning/error if they don't match.

But that only checks the binary of the checksum, and doesn't prove that binary is derived from the source, right?

Probably a follow-up PR if we want to go down that route.

That's fine, but in that case we have to make sure CocoaPods still works after this PR, because I'm not going to consume binary packages in my own project :-)

@jurvis
Copy link
Contributor Author

jurvis commented Apr 15, 2022

But that only checks the binary of the checksum, and doesn't prove that binary is derived from the source, right?

I want to say it does, mostly because the full compute-checksum command requires a package-path to be passed in as a parameter:

swift package --package-path /path/to/libwally-swift compute-checksum LibwallySwift.xcframework.zip

When running within the libwally-swift directory, it isn't necessary:

swift package compute-checksum LibwallySwift.xcframework.zip

I can ask around and see if I can get confirmation on this 😄

@Sjors
Copy link
Owner

Sjors commented Apr 15, 2022

The checksum of a zip file only tells you what is in the zip file. Even if the zip file contains both code and a binary, the checksum doesn't prove that the binary is actually generated from the code.

@jurvis
Copy link
Contributor Author

jurvis commented Apr 15, 2022

@Sjors ah, gotcha. I see your concern now. That makes sense. I'll keep tabs on the Cocoapods issue and update this PR to keep it working 😄

@jurvis
Copy link
Contributor Author

jurvis commented Apr 21, 2022

hey @Sjors I don't know how long we'll have to wait until we get back a response on our GitHub issue, I figured it may just be quicker if I figured out how to use Xcode to run an external build config so we won't need to manually call build-libwally.sh and handle every possible architecture we want to support.

in essence, Github projects will use the external build config target to pass a set of envvars, build the libraries it needs, and produce the framework necessary. I'm still working on it while we wait in this forked repo, but it's already opened up an opportunity for us to get GitHub CI in. (see: https://github.com/jurvis/libwally-swift/actions/runs/2200395095)

@jurvis jurvis changed the title [wip] Cleanup Build Script, Support XCframeworks Cleanup Build Script, Support XCframeworks May 1, 2022
@jurvis jurvis force-pushed the jurvis/cleanup-build-script-2 branch 2 times, most recently from 8d84209 to c6887eb Compare May 1, 2022 01:24
@jurvis jurvis marked this pull request as ready for review May 1, 2022 01:43
@jurvis
Copy link
Contributor Author

jurvis commented May 1, 2022

@Sjors fixed Cocoapods. I also updated the PR to include a more detailed implementation description alongside some new demo screenshots. I also attached two sample projects, with Cocoapods and SwiftPM, that you can clone to try.

@jurvis jurvis force-pushed the jurvis/cleanup-build-script-2 branch 2 times, most recently from 3392047 to 2dba1ac Compare May 2, 2022 02:42
@jurvis
Copy link
Contributor Author

jurvis commented May 2, 2022

@Sjors I think it should work for real now.

turns out the issue was with Cocoapods asking for support for multiple architectures (arm64 and x86_64) on Intel macs, which makes sense, since you'll need the arm64 version to archive for iOS devices and x86_64 for the simulator.

I needed to modify the script to essentially do almost the same thing as the old one by lipo-ing those two potential architectures. In Apple Silicon Macs, this wouldn't matter, because we'll only be building for one architecture -- arm64.

@Sjors
Copy link
Owner

Sjors commented May 2, 2022

Thanks, this worked!

Let's clean up the PR just a little bit more, so in the future it's easier to understand what changed.

Can you drop 8884c21? Adding a Swift package can be done in a separate PR, and it hardcodes a binary in your fork.

Can you squash a255ccb into the previous two commits?

I also get the impression that 2dba1ac is changing some of the things that were introduced in earlier commits. However, I'm not sure if squashing all of it is a good idea, since this PR is a rather big change.

Ideally each commit leaves the project in a working state, though that may be too much to ask with such a big change in the build process.

jurvis added 2 commits May 2, 2022 06:48
Adapt podspec to vendored xcframework

Use new build-libwally-swift to prebuild cocoapod

Create LibWallyCore scheme

For testing external builds

Cleanup build script

Remove development team from LibWallyCore target

Fix x86_64 builds

Update docs
@jurvis jurvis force-pushed the jurvis/cleanup-build-script-2 branch from 2dba1ac to 94fa0ad Compare May 2, 2022 13:49
@jurvis
Copy link
Contributor Author

jurvis commented May 2, 2022

Can you drop 8884c21? Adding a Swift package can be done in a separate PR, and it hardcodes a binary in your fork.

Sure thing! Reopened at #73

Can you squash a255ccb into the previous two commits?

Done

I also get the impression that 2dba1ac is changing some of the things that were introduced in earlier commits. However, I'm not sure if squashing all of it is a good idea, since this PR is a rather big change.

yeah, I deliberately left this one separate because it changes some of the static lib import behaviours in Xcode.

@jurvis jurvis force-pushed the jurvis/cleanup-build-script-2 branch from 94fa0ad to 8b60f26 Compare May 2, 2022 13:53
@jurvis jurvis mentioned this pull request May 2, 2022
2 tasks
@Sjors
Copy link
Owner

Sjors commented May 2, 2022

Why does the last commit message say "Delete Package.swift from pbxproj"?

@jurvis
Copy link
Contributor Author

jurvis commented May 2, 2022

@Sjors oh, it was my Git client that added it there. I was cleaning up the SPM commit and realized Xcode didn't remove the file import. I'll nix the commit message. all the trial-and-error has gotten me confused - my bad 😅

@jurvis jurvis force-pushed the jurvis/cleanup-build-script-2 branch from 8b60f26 to 5f52df6 Compare May 2, 2022 16:21
@Sjors
Copy link
Owner

Sjors commented May 2, 2022

Thanks, I'll do one more quick test and then merge this thing...

@Sjors
Copy link
Owner

Sjors commented May 2, 2022

One more issue... if you build for the simulator, e.g. to run the test suite, and then for the device, it complains:

Schermafbeelding 2022-05-02 om 19 34 39

Cleaning the build folder is not enough to make that go away. I had to do a git clean -dfx in the libwally-core submodule. That's probably too much to ask. Maybe we should put the build artefacts elsewhere?

Also, unrelated, and can wait for a followup, it might be a good idea to switch src/secp256k back to the correct commit after we finish building, so the submodule is not in a changed / dirty state.

@jurvis
Copy link
Contributor Author

jurvis commented May 3, 2022

@Sjors yeah, unfortunately, the only way to get it to work is to cd into libwally-core and run make clean. I don't think it's too much to ask at all, let me try and figure out how to do this quickly.

Also, unrelated, and can wait for a followup, it might be a good idea to switch src/secp256k back to the correct commit after we finish building, so the submodule is not in a changed / dirty state.

I'll file a new issue for this, I will prefer to close this earlier because I'll be busy over the next couple of days and don't want to block.

@jurvis
Copy link
Contributor Author

jurvis commented May 3, 2022

@Sjors I just thought about this a little more, I don't think we'll need to do anything too complicated.

I just turned off "Build Active Architectures Only" so Xcode knows to build for all supported architectures. In the case of dev, I believe it will be x86_64 and arm64. Sure, it'll take longer since we're building the framework to support both more architectures, but it's only for dev (and setting -destination in build-libwally-swift.sh will set the supported architecture it needs accordingly), so it's probably fine.

fwiw, build still green on CI: https://github.com/jurvis/libwally-swift/actions/runs/2261318103

@jurvis jurvis force-pushed the jurvis/cleanup-build-script-2 branch from ed14176 to cb2cdf4 Compare May 3, 2022 00:54
@Sjors
Copy link
Owner

Sjors commented May 3, 2022

@jurvis building both architectures is fine by me. I'll test again.

@Sjors
Copy link
Owner

Sjors commented May 3, 2022

Still getting that error. First I run the tests on the simulator, and then I try to build for a device:
Schermafbeelding 2022-05-03 om 09 49 36

I don't think it's actually building for the device:
Schermafbeelding 2022-05-03 om 09 55 40

@jurvis
Copy link
Contributor Author

jurvis commented May 3, 2022

@Sjors thanks for that! I was able to reproduce. I just tried adding my build artifact cleanup code snippet to the scheme's pre-build script instead.

can you try again? let me know how you'll like the commits squashed, I don't have an opinion either way.

@Sjors Sjors merged commit 645cde1 into Sjors:master May 3, 2022
@Sjors
Copy link
Owner

Sjors commented May 3, 2022

It worked!

@jurvis
Copy link
Contributor Author

jurvis commented May 3, 2022

@Sjors yay! thank you so much for your patience in review this. I will rebase the changes on #75 so we can get some nice CI running for the project :)

@jurvis jurvis deleted the jurvis/cleanup-build-script-2 branch May 3, 2022 15:02
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.

Error building libwally-swift with libtool@2.4.7
2 participants