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

chore: Increase iOS script portability #44417

Closed
wants to merge 5 commits into from

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented May 5, 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

Test Plan:

@facebook-github-bot facebook-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
Copy link

analysis-bot commented May 6, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,542,747 -6
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,912,806 -11
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 93c079b
Branch: main

@legobeat legobeat marked this pull request as ready for review May 6, 2024 00:14
@facebook-github-bot facebook-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
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
Copy link
Contributor Author

legobeat commented May 6, 2024

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)

Copy link
Contributor

@cipolleschi cipolleschi left a comment

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
Contributor

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

@legobeat
Copy link
Contributor Author

Rebased on main; no changes.

@legobeat
Copy link
Contributor Author

legobeat commented May 16, 2024

@cipolleschi Thanks!

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

@cipolleschi
Copy link
Contributor

/rebase - this comment automatically rebase on top of main

@cipolleschi
Copy link
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
Copy link
Contributor Author

legobeat commented May 20, 2024

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
Contributor

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

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

@cipolleschi merged this pull request in 9970480.

Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied 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.

None yet

5 participants