Set CMAKE_OSX_ARCHITECTURES to target architecture #891
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes a bug where on M1 macOS machines, when building for the x86_64 iOS simulator, the cmake dependencies were building for arm64 instead, _if_ you had `/usr/sbin` in your `$PATH` used with bazel. The issue is that cmake calls `/usr/sbin/sysctl` to determine the host architecture: https://gitlab.kitware.com/cmake/cmake/-/blob/6453bd046ef23798c64333042d76851f15eff2fc/Modules/Platform/Darwin-Initialize.cmake#L23-41 And then it uses that computed value here if no other archs are set: https://gitlab.kitware.com/cmake/cmake/-/blob/6453bd046ef23798c64333042d76851f15eff2fc/Source/cmGeneratorTarget.cxx#L3378-3399 As far as I can tell this is a mismatch in expectation between how we call cmake vs how you would normally call it for cross compilation since we do not set `CMAKE_SYSTEM_NAME` bazel-contrib#523 By setting `CMAKE_OSX_ARCHITECTURES` ourselves to the target arch, we stop cmake from checking this default value. To get the target arch we use the Apple fragment, which as far as I can tell is the only way to get this from a bazel API. Alternatively we could theoretically parse the command line args and extract the arch from the `-target` but that felt more fragile. This option is ignored on non-Apple platforms, and it's conditional on bazel's Apple logic, so it shouldn't affect any other cases. This also updates the Apple related test to catch this case, which requires linking, which the ios_build_test was not doing. For now it also uses apple_binary to force the transition, which means rules_apple isn't required.
jsharpe
approved these changes
Apr 16, 2022
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug where on M1 macOS machines, when building for the
x86_64 iOS simulator, the cmake dependencies were building for arm64
instead, if you had
/usr/sbinin your$PATHused with bazel. Theissue is that cmake calls
/usr/sbin/sysctlto determine the hostarchitecture:
https://gitlab.kitware.com/cmake/cmake/-/blob/6453bd046ef23798c64333042d76851f15eff2fc/Modules/Platform/Darwin-Initialize.cmake#L23-41
And then it uses that computed value here if no other archs are set:
https://gitlab.kitware.com/cmake/cmake/-/blob/6453bd046ef23798c64333042d76851f15eff2fc/Source/cmGeneratorTarget.cxx#L3378-3399
As far as I can tell this is a mismatch in expectation between how we
call cmake vs how you would normally call it for cross compilation since
we do not set
CMAKE_SYSTEM_NAME#523By setting
CMAKE_OSX_ARCHITECTURESourselves to the target arch, westop cmake from checking this default value. To get the target arch we
use the Apple fragment, which as far as I can tell is the only way to
get this from a bazel API. Alternatively we could theoretically parse
the command line args and extract the arch from the
-targetbut thatfelt more fragile. This option is ignored on non-Apple platforms, and
it's conditional on bazel's Apple logic, so it shouldn't affect any
other cases.
This also updates the Apple related test to catch this case, which
requires linking, which the ios_build_test was not doing. For now it
also uses apple_binary to force the transition, which means rules_apple
isn't required.