[iOS] Fix build of glog & Flipper-Glog on Apple Silicon #32486
Conversation
|
Hi @rayzr522! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
the CLA site appears to be broken at the moment, so I'm unable to sign -- tried both Safari and Google Chrome, it redirects to an error page after authorizing thru GitHub OAuth... guess I'll try again in a day or two? |
react-native-bot
added
Bug
Platform: iOS
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
react-native-bot
removed
the
No CLA
facebook-github-bot
added
the
Shared with Meta
|
@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| cat <<\EOF >>fix_glog_0.3.5_apple_silicon.patch | ||
| diff --git a/config.sub b/config.sub | ||
| index 1761d8b..43fa2e8 100755 | ||
| --- a/config.sub | ||
| +++ b/config.sub | ||
| @@ -1096,6 +1096,9 @@ case $basic_machine in | ||
| basic_machine=z8k-unknown | ||
| os=-sim | ||
| ;; | ||
| + arm64-*) | ||
| + basic_machine=$(echo $basic_machine | sed 's/arm64/aarch64/') | ||
| + ;; | ||
| none) | ||
| basic_machine=none-none | ||
| os=-none | ||
| EOF |
There was a problem hiding this comment.
Can you move this to a separate file? That can make it easier to read and iterate on in the future.
There was a problem hiding this comment.
sure! any preference on where that file lives? and do you know where this script is run relative to?
There was a problem hiding this comment.
Hmm… maybe you can create that file as a sibling to this one (scripts/) and reference it by fetching the path to the current file?
Something like this…
THIS_DIR=$(cd -P "$(dirname "$(readlink "${BASH_SOURCE[0]}" || echo "${BASH_SOURCE[0]}")")" && pwd)
patch … "$THIS_DIR/fix_glog_0.3.5_apple_silicon.patch"
There was a problem hiding this comment.
@yungsters so i did some experimenting, and the issue is that we aren't actually calling this script directly, but rather reading the contents and passing that directly into the podspec:
https://github.com/facebook/react-native/blob/main/third-party-podspecs/glog.podspec#L14
therefore there is no ${BASH_SOURCE[0]}, as the script is called directly via bash -c, basically eval'ing the contents of the file. so i see three solutions:
- interpolate the scripts dir path into the script programmatically after we
File.readthe script - change the glog.podspec file to call the script directly (i.e.
/bin/bash /absolute/path/to/scripts/ios-configure-glog.sh), and thus be able to infer that path - leave it as is in the PR, dynamically generating the file when needed
do you have a preference? my gut says option 2 because it feels like the "most correct" use of this bash script, but it also is a more fundamental change to how this script is being used and may have ramifications i don't see right now
There was a problem hiding this comment.
Reading the contents of the file into the podspec? Oh boy… 😬
Thanks for investigating, haha. Hmm… #1 increases complexity so I prefer not doing that.
I agree that #2 seems more right, but I would (maybe incorrectly) presume that this was done a certain way for some reason. But I am all for trying it out!
How about this… I'll merge this PR as-is, and you can optionally follow-up with #2 to improve general engineering of this. If that ends up being a rabbit hole or if you don't have time for it, then we still resolved the issue you set out to initially resolve.
rayzr522
force-pushed
the
fix-glog-on-apple-silicon
branch
from
ac41b74
to
0c32d64
Compare
|
@yungsters merged this pull request in 25d4cb9. |
Summary: Reverts #32486 because this commit is suspected of breaking some internal CI. Changelog: [iOS][Changed] - Reverts "Apple Silicon builds of glog & Flipper-Glog" Reviewed By: mendoncakeegan Differential Revision: D32129681 fbshipit-source-id: e195009f2ab202cd4e30b978a1ca89dc388c9ddf
Summary: NOTE: Second attempt at merging #32486 (D32080994 (25d4cb9)). This is a fixed version of the #32380 PR. It solves a typo, prevents variable substitution in the patch file, and moves it to a better place in the script so that CURRENT_ARCH is actually detected before checking whether to patch. The `config.sub` included in glog is too old and does not recognize `arm64-*` as a valid arch when building. This, combined with an out of date Flipper-Glog version, results in persistent build failures on Apple Silicon machines. p.s. i assume all the podfile lock changes were caused by me running this on an Apple Silicon Mac, and thus all the pod checksums were run against the arm64 versions of those pods rather than the normal x86_64 versions. if this is an issue I can revert the changes to that file, but it would seem to be an inevitable issue in future PR diffs... ## Changelog - [iOS] [Fixed] - Apple Silicon builds of glog & Flipper-Glog Pull Request resolved: #32486 Test Plan: See `react-native-oss-ios` Sandcastle job succeed. Reviewed By: fkgozali Differential Revision: D32256761 Pulled By: yungsters fbshipit-source-id: c7f32b72287018f070910b26aad02aa0adf4a61f
Summary
This is a fixed version of the #32380 PR. It solves a typo, prevents variable substitution in the patch file, and moves it to a better place in the script so that CURRENT_ARCH is actually detected before checking whether to patch.
The
config.subincluded in glog is too old and does not recognizearm64-*as a valid arch when building. This, combined with an out of date Flipper-Glog version, results in persistent build failures on Apple Silicon machines.p.s. i assume all the podfile lock changes were caused by me running this on an Apple Silicon Mac, and thus all the pod checksums were run against the arm64 versions of those pods rather than the normal x86_64 versions. if this is an issue I can revert the changes to that file, but it would seem to be an inevitable issue in future PR diffs...
Changelog
Test Plan
pod installinpackages/rn-tester