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

cmd/gomobile: support Catalyst and Apple Silicon #63

Closed
wants to merge 5 commits into from

Conversation

waylybaye
Copy link

@waylybaye waylybaye commented Jan 29, 2021

This PR is derived from #45 .

I changed the build process and it will create 3 frameworks for ios, simulator and catalyst. Every framework has a fat library contains both arm64 and x86_64 (except ios that only needs arm64).

Finally create one xcframework from all 3 fat frameworks and this xcframework contains every platform and architecture.

dpwiese and others added 4 commits January 18, 2021 16:11
create one framework for all targets: ios, simulator and catalyst, every
framework contains at least one arch library, and combine all framework
into one xcframework
@google-cla
Copy link

google-cla bot commented Jan 29, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jan 29, 2021
@waylybaye
Copy link
Author

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Jan 29, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@aeddi
Copy link

aeddi commented Feb 2, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@dpwiese Can you confirm your consent with the bot? :)

@google-cla
Copy link

google-cla bot commented Feb 2, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@dpwiese
Copy link

dpwiese commented Feb 2, 2021

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Feb 2, 2021
@gopherbot
Copy link
Contributor

This PR (HEAD: 8c9ecea) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/mobile/+/288752 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://golang.org/doc/contribute.html#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.


Please don’t reply on this GitHub thread. Visit golang.org/cl/288752.
After addressing review feedback, remember to publish your drafts!

@hyangah hyangah changed the title Support Catalyst and Apple Silicon cmd/gomobile: support Catalyst and Apple Silicon Feb 2, 2021
Copy link

@dpwiese dpwiese left a comment

Choose a reason for hiding this comment

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

I'm happy to see this moving along, and think considering targets and architectures separately (as opposed to just treating catalyst as a new architecture as I had) is a much more elegant approach! And great to see Apple Silicon support added. 🎉

I've built my Sample.xcframework from this branch it it works on the target/architecture combos I've tested it quickly on: my old x86_64 mac with both simulator and catalyst, and my arm64 iPhone. Seems to work as expected. 👍

As far as tests, TestBindIOS fails now, but that's expected and should be a quick fix of updating the templates. I also have the TestBindWithGoModules tests failing. And finally TestIOSBuild is failing too, but that was already the case as mentioned in #45 (comment). I'm not sure how much effort should be placed towards these tests given they are failing already, but it would be very nice to get this helpful PR merged in. 🙂

cmd/gomobile/env.go Outdated Show resolved Hide resolved
@gopherbot
Copy link
Contributor

Message from Hyang-Ah Hana Kim:

Patch Set 3: Run-TryBot+1 Trust+1

(6 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/288752.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 3:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=79d4c5ae


Please don’t reply on this GitHub thread. Visit golang.org/cl/288752.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 68a32d9) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/mobile/+/288752 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@michaellady
Copy link

michaellady commented Feb 11, 2021

Would this PR make .xcframeworks built by gomobile compatible with Swift Package Manager (SPM)? If so, my team is very excited for this to move forward!

@ydnar
Copy link
Contributor

ydnar commented Mar 1, 2021

I’m trying to use this fork with a replace directive in my project’s go.mod file, and it fails with an error:

go: cloud.google.com/go@v0.37.0: missing go.sum entry; to add it:
	go mod download cloud.google.com/go

The full command output:

❯ gomobile bind -target ios ./mobile/ping
[debug] goenv:
[GOOS=darwin GOARCH=arm64 CC=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang CXX=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ CGO_CFLAGS=-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.4.sdk -mios-simulator-version-min=7.0 -fembed-bitcode -arch arm64 CGO_CXXFLAGS=-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.4.sdk -mios-simulator-version-min=7.0 -fembed-bitcode -arch arm64 CGO_LDFLAGS=-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.4.sdk -mios-simulator-version-min=7.0 -fembed-bitcode -arch arm64 CGO_ENABLED=1 ARCH=arm64 GOPATH=/var/folders/6g/1gng3zts0t39s_qbtt7p0wsc0000gn/T/gomobile-work-204926828:/Users/ydnar/go]
gomobile: darwin-arm64: go build -tags ios -buildmode=c-archive -o /var/folders/6g/1gng3zts0t39s_qbtt7p0wsc0000gn/T/gomobile-work-204926828/ping-arm64.a ./gobind failed: exit status 1
go: cloud.google.com/go@v0.37.0: missing go.sum entry; to add it:
	go mod download cloud.google.com/go

Go 1.16 on macOS 11.3 beta, using Xcode 12.4 on an Intel MB Pro.

Edit: I forked this fork and rebased on golang/mobile@master and it worked! ydnar#1

Edit 2: it was trivial to add macOS support to gomobile in this fork, which enables the framework to be packaged and distributed via Swift Package Manager:

https://developer.apple.com/documentation/swift_packages/distributing_binary_frameworks_as_swift_packages

@nadimkobeissi
Copy link

This is a pretty exciting and important PR, any idea when it will get merged?

Currently, if I try to install it manually, I get this strange error when I run gomobile bind:

[debug] goenv:
[GOOS=darwin GOARCH=arm64 CC=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang CXX=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ CGO_CFLAGS=-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.5.sdk -mios-simulator-version-min=7.0 -fembed-bitcode -arch arm64 CGO_CXXFLAGS=-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.5.sdk -mios-simulator-version-min=7.0 -fembed-bitcode -arch arm64 CGO_LDFLAGS=-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.5.sdk -mios-simulator-version-min=7.0 -fembed-bitcode -arch arm64 CGO_ENABLED=1 ARCH=arm64 GOPATH=/var/folders/sk/85yp792d7xb6nlmnzn23_jf40000gn/T/gomobile-work-735475716:/Users/nadim/go]
gomobile: darwin-arm64: go build -tags ios -gcflags -e -ldflags -s -w -trimpath -buildmode=c-archive -o /var/folders/sk/85yp792d7xb6nlmnzn23_jf40000gn/T/gomobile-work-735475716/MyGoLibrary-arm64.a ./gobind failed: exit status 1
go: github.com/BurntSushi/xgb@v0.0.0-20210121224620-deaf085860bc: missing go.sum entry; to add it:
	go mod download github.com/BurntSushi/xgb

Error aside, it would be really cool to see this merged and pushed into stable.

@ydnar
Copy link
Contributor

ydnar commented May 18, 2021

This is a pretty exciting and important PR, any idea when it will get merged?

Currently, if I try to install it manually, I get this strange error when I run gomobile bind:


[debug] goenv:

[GOOS=darwin GOARCH=arm64 CC=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang CXX=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ CGO_CFLAGS=-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.5.sdk -mios-simulator-version-min=7.0 -fembed-bitcode -arch arm64 CGO_CXXFLAGS=-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.5.sdk -mios-simulator-version-min=7.0 -fembed-bitcode -arch arm64 CGO_LDFLAGS=-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.5.sdk -mios-simulator-version-min=7.0 -fembed-bitcode -arch arm64 CGO_ENABLED=1 ARCH=arm64 GOPATH=/var/folders/sk/85yp792d7xb6nlmnzn23_jf40000gn/T/gomobile-work-735475716:/Users/nadim/go]

gomobile: darwin-arm64: go build -tags ios -gcflags -e -ldflags -s -w -trimpath -buildmode=c-archive -o /var/folders/sk/85yp792d7xb6nlmnzn23_jf40000gn/T/gomobile-work-735475716/MyGoLibrary-arm64.a ./gobind failed: exit status 1

go: github.com/BurntSushi/xgb@v0.0.0-20210121224620-deaf085860bc: missing go.sum entry; to add it:

	go mod download github.com/BurntSushi/xgb

Error aside, it would be really cool to see this merged and pushed into stable.

See #65 for a build that fixes that issue

@gadireddi226
Copy link

When this PR will be merged ?

gopherbot pushed a commit that referenced this pull request Jul 10, 2021
Add support for macOS (non-Catalyst) and Catalyst targets.

The compiled library is packaged into a "fat" XCFramework file (as
opposed to a Framework), which includes binaries for iOS, macOS,
MacCatalyst (iOS on macOS), and iOS Simulator targets, for amd64 and
arm64 architectures.

The generated XCFramework file is suitable for distribution as a binary
Swift Package Manager package:
https://developer.apple.com/documentation/swift_packages/distributing_binary_frameworks_as_swift_packages

This change is based on earlier work:
#45
#63

Fixes golang/go#36856

Change-Id: Iabe535183c7215c68838d6c8f31618d8bceefdcf
GitHub-Last-Rev: 623f8f3
GitHub-Pull-Request: #65
Reviewed-on: https://go-review.googlesource.com/c/mobile/+/310949
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Hajime Hoshi <hajimehoshi@gmail.com>
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Trust: Hajime Hoshi <hajimehoshi@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
@deepumukundan
Copy link

Guess we can close this PR now that the fork with these and additional changes has been merged in! Finally closure!

@waylybaye
Copy link
Author

closed since #65 has been merged

@waylybaye waylybaye closed this Jul 11, 2021
@nadimkobeissi
Copy link

Thank you so much!

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