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

[wip] Cleanup build script, use XCFramework #61

Closed
wants to merge 14 commits into from

Conversation

jurvis
Copy link
Contributor

@jurvis jurvis commented Apr 9, 2022

This PR is still a work in progress!
fixes #59

Purpose

This PR primarily does a few things:

  1. Support latest libtool@2.4.7 change which will require a PR merge in libwally-core (Update secp, build fixes, add M1 and universal2 support ElementsProject/libwally-core#321.
  2. Support running in iOS simulator on M1 Macs

Implementation

To achieve 1, I noticed that the latest libwally-core change no longer includes libsecp256k1 symbols inside the libwally-core.a library. To keep this simple, I imported libsecp256k1.a from CLibWally/libwally-core/src/secp256k1/.libs to the Xcode project

To achieve 2, we will have to build an xcodebuild -xcframework instead of using the previous lipo build command. This is because M1 macs use arm64 as an architecture for iOS simulator, which will conflict in a fat binary since an iOS simulator's arm64 arch is undifferentiated from an iOS device's arm64 arch.

Testing Notes

  • Cocoapods Install
  • Carthage script update (?)

Follow-up

  • Add Swift Package Manager support (?)
  • Now that we use xcframeworks, we can probably support Mac Catalyst too (Mac Catalyst build? #41)

@jurvis
Copy link
Contributor Author

jurvis commented Apr 9, 2022

@Sjors putting this PR up so you can track my progress with this.

Right now, I've gotten build-libwally.sh to spit out an xcframework for device-only, simulator-only, or both (with ElementsProject/libwally-core@59652b5 checked out as libwally-core)

Over the coming days I'll test it with Carthage/Cocoapods. Once that's done, you're OK with these changes, and ElementsProject/libwally-core#321 is merged, we can put out a release and everything should work again!

jurvis added 6 commits April 11, 2022 19:35
prevent the import from being exported in the interface, and allows us to not make it necessary for CLibwally headers to be present
@jurvis jurvis force-pushed the jurvis/cleanup-build-script branch from d4e3390 to 34961bf Compare April 13, 2022 05:45
@jurvis jurvis force-pushed the jurvis/cleanup-build-script branch from 34961bf to 188794c Compare April 13, 2022 05:45
@Sjors
Copy link
Owner

Sjors commented Apr 13, 2022

Thanks! This all sounds great, I'll give it a review soon(tm); I'm just back from the Miami conference, so a bit behind on ... everything.

I like the idea of using the Swift Package Manager, though I suggest a separate PR for that. I might even drop Cocoapods and Carthage once that works, unless there's a good reason other projects can't use the Swift Package Manager.

I just bumped the libwally-core version in #62 (unmerged), so you might want to rebase on that so you can drop 4050ff7 and 4588994.

@Sjors Sjors mentioned this pull request Apr 13, 2022
Copy link
Owner

@Sjors Sjors left a comment

Choose a reason for hiding this comment

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

This is a really nice cleanup! I'll test once you had a chance to rebase.

fi

./configure --disable-shared --host=$arch_target --enable-static --disable-elements
./configure --disable-shared --host=aarch64-apple-darwin --enable-static --disable-elements
Copy link
Owner

Choose a reason for hiding this comment

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

7d6ef31: why would this work on an intel mac?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, it probably won't. I just hard-coded it for now and can fix it later 😄

export CFLAGS="-O3 -arch arm64 -arch arm64e -arch armv7 -arch armv7s -fembed-bitcode -mios-version-min=10.0 -isysroot `xcrun -sdk iphoneos --show-sdk-path`"
export CXXFLAGS="-O3 -arch arm64 -arch arm64e -arch armv7 -arch armv7s -isysroot -fembed-bitcode -mios-version-min=10.0 -isysroot `xcrun -sdk iphoneos --show-sdk-path`"
export CFLAGS="-O3 -arch arm64 -fembed-bitcode -mios-version-min=11.0 -isysroot `xcrun -sdk iphoneos --show-sdk-path`"
export CXXFLAGS="-O3 -arch arm64 -fembed-bitcode -mios-version-min=11.0 -isysroot `xcrun -sdk iphoneos --show-sdk-path`"
Copy link
Owner

Choose a reason for hiding this comment

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

a3976b0: If you bump the minimum version, don't forget to update the Podspec.

For my own app (nthKey) I'm also fine with a bump to 15.0 if there's any benefit. But I don't know if anyone else is using LibWally-Swift and needs support for older versions of iOs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I bumped to 11.0 because 10.3 was the last supported iOS for >=armv7 (iPhone 5c, iirc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

think it will be nice to keep it at a generous minimum version, since developers may want to support them

if [ $device == 1 ]; then
cp src/.libs/libwallycore.a build/libwallycore-simulator.a
cd $PROJ_DIRECTORY
xcodebuild archive -scheme LibWally -destination "generic/platform=iOS Simulator" -archivePath ${BIN_OUTPUT_DIRECTORY}/LibwallySwift-Sim CLANG_ADDRESS_SANITIZER=NO CLANG_ADDRESS_SANITIZER_ALLOW_ERROR_RECOVERY=NO CLANG_ADDRESS_SANITIZER_USE_AFTER_SCOPE=NO SKIP_INSTALL=NO BUILD_LIBRARY_FOR_DISTRIBUTION=YES
Copy link
Owner

Choose a reason for hiding this comment

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

6d1bcab nit: maybe put CLANG_ADDRESS_SANITIZER etc on new lines

@@ -24,10 +24,10 @@ Pod::Spec.new do |spec|
spec.vendored_frameworks = "build/LibwallySwift.xcframework"

spec.pod_target_xcconfig = {
'SWIFT_WHOLE_MODULE_OPTIMIZATION' => 'YES',
Copy link
Owner

Choose a reason for hiding this comment

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

de7ecc9 Why the removal? Is it deprecated? Useless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, ignore all the diffs in podspec for now. This has turned into kind of a working branch for me to test Cocoapods like a monkey and figuring out how it all comes together 😅

@@ -67,7 +67,7 @@ if [ $simulator == 1 ]; then
fi
make
cd $PROJ_DIRECTORY
xcodebuild archive -scheme LibWally -destination "generic/platform=iOS Simulator" -archivePath ${BIN_OUTPUT_DIRECTORY}/LibwallySwift-Sim CLANG_ADDRESS_SANITIZER=NO CLANG_ADDRESS_SANITIZER_ALLOW_ERROR_RECOVERY=NO CLANG_ADDRESS_SANITIZER_USE_AFTER_SCOPE=NO SKIP_INSTALL=NO BUILD_LIBRARY_FOR_DISTRIBUTION=YES
Copy link
Owner

Choose a reason for hiding this comment

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

16cbc4f: can you squash this into 6d1bcab where you added them?

@@ -67,7 +67,7 @@ if [ $simulator == 1 ]; then
fi
make
cd $PROJ_DIRECTORY
xcodebuild archive -scheme LibWally -destination "generic/platform=iOS Simulator" -archivePath ${BIN_OUTPUT_DIRECTORY}/LibwallySwift-Sim
xcodebuild archive -scheme LibWally -destination "generic/platform=iOS Simulator" -archivePath ${BIN_OUTPUT_DIRECTORY}/LibwallySwift-Sim ONLY_ACTIVE_ARCH=NO SKIP_INSTALL=NO BUILD_LIBRARY_FOR_DISTRIBUTION=YES
Copy link
Owner

Choose a reason for hiding this comment

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

dde977c: please squash when you find the right ones :-)

@@ -6,7 +6,7 @@
Pod::Spec.new do |spec|

spec.name = "LibWally"
spec.version = "0.0.1"
spec.version = "0.0.7"
Copy link
Owner

Choose a reason for hiding this comment

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

188794c: no need to bump the version when you're trying this. You can point to a branch, see e.g. Sjors/nthkey-ios@ecf2513

I'll try to remember to bump this version next time I tag a release though.

@@ -17,12 +17,12 @@ Pod::Spec.new do |spec|
spec.platform = :ios, "10"
spec.swift_version = '5.0'

spec.source = { :git => "https://github.com/Sjors/libwally-swift.git", :tag => "v#{spec.version}", :submodules => true }
spec.source = { :git => "https://github.com/jurvis/libwally-swift.git", :tag => "v#{spec.version}", :submodules => true }
Copy link
Owner

Choose a reason for hiding this comment

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

60ed50a: don't forget to drop this. Also, I wonder if you really need that in order to test a fork.

Copy link
Contributor Author

@jurvis jurvis Apr 13, 2022

Choose a reason for hiding this comment

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

I did this to test pod spec lint LibWally.podspec since it'll use the information defined at spec.source and do its work.

@Sjors
Copy link
Owner

Sjors commented Apr 13, 2022

If it makes things easier (see #62 (comment)) feel free to create a separate PR that cherry-picks some commits from here.

@jurvis
Copy link
Contributor Author

jurvis commented Apr 14, 2022

@Sjors sorry I need some time to catch up on some school deadlines over the next couple of days. hoping to get this done by this weekend 😄

@jurvis jurvis closed this Apr 15, 2022
@Sjors
Copy link
Owner

Sjors commented Apr 15, 2022

Take your time!

By the way, it should be possible to just force push, rather than open a new PR.

@jurvis
Copy link
Contributor Author

jurvis commented Apr 15, 2022

@Sjors oh, yeah. Not sure why I didn’t think of that. Must’ve been late 😅

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