Skip to content

chore: Increase iOS script portability - #44417

Closed
legobeat wants to merge 5 commits into
react:mainfrom
legobeat:pods-outside-macos
Closed

chore: Increase iOS script portability#44417
legobeat wants to merge 5 commits into
react:mainfrom
legobeat:pods-outside-macos

Conversation

@legobeat

@legobeatlegobeat commented May 5, 2024

Copy link
Copy Markdown
Contributor

Summary:

pod install and CocoaPods are actually not macOS specific.
Still, the pod lifecycle scripts of react-native depend on macOS-only utilities and will fail on Linux.

This is an attempt to make the scripts portable and make the pod install cleanly on Linux as well as macOS.

Changelog:

[INTERNAL] [FIXED] - Skip XCode patching when not run on macOS
[INTERNAL] [FIXED] - Fall back to `which gcc`/`which g++` to identify C/C++ compiler when `xcrun` not available
[INTERNAL] [FEAT] - Recognize CC and CXX env vars supplied to the script and prefer them over autodetection

Test Plan:

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 5, 2024
@analysis-bot

analysis-bot commented May 6, 2024

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a19,542,747-6
androidhermesarmeabi-v7an/a--
androidhermesx86n/a--
androidhermesx86_64n/a--
androidjscarm64-v8a22,912,806-11
androidjscarmeabi-v7an/a--
androidjscx86n/a--
androidjscx86_64n/a--

Base commit: 93c079b
Branch: main

@legobeat
legobeat marked this pull request as ready for review May 6, 2024 00:14
@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label May 6, 2024
@NickGerleman

Copy link
Copy Markdown
Contributor

I kinda suspect this breaks a lot of other assumptions in the build logic. What is use-case and how far does this actually get?

@legobeat

legobeat commented May 6, 2024

Copy link
Copy Markdown
ContributorAuthor

I kinda suspect this breaks a lot of other assumptions in the build logic. What is use-case and how far does this actually get?

I think the use-case can be summarized as "can run pod install for a project using react-native as dependency on Linux without error". This can be useful e.g. in CI jobs validating Podfile.lock consistency as well as for auditing and experimental development on Linux.

I believe this will retain existing behavior for any scenario that wasn't already erroring before this change.

(Separately, it does feel surprising even on macOS that installing this as a dependency would be automatically and silently be changing the user's Xcode project settings - but that's not the point here)

@cipolleschicipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes looks good to me.
I know that cocoapods can be used outside MacOS, but we never encountered the use case before.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@legobeat
legobeatforce-pushed the pods-outside-macos branch from 9a2400e to 5af62fcCompareMay 14, 2024 22:51
@legobeat

Copy link
Copy Markdown
ContributorAuthor

Rebased on main; no changes.

@legobeat

legobeat commented May 16, 2024

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi Thanks!

Some small improvements, should be good to go now I believe. PR description updated.

@legobeat
legobeatforce-pushed the pods-outside-macos branch from be345f5 to e8ec34cCompareMay 16, 2024 23:05
@cipolleschi

Copy link
Copy Markdown
Contributor

/rebase - this comment automatically rebase on top of main

@cipolleschi

Copy link
Copy Markdown
Contributor

Sorry for the delay, I have been busy with the React Conf and couldn't really work on anything else. I'm rebasing automatically on top of main, given that many tests were red in CI.

- fix: Don't attempt xcrun if not present
- fix: Default to g++ instead of gcc for CXX when Xcode not present
- feat: Allow user to override CC and CXX env vars
@legobeat

legobeat commented May 20, 2024

Copy link
Copy Markdown
ContributorAuthor

Sorry for the delay, I have been busy with the React Conf and couldn't really work on anything else. I'm rebasing automatically on top of main, given that many tests were red in CI.

Thanks! I am surprised to see the errors show up. I just reverted the change which allows overriding the Xcode CC and CXX via env vars to see if that's what's causing it.

EDIT: Looks like that was it.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 20, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cipolleschi merged this pull request in 9970480.

@github-actions

Copy link
Copy Markdown

This pull request was successfully merged by @legobeat in 9970480.

When will my fix make it into a release? | How to file a pick request?

legobeat added a commit to MetaMask/metamask-mobile that referenced this pull request May 29, 2024
- Enable `pod install` step in `yarn setup` script on all platforms
- Failing installation is still only treated as actual failure on macOS
- Apply `react-native` patch that resolves installation error due to
attempt at patching missing Xcode
- Upstream PR: react/react-native#44417
- Add developer `Dockerfile` which can be used locally to run routine
development tasks like `yarn setup`.
- Add CI job that builds docker image and tests it with metamask-mobile
- In order for the new job to run, the newly used actions must be
enabled in repository settings.
cipolleschi pushed a commit that referenced this pull request Jun 4, 2024
Summary:
`pod install` and CocoaPods are actually not macOS specific.
Still, the pod lifecycle scripts of `react-native` depend on macOS-only utilities and will fail on Linux.
This is an attempt to make the scripts portable and make the pod install cleanly on Linux as well as macOS.
## Changelog:
[INTERNAL] [FIXED] - Skip XCode patching when not run on macOS
[INTERNAL] [FIXED] - Fall back to `which gcc`/`which g++` to identify C/C++ compiler when `xcrun` not available
[INTERNAL] [FEAT] - Recognize CC and CXX env vars supplied to the script and prefer them over autodetection
Pull Request resolved: #44417
Reviewed By: NickGerleman
Differential Revision: D57055928
Pulled By: cipolleschi
fbshipit-source-id: 1c49f70c52b4667abf0a215cbee52ee6aa6dd052
kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
Summary:
`pod install` and CocoaPods are actually not macOS specific.
Still, the pod lifecycle scripts of `react-native` depend on macOS-only utilities and will fail on Linux.
This is an attempt to make the scripts portable and make the pod install cleanly on Linux as well as macOS.
## Changelog:
[INTERNAL] [FIXED] - Skip XCode patching when not run on macOS
[INTERNAL] [FIXED] - Fall back to `which gcc`/`which g++` to identify C/C++ compiler when `xcrun` not available
[INTERNAL] [FEAT] - Recognize CC and CXX env vars supplied to the script and prefer them over autodetection
Pull Request resolved: react#44417
Reviewed By: NickGerleman
Differential Revision: D57055928
Pulled By: cipolleschi
fbshipit-source-id: 1c49f70c52b4667abf0a215cbee52ee6aa6dd052
This was referenced Jun 28, 2024
@cipolleschicipolleschi mentioned this pull request Jul 22, 2024
facebook-github-bot pushed a commit that referenced this pull request Sep 17, 2024
…46358)
Summary:
Due to `set -e` shell option, missing `xcrun` may cause the setup script to fail on Linux.
Also makes the script continue even if the glog `./configure` call does not succeed.
### Related
- Follow-up to #44417
## Changelog:
- [Internal] [Fixed] - react-native: make missing xcrun not throw in `ios-configure-glog.sh`
- [Internal] [Fixed] - react-native: accept failing glog ./configure in `ios-configure-glog.sh`
Pull Request resolved: #46358
Reviewed By: cortinico
Differential Revision: D62851737
Pulled By: cipolleschi
fbshipit-source-id: 3ed76a81b6f428a1697c364698a0dfdd3bbb74f2
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Shared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@legobeat@analysis-bot@NickGerleman@facebook-github-bot@cipolleschi