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.

Sign up for GitHub

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

Jump to bottom

Quote --sourcemap-output argument during ios build #31587

Closed
wants to merge 1 commit into from
Closed

Quote --sourcemap-output argument during ios build #31587

wants to merge 1 commit into from

Conversation

andersonvom
Copy link
Contributor

@andersonvom andersonvom commented May 25, 2021 •
edited
Loading

Summary

Scheme names may contain whitespace characters is used as path of the
path for various files during build time. This means any path that
includes the scheme name in it needs to be surrounded by quotes.

You can see the generated command below for a project with a scheme
called Some Scheme:

node ./node_modules/react-native/cli.js bundle \
  --entry-file index.js \
  --platform ios \
  --dev false \
  --reset-cache \
  --bundle-output './ios/derivedDataBuild/Build/Intermediates.noindex/ArchiveIntermediates/Some Scheme/BuildProductsPath/Release-iphoneos/Some Scheme.app/main.jsbundle' \
  --assets-dest './ios/derivedDataBuild/Build/Intermediates.noindex/ArchiveIntermediates/Some Scheme/BuildProductsPath/Release-iphoneos/Some Scheme.app' \
  --sourcemap-output ./ios/derivedDataBuild/Build/Intermediates.noindex/ArchiveIntermediates/Some Scheme/BuildProductsPath/Release-iphoneos/Some Scheme.app/main.jsbundle.map

--bundle-output and --assets-dest are properly quoted, but
--sourcemap-output is not.

This changes $EXTRA_ARGS to an array of strings so that we can
propertly quote $PACKAGER_SOURCEMAP_FILE when passing it to the
--sourcemap-output argument. When running the bundle command, this
array is unwrapped and all its elements passed as individual arguments.

It also applies the same unwrapping to $EXTRA_PACKAGER_ARGS so that
users can also pass an array of options when arguments containing spaces
are needed.

It's important to note that these changes ARE backwards compatible: if
$EXTRA_PACKAGER_ARGS is defined as a simple string, instead of an
array of strings, the command won't break, as it will still expand
correctly.

Changelog

[iOS] [Fixed] - Source map path for schemes containing whitespaces

Test Plan

With the new change, the generated command becomes:

node ./node_modules/react-native/cli.js bundle \
  --entry-file index.js \
  --platform ios \
  --dev false \
  --reset-cache \
  --bundle-output './ios/derivedDataBuild/Build/Intermediates.noindex/ArchiveIntermediates/Some Scheme/BuildProductsPath/Release-iphoneos/Some Scheme.app/main.jsbundle' \
  --assets-dest './ios/derivedDataBuild/Build/Intermediates.noindex/ArchiveIntermediates/Some Scheme/BuildProductsPath/Release-iphoneos/Some Scheme.app' \
  --sourcemap-output './ios/derivedDataBuild/Build/Intermediates.noindex/ArchiveIntermediates/Some Scheme/BuildProductsPath/Release-iphoneos/Some Scheme.app/main.jsbundle.map'

@facebook-github-bot
Copy link
Contributor

Hi @andersonvom!

Thank you for your pull request and welcome to our community.

Action Required

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

Process

In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@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 25, 2021
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@analysis-bot
Copy link

analysis-bot commented May 25, 2021 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: f4fdf4b

@charlesbdudley charlesbdudley self-assigned this Aug 30, 2021
@facebook-github-bot
Copy link
Contributor

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

@charlesbdudley
Copy link
Contributor

HI @andersonvom do you mind rebasing on top of latest so that we can get these circle ci tests passing?

@andersonvom
Quote --sourcemap-output argument during ios build
e2c3dbd 
Scheme names may contain whitespace characters is used as path of the
path for various files during build time. This means any path that
includes the scheme name in it needs to be surrounded by quotes.

You can see the generated command below for a project with a scheme
called `Some Scheme`:

    node ./node_modules/react-native/cli.js bundle \
      --entry-file index.js \
      --platform ios \
      --dev false \
      --reset-cache \
      --bundle-output './ios/derivedDataBuild/Build/Intermediates.noindex/ArchiveIntermediates/Some Scheme/BuildProductsPath/Release-iphoneos/Some Scheme.app/main.jsbundle' \
      --assets-dest './ios/derivedDataBuild/Build/Intermediates.noindex/ArchiveIntermediates/Some Scheme/BuildProductsPath/Release-iphoneos/Some Scheme.app' \
      --sourcemap-output ./ios/derivedDataBuild/Build/Intermediates.noindex/ArchiveIntermediates/Some Scheme/BuildProductsPath/Release-iphoneos/Some Scheme.app/main.jsbundle.map

`--bundle-output` and `--assets-dest` are properly quoted, but
`--sourcemap-output` is not.

This changes `$EXTRA_ARGS` to an array of strings so that we can
propertly quote `$PACKAGER_SOURCEMAP_FILE` when passing it to the
`--sourcemap-output` argument. When running the bundle command, this
array is unwrapped and all its elements passed as individual arguments.

It also applies the same unwrapping to `$EXTRA_PACKAGER_ARGS` so that
users can also pass an array of options when arguments containing spaces
are needed.

It's important to note that these changes ARE backwards compatible: if
`$EXTRA_PACKAGER_ARGS` is defined as a simple string, instead of an
array of strings, the command won't break, as it will still expand
correctly.
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,705,335 +0
android hermes armeabi-v7a 7,236,558 +0
android hermes x86 8,126,335 +0
android hermes x86_64 8,091,316 +0
android jsc arm64-v8a 9,624,993 +0
android jsc armeabi-v7a 8,543,744 +0
android jsc x86 9,640,286 +0
android jsc x86_64 10,248,954 +0

Base commit: f4fdf4b

@andersonvom
Copy link
Contributor Author

@charlesbdudley absolutely. we're back to all green! :D

@github-actions github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Sep 21, 2021
@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

8 similar comments
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

4 similar comments
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andersonvom in f3fe7a0.

When will my fix make it into a release? | Upcoming Releases

@andersonvom andersonvom deleted the patch-1 branch September 29, 2021 00:16
@janicduplessis
Copy link
Contributor

janicduplessis commented Oct 1, 2021 •
edited
Loading

@andersonvom This seems to be causing issues since bash doesn't support passing arrays in the environment.

I'm using originally:

export EXTRA_PACKAGER_ARGS="--unstable-transform-profile hermes-canary"

Which expands to

node cli.js bundle --config ./metro.config.js --entry-file apps/app/loyalty-appclip.index.js --platform ios --dev false --reset-cache '--unstable-transform-profile hermes-canary'

That's invalid since the extra packager arg is interpreted as a single parameter and the rn cli errors because of invalid parameter.

I tried using an array, but this doesn't seem supported by bash and no extra arguments is passed.

export EXTRA_PACKAGER_ARGS=("--unstable-transform-profile" "hermes-canary")

I suggest removing the change that affects EXTRA_PACKAGER_ARGS

janicduplessis added a commit to janicduplessis/react-native that referenced this pull request Oct 1, 2021
@janicduplessis
Revert "Quote --sourcemap-output argument during ios build (facebook#…
eb3ef6c 
…31587)"

This reverts commit f3fe7a0.
janicduplessis added a commit to janicduplessis/react-native that referenced this pull request Oct 19, 2021
@janicduplessis
Revert "Quote --sourcemap-output argument during ios build (facebook#…
62998cb 
…31587)"

This reverts commit f3fe7a0.
janicduplessis added a commit to janicduplessis/react-native that referenced this pull request Oct 21, 2021
@janicduplessis
Revert "Quote --sourcemap-output argument during ios build (facebook#…
310de04 
…31587)"

This reverts commit f3fe7a0.
janicduplessis added a commit to janicduplessis/react-native that referenced this pull request Nov 8, 2021
@janicduplessis
Revert "Quote --sourcemap-output argument during ios build (facebook#…
d7237af 
…31587)"

This reverts commit f3fe7a0.
janicduplessis added a commit to janicduplessis/react-native that referenced this pull request Nov 10, 2021
@janicduplessis
Revert "Quote --sourcemap-output argument during ios build (facebook#…
74147ce 
…31587)"

This reverts commit f3fe7a0.
janicduplessis added a commit to janicduplessis/react-native that referenced this pull request Nov 15, 2021
@janicduplessis
Revert "Quote --sourcemap-output argument during ios build (facebook#…
37f945a 
…31587)"

This reverts commit f3fe7a0.
janicduplessis added a commit to janicduplessis/react-native that referenced this pull request Nov 25, 2021
@janicduplessis
Revert "Quote --sourcemap-output argument during ios build (facebook#…
bde32c8 
…31587)"

This reverts commit f3fe7a0.
@oblador
Copy link
Contributor

oblador commented Nov 26, 2021

Just to confirm what @janicduplessis said, this is also breaking my use of EXTRA_PACKAGER_ARGS.

janicduplessis added a commit to janicduplessis/react-native that referenced this pull request Nov 30, 2021
@janicduplessis
Revert "Quote --sourcemap-output argument during ios build (facebook#…
8be1450 
…31587)"

This reverts commit f3fe7a0.
lunaleaps pushed a commit that referenced this pull request Nov 30, 2021
Revert "Quote --sourcemap-output argument during ios build (#31587)"
636f4c7 
This reverts commit f3fe7a0.
@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by cdce733.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@charlesbdudley charlesbdudley

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Needs: Attention Issues where the author has responded to feedback. Reverted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@andersonvom @facebook-github-bot @analysis-bot @charlesbdudley @react-native-bot @janicduplessis @oblador