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

fix(xcode): Pass hermesc $EXTRA_COMPILER_ARGS as individual arguments #38199

Closed
wants to merge 1 commit into from

Conversation

krystofwoldrich
Copy link
Contributor

@krystofwoldrich krystofwoldrich commented Jul 5, 2023

Summary:

If "$EXTRA_COMPILER_ARGS" is wrapped by " it will be passed to hermesc binary as one argument which will cause Unknown command line argument error when using $SOURCEMAP_FILE to generate Hermes source maps.

+ EXTRA_COMPILER_ARGS='-O -output-source-map'
+ /ios/Pods/hermes-engine/destroot/bin/hermesc -emit-binary -max-diagnostic-width=80 '-O -output-source-map' -out /dtnzzmpchfyuyyajisnbghavfleb/Build/Products/Release-iphonesimulator/sampleNewArchitecture.app/main.jsbundle /dtnzzmpchfyuyyajisnbghavfleb/Build/Products/Release-iphonesimulator/main.jsbundle
hermesc: Unknown command line argument '-O -output-source-map'.  Try: '/ios/Pods/hermes-engine/destroot/bin/hermesc -help'

This error doesn't happen by default as the only argument in $EXTRA_COMPILER_ARGS is -O but if you add an extra argument or use $SOURCEMAP_FILE which results in multiple extra compiler args the error will happen.

Introduced in a168f4b
Affects 0.72.0 and 0.72.1

Changelog:

[IOS] [FIXED] - Pass hermesc $EXTRA_COMPILER_ARGS as individual arguments

Test Plan:

Run build with $SOURCEMAP_FILE or any other $EXTRA_COMPILER_ARGS which has more than one argument.

If "$EXTRA_COMPILER_ARGS" is wrapped by `"` it will be passed to `hermesc` binary as one argument which will cause `Unknown command line argument` error when using `$SOURCEMAP_FILE` to generate Hermes source maps.

```
+ EXTRA_COMPILER_ARGS='-O -output-source-map'
+ /Users/krystofwoldrich/repos/sentry-react-native/sample-new-architecture/ios/Pods/hermes-engine/destroot/bin/hermesc -emit-binary -max-diagnostic-width=80 '-O -output-source-map' -out /Users/krystofwoldrich/Library/Developer/Xcode/DerivedData/sampleNewArchitecture-dtnzzmpchfyuyyajisnbghavfleb/Build/Products/Release-iphonesimulator/sampleNewArchitecture.app/main.jsbundle /Users/krystofwoldrich/Library/Developer/Xcode/DerivedData/sampleNewArchitecture-dtnzzmpchfyuyyajisnbghavfleb/Build/Products/Release-iphonesimulator/main.jsbundle
hermesc: Unknown command line argument '-O -output-source-map'.  Try: '/Users/krystofwoldrich/repos/sentry-react-native/sample-new-architecture/ios/Pods/hermes-engine/destroot/bin/hermesc -help'
```
@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. p: Sentry Partner: Sentry Partner labels Jul 5, 2023
@kelset kelset requested a review from cipolleschi July 5, 2023 16:28
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,061,532 -2
android hermes armeabi-v7a 8,310,872 -1
android hermes x86 9,577,749 -3
android hermes x86_64 9,420,183 +1
android jsc arm64-v8a 9,613,325 +2
android jsc armeabi-v7a 8,739,950 -3
android jsc x86 9,700,268 -1
android jsc x86_64 9,946,827 +0

Base commit: 1489f0f
Branch: main

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.

I will have to do something internally to make the linter happy. :(

@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.

@byCedric
Copy link
Contributor

byCedric commented Jul 6, 2023

Seems that this is the same as #38147 right? 😄

@cipolleschi
Copy link
Contributor

Yes, Thanks @byCedric!
@krystofwoldrich, as the other PR has been opened before this one, I'll proceed importing and landing that one, instead.

@cipolleschi cipolleschi closed this Jul 6, 2023
@krystofwoldrich
Copy link
Contributor Author

@byCedric @cipolleschi Thanks, I've missed that one.

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. p: Sentry Partner: Sentry Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants