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

Update build-hermes-xcode.sh to fail faster #47894

Closed

Conversation

kraenhansen
Copy link
Contributor

@kraenhansen kraenhansen commented Nov 21, 2024

Summary:

While debugging an error building hermes from source in a React Native app, I kept getting this weird error:

Building for 'iOS-simulator', but linking in dylib ({redacted}/ios/Pods/hermes-engine/destroot/Library/Frameworks/ios/hermes.framework/hermes) built for 'macOS'

The root cause was a call to cmake failing, but /sdks/hermes-engine/utils/build-hermes-xcode.sh didn't exit on the error and instead continued building the hermes.framework from the dummy frameworks created by ./sdks/hermes-engine/utils/create-dummy-hermes-xcframework.sh.

I suggest fixing this by introducing a set -e call similar to that used in build-hermesc-xcode.sh:

Changelog:

[Internal] - Ensure building hermes from source exits early on failures

Test Plan:

I followed https://github.com/facebook/hermes/blob/main/doc/ReactNativeIntegration.md getting to a state of building a React Native app with Hermes built from source. I then introduced an error in hermes/API/hermes/hermes.cpp (I simply typed asd in the top of the file) and built the app from Xcode:

Screenshot 2024-11-22 at 15 13 04

If you scroll up, you can see the failed cmake build, but it just continues trying to build the framework, effectively hiding the error:

Screenshot 2024-11-22 at 15 14 19

When applying this patch and cleaning the build folder, the error is more prominent and actionable:

Screenshot 2024-11-22 at 15 18 27

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 21, 2024
@dmytrorykun
Copy link
Contributor

@kraenhansen thank you for this PR.
Could you please change [IOS] [FIXED] to [Internal].
And also update the test plan with the error messages before and after the change.

@kraenhansen
Copy link
Contributor Author

I've updated the description and uploaded a few screenshots to illustrate the before and after state of things 🤞

@facebook-github-bot
Copy link
Contributor

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

@kraenhansen
Copy link
Contributor Author

Two of the jobs seem to be CI infrastructure related and one seem to be a legitimate build failure for Android / Maven, which this PR can't possibly touch (as the script is only supposed to be called through xcode).

@dmytrorykun
Copy link
Contributor

@kraenhansen could you please rebase this PR to re-trigger the CI?

@kraenhansen kraenhansen force-pushed the kh/fail-build-hermes-fast branch from e7ceccc to 22e251c Compare December 5, 2024 13:32
@kraenhansen
Copy link
Contributor Author

Done 👍

@kraenhansen
Copy link
Contributor Author

Failed to install CMake
At D:\a\_temp\74e1642d-151b-4b05-a7d0-9b8772823ca1.ps1:4 char:18
+   if (-not $?) { throw "Failed to install CMake" }
+                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (Failed to install CMake:String) [], RuntimeException
    + FullyQualifiedErrorId : Failed to install CMake

I can't see that being caused by my suggested change 🙈

@dmytrorykun
Copy link
Contributor

@kraenhansen I believe the fix for this CI issue has landed (github.com//pull/48122). Please try rebasing once again.

@kraenhansen kraenhansen force-pushed the kh/fail-build-hermes-fast branch from 22e251c to 1970403 Compare December 6, 2024 14:27
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@dmytrorykun merged this pull request in acecf99.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Dec 6, 2024
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @kraenhansen in acecf99

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.

4 participants