-
Notifications
You must be signed in to change notification settings - Fork 17.9k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
x/mobile: gomobile bind
is ignoring target architecture parameter
#47212
Comments
More context: It looks like a PR was made whilst I was writing this issue (!) which might include fixes for the bug and missing functionality golang/mobile#70 |
@iamcalledrob hi there, golang/mobile#70 / http://golang.org/cl/334689 author here. It turns out that while golang/mobile#65 worked for some use cases, it subtly broke others. Sorry! @hajimehoshi asked if I could file an issue to discuss the changes in the new PR, but it turns out someone (you) already did. So to summarize what I think the problem is, and some thoughts on how we might fix it:
My goal with golang/mobile#70 was to address these subtle breakages, and update
@iamcalledrob could you try your project with this branch?
|
@hajimehoshi does the mobile repo have a macOS trybot? |
CC @hyangah The issue that a specified architecture is ignored (as the first comment says) is a regression. Also, we have multiple issues as @ydnar listed. I think we should revert the Catalyst change once, and then re-discuss the design. @ydnar What do you think?
This seems good, as macOS is not iOS, but let me think more...
I don't know about this, but I don't think there is. |
I’m OK reverting the change, and revising it on the new PR, since it preserves the earlier behavior of
The alternative would be for developers to manually specify all the platforms they want to build for, e.g.
Given that this needs to run tests on macOS with Xcode (and a developer environment with a cert), it’s probably a good idea. That would have caught a few of the broken tests, some of which predate the Catalyst PR. Is it possible to add one? |
CC @dmitshur |
Change https://golang.org/cl/334590 mentions this issue: |
I'd like to know whether there are things Go should care for macOS Catalyst, like what Go does for iOS. Or, is it enough for Go to behave as if the OS is macOS or iOS for macOS Catalyst? https://github.com/golang/go/search?q=filename%3A*_ios.go&type=code |
I found an issue in cgo that’s potentially blocking a real fix for this: #47228 |
@hajimehoshi my current workaround for #47228 is to supply both |
I removed target=darwin and target=iossimulator in current build of https://go-review.googlesource.com/c/mobile/+/334689 It also fixes OP’s bug, and builds with only a single target arch will now succeed. |
This reverts commit 76c259c. Reason for revert: Regression. Specifying architectures doesn't work. See golang/go#47212 Change-Id: I3200316cf28535cfb48e37636bc3b9d14d13e91e Reviewed-on: https://go-review.googlesource.com/c/mobile/+/334590 Trust: Hajime Hoshi <hajimehoshi@gmail.com> Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hajime Hoshi <hajimehoshi@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
I can confirm that things are working perfectly with your new branch (amazing!). The biggest benefit here is the build times while developing—since a build can now be for a specific arch/platform, the build time is reduced from ~90s to ~13s. For anyone else who might be following this, I had to also |
Following up, I'm unable to get builds with From peeking at the code, it looks like there's an assumption made that ios/arm64 is a physical device, and ios/amd64 is simulator. M1 complicates that assumption a bit :) |
Curious, are you able to build for iOS simulator with current I ran into an issue merging ios/arm64 and iphonesimulator/arm64 frameworks into a single XCFramework. Maybe the solution I chose (inhibit creating an iphonesimulator/arm64 slice) wasn’t right. |
I'm also not able to build for iOS simulator with the When building from the |
I think I got it! Going to run a couple more tests and push a commit: Edit: commit pushed. @iamcalledrob can you try with current HEAD?
Edit: building for iOS, iPhoneSimulator, macCatalyst, and macOS with arm64 and amd64 architectures in the same XCFramework now work. I’ve tested this with a simple app in a simulator: |
This enables building for iphonesimulator-arm64 and merging into a fat XCFramework that also may include ios/arm64. This should fix golang/go#47212 (comment) Note: tests are broken. Will fix in a followup commit.
Brilliant. I can confirm this now builds a working framework for the ios simulator on arm64! |
Terrific. I just pushed a couple updates. Target |
Nice work @ydnar i hope it's will be merged soon <3 |
@Milerius me too! @hajimehoshi what do you think about reviewing the updated CL? |
My understanding is that Go itself doesn't support Catalyst yet, then we should fix Go first. Is that correct? |
This CL doesn’t require any upstream fixes to Go to land it. It’s possible to improve this later if Go is Catalyst-aware, potentially supporting |
Oh sure. I'll take a look. Which CL are you talking about? https://go-review.googlesource.com/c/mobile/+/334689 still has Catalyst-related code. |
Yes, that one. Catalyst support is kind of the point.
|
As it is still impossible to support Catalyst by Go now, we should remove the code for Catalyst from the CL and leave TODO comments. Also please update the description of the CL. Let's revisit supporting Catalyst later. |
It’s not impossible. A Catalyst framework generated by |
I think it works in most cases but is not well tested by the Go team. The Go itself has iOS-specific code in the standard libraries, and we're not sure how Catalyst code should treat them. We might need more fix like Catalyst-specific code. |
I see your point. Given that x/mobile is outside of the Go core, and not subject to the same compatibility guarantee, what about marking Catalyst support as experimental?
There are 18 lines (10 SLOC) that mention "catalyst" in this CL, and it doesn’t interfere with the other platforms: $ grep -i catalyst ./cmd/gomobile/*
./cmd/gomobile/bind_darwinapp.go: // Catalyst support requires iOS 13+
./cmd/gomobile/bind_darwinapp.go: if platform == "maccatalyst" && v < 13.0 {
./cmd/gomobile/bind_darwinapp.go: return errors.New("catalyst requires -iosversion=13 or higher")
./cmd/gomobile/build.go: // Catalyst support requires iOS 13+
./cmd/gomobile/build.go: if platform == "maccatalyst" && v < 13.0 {
./cmd/gomobile/build.go: return nil, errors.New("catalyst requires -iosversion=13 or higher")
./cmd/gomobile/build.go:// ios,iossimulator,maccatalyst
./cmd/gomobile/doc.go: gomobile bind [-target android|ios|iossimulator|macos|maccatalyst] [-bootclasspath <path>] [-classpath <path>] [-o output] [build flags] [package]
./cmd/gomobile/doc.go:either android (the default) or ios, iossimulator, macos, maccatalyst.
./cmd/gomobile/doc.go: gomobile build [-target android|ios|iossimulator|macos|maccatalyst] [-o output] [-bundleid bundleID] [build flags] [package]
./cmd/gomobile/doc.go:default) or ios, iossimulator, macos, maccatalyst.
./cmd/gomobile/env.go:var darwinPlatforms = []string{"ios", "iossimulator", "macos", "maccatalyst"}
./cmd/gomobile/env.go: case "macos", "maccatalyst":
./cmd/gomobile/env.go: case "macos", "maccatalyst":
./cmd/gomobile/env.go: case "maccatalyst":
./cmd/gomobile/env.go: // TODO(ydnar): remove tag "ios" when cgo supports Catalyst
./cmd/gomobile/env.go: return []string{"ios", "macos", "maccatalyst"}
./cmd/gomobile/env.go: case "maccatalyst": In summary, I’m happy to split this CL into two parts if you think it’s necessary:
|
I slightly tend to against it, but the Go team might allow this. CC @hyangah |
I strongly support @ydnar's proposed fix here. |
@hajimehoshi @hyangah @ydnar What's the status on this? :-) |
You can use this branch now with a |
This whole project is highly experimental - so it's fine to include the catalyst handling code IMO. |
@nadimkobeissi @iamcalledrob golang.org/cl/334689 was merged, so Catalyst and macOS support should be working again. |
This is fixed in CL334689. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes (and only with the latest release)
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
I ran gomobile bind with a specific target and architecture (amd64 in this example)
Exact bind command:
Gomobile version
What did you expect to see?
I expected a framework to be built only for iOS with the amd64 architecture (i.e. for an intel simulator). I would expect to see something like this inside the xcframework:
What did you see instead?
The framework was built for all "iOS" platforms (macos, catalyst, simulator, iOS) and architectures (amd64, arm64).
The additional platforms are a result of the latest release (golang/mobile@76c259c).
I think this is a combination of both a bug and some missing functionality—i.e. being able to specify which platforms to build for in addition to which architectures.
There's a comment by another person on this release with more information on a specific bug regarding this: golang/mobile@76c259c#commitcomment-53412472
The text was updated successfully, but these errors were encountered: