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 support for --sourcemap-output path containing spaces in Xcode projects #40937

Conversation

JorenVos
Copy link
Contributor

This PR contains the changes from #30981 that got closed due to inactivity.

Many thanks to @nickdowell for this bug report & fix. We encountered this error in our project when we had an Xcode scheme that contains a space (like AppName alpha).

Summary:

This change fixes the generation of source maps for Xcode projects where the output path contains spaces.

The EXTRA_ARGS environment variable, being a plain string, would be split into arguments by whitespace - so a path containing spaces was being treated as several arguments rather than one.

This change uses an array to contain the arguments instead, allowing the proper handling of arguments that may contain spaces.

Changelog:

[iOS] [Fixed] - Fix support for --sourcemap-output path containing spaces

Test Plan:

Tested using a sample project with the following "Bundle React Native code and images" Xcode build phase

export SOURCEMAP_FILE="$CONFIGURATION_BUILD_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/main.jsbundle.map"
set -e
export NODE_BINARY=node
../node_modules/react-native/scripts/react-native-xcode.sh

and a CONFIGURATION_BUILD_DIR that contains spaces - ~/Library/Xcode/Derived Data. You can also try an XCode-scheme that contains a space.

Before

+ EXTRA_ARGS=
+ case "$PLATFORM_NAME" in
+ BUNDLE_PLATFORM=ios
+ EMIT_SOURCEMAP=
+ [[ ! -z /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map ]]
+ EMIT_SOURCEMAP=true
+ PACKAGER_SOURCEMAP_FILE=
+ [[ true == true ]]
+ [[ '' == true ]]
+ PACKAGER_SOURCEMAP_FILE='/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map'
+ EXTRA_ARGS=' --sourcemap-output /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map'
+ node /Users/nick/Desktop/RN064/node_modules/react-native/cli.js bundle --entry-file index.js --platform ios --dev false --reset-cache --bundle-output '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/main.jsbundle' --assets-dest '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app' --sourcemap-output /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map
                    Welcome to Metro!
              Fast - Scalable - Integrated


info Writing bundle output to:, /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/main.jsbundle
info Writing sourcemap output to:, /Users/nick/Library/Developer/Xcode/Derived

Note the incorrect sourcemap output path.

After

+ EXTRA_ARGS=()
+ case "$PLATFORM_NAME" in
+ BUNDLE_PLATFORM=ios
+ EMIT_SOURCEMAP=
+ [[ ! -z /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map ]]
+ EMIT_SOURCEMAP=true
+ PACKAGER_SOURCEMAP_FILE=
+ [[ true == true ]]
+ [[ '' == true ]]
+ PACKAGER_SOURCEMAP_FILE='/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map'
+ EXTRA_ARGS+=("--sourcemap-output")
+ EXTRA_ARGS+=("$PACKAGER_SOURCEMAP_FILE")
+ node /Users/nick/Desktop/RN064/node_modules/react-native/cli.js bundle --entry-file index.js --platform ios --dev false --reset-cache --bundle-output '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/main.jsbundle' --assets-dest '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app' --sourcemap-output '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map'
                    Welcome to Metro!
              Fast - Scalable - Integrated


info Writing bundle output to:, /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/main.jsbundle
info Writing sourcemap output to:, /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map

sourcemap output path fixed 🎉

This change fixes the generation of source maps for Xcode projects where the output path contains spaces.
@facebook-github-bot
Copy link
Contributor

Hi @JorenVos!

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 [email protected]. Thanks!

@JorenVos
Copy link
Contributor Author

CLA should be fine.

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,222,539 -1
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 20,592,679 -2
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: edb7332
Branch: main

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. 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 Oct 13, 2023
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@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 Oct 13, 2023
@JorenVos
Copy link
Contributor Author

When will this PR be merged?

@JorenVos
Copy link
Contributor Author

JorenVos commented Nov 2, 2023

Bump

1 similar comment
@JorenVos
Copy link
Contributor Author

Bump

@NickGerleman
Copy link
Contributor

@cipolleschi any concerns with this?

@cipolleschi
Copy link
Contributor

The changes looks good to me, the problem is that those script are very brittle.
I remember once I landed a PR that looked innocent, it was wrapping a shell variable in " and that broke some setup because they were passing already escaped paths.

These changes seem to sanitize the params, so I'm positive in try to import and landing these.
If they break something, somebody will reach out, but I think that these changes are for the best.

@cortinico
Copy link
Contributor

Also see #41845

@maykonmichel
Copy link

App is crashing for me when I add these changes.

*** Terminating app due to uncaught exception 'RCTFatalException: Unhandled JS Exception: Property 'Reproducer' doesn't exist

ReferenceError: Property 'Reproducer' doesn't exist
    at global (address at /Users/maykon/Library/Developer/CoreSimulator/Devices/A2F4DE2D-DC69-43A7-B897-744B56C8813E/data/Containers/Bundle/Application/A87E55DB-E847-458E-A806-71BA6CA1DFAF/Reproducer App.app/main.jsbundle:1:13284)', reason: 'Unhandled JS Exception: Property 'Reproducer' doesn't exist

ReferenceError: Property 'Reproducer' doesn't exist
    at global (address at /Users/maykon/Library/Developer/Core...'
*** First throw call stack:
(
	0   CoreFoundation                      0x0000000180491128 __exceptionPreprocess + 172
	1   libobjc.A.dylib                     0x000000018008412c objc_exception_throw + 56
	2   Reproducer App                      0x000000010078ed6c RCTFormatError + 0
	3   Reproducer App                      0x00000001007a4edc __28-[RCTCxxBridge handleError:]_block_invoke + 516
	4   libdispatch.dylib                   0x00000001010980f0 _dispatch_call_block_and_release + 24
	5   libdispatch.dylib                   0x000000010109993c _dispatch_client_callout + 16
	6   libdispatch.dylib                   0x00000001010a95e4 _dispatch_main_queue_drain + 1228
	7   libdispatch.dylib                   0x00000001010a9108 _dispatch_main_queue_callback_4CF + 40
	8   CoreFoundation                      0x00000001803f1a30 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 12
	9   CoreFoundation                      0x00000001803ec148 __CFRunLoopRun + 1936
	10  CoreFoundation                      0x00000001803eb5a4 CFRunLoopRunSpecific + 572
	11  GraphicsServices                    0x000000018e9fbae4 GSEventRunModal + 160
	12  UIKitCore                           0x00000001852f02e4 -[UIApplication _run] + 868
	13  UIKitCore                           0x00000001852f3f5c UIApplicationMain + 124
	14  Reproducer App                      0x0000000100746530 main + 80
	15  dyld                                0x0000000101011544 start_sim + 20
	16  ???                                 0x00000001011b60e0 0x0 + 4313538784
	17  ???                                 0xaf4a000000000000 0x0 + 12630908104913977344
)
libc++abi: terminating due to uncaught exception of type NSException

Reproducible example here.

I'm setting SOURCEMAP_FILE to "$DERIVED_FILE_DIR/main.jsbundle.map" and this is probably an edge case of your code. In my case the fix was pretty simple as you can see on PR #41845.

@cipolleschi
Copy link
Contributor

Hi @JorenVos and @maykonmichel, thanks for your PRs.

@maykonmichel did you apply your PR (#41845) on top of this one? I'm trying to understand whether we need both or yours would be enough for everyone.

maykonmichel added a commit to maykonmichel/reproducer-react-native that referenced this pull request Dec 18, 2023
@maykonmichel
Copy link

I did some tests and these are the results:

I have addressed and resolved all scenarios individually tested in #41845. This fix can be applied either independently or on top of #40937. In the case of #40937, there is a crash when setting SOURCEMAP_FILE="$DERIVED_FILE_DIR/main.jsbundle.map". From my analysis, it seems safe to merge only my pull request, unless there are additional issues that I may have overlooked.

@cipolleschi
Copy link
Contributor

/rebase - this comment will rebase this PR on top of main

@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 Dec 28, 2023
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in e25a9b4.

Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
…ojects (facebook#40937)

Summary:
This PR contains the changes from facebook#30981 that got closed due to inactivity.

Many thanks to nickdowell for this bug report & fix. We encountered this error in our project when we had an Xcode scheme that contains a space (like `AppName alpha`).

This change fixes the generation of source maps for Xcode projects where the output path contains spaces.

The `EXTRA_ARGS` environment variable, being a plain string, would be split into arguments by whitespace - so a path containing spaces was being treated as several arguments rather than one.

This change uses an array to contain the arguments instead, allowing the proper handling of arguments that may contain spaces.

bypass-github-export-checks

## Changelog:

[iOS] [Fixed] - Fix support for --sourcemap-output path containing spaces

Pull Request resolved: facebook#40937

Test Plan:
Tested using a sample project with the following "Bundle React Native code and images" Xcode build phase

```
export SOURCEMAP_FILE="$CONFIGURATION_BUILD_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/main.jsbundle.map"
set -e
export NODE_BINARY=node
../node_modules/react-native/scripts/react-native-xcode.sh
```

and a `CONFIGURATION_BUILD_DIR` that contains spaces - `~/Library/Xcode/Derived Data`. **You can also try an XCode-scheme that contains a space.**

### Before

```
+ EXTRA_ARGS=
+ case "$PLATFORM_NAME" in
+ BUNDLE_PLATFORM=ios
+ EMIT_SOURCEMAP=
+ [[ ! -z /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map ]]
+ EMIT_SOURCEMAP=true
+ PACKAGER_SOURCEMAP_FILE=
+ [[ true == true ]]
+ [[ '' == true ]]
+ PACKAGER_SOURCEMAP_FILE='/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map'
+ EXTRA_ARGS=' --sourcemap-output /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map'
+ node /Users/nick/Desktop/RN064/node_modules/react-native/cli.js bundle --entry-file index.js --platform ios --dev false --reset-cache --bundle-output '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/main.jsbundle' --assets-dest '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app' --sourcemap-output /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map
                    Welcome to Metro!
              Fast - Scalable - Integrated

info Writing bundle output to:, /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/main.jsbundle
info Writing sourcemap output to:, /Users/nick/Library/Developer/Xcode/Derived
```
Note the incorrect sourcemap output path.

### After

```
+ EXTRA_ARGS=()
+ case "$PLATFORM_NAME" in
+ BUNDLE_PLATFORM=ios
+ EMIT_SOURCEMAP=
+ [[ ! -z /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map ]]
+ EMIT_SOURCEMAP=true
+ PACKAGER_SOURCEMAP_FILE=
+ [[ true == true ]]
+ [[ '' == true ]]
+ PACKAGER_SOURCEMAP_FILE='/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map'
+ EXTRA_ARGS+=("--sourcemap-output")
+ EXTRA_ARGS+=("$PACKAGER_SOURCEMAP_FILE")
+ node /Users/nick/Desktop/RN064/node_modules/react-native/cli.js bundle --entry-file index.js --platform ios --dev false --reset-cache --bundle-output '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/main.jsbundle' --assets-dest '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app' --sourcemap-output '/Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map'
                    Welcome to Metro!
              Fast - Scalable - Integrated

info Writing bundle output to:, /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/main.jsbundle
info Writing sourcemap output to:, /Users/nick/Library/Developer/Xcode/Derived Data/RN064-cpnwckdferodycbevupbrkjydate/Build/Products/Release-iphonesimulator/RN064.app/main.jsbundle.map
```
sourcemap output path fixed 🎉

Reviewed By: arushikesarwani94

Differential Revision: D52431057

Pulled By: cipolleschi

fbshipit-source-id: 528217c84fe3f467a30baa15cfa4dcb2ed713165
facebook-github-bot pushed a commit that referenced this pull request Jan 10, 2024
Summary:
Properly handle targets with spaces in their names

Use quotes around the argument to basename path. Ensures this succeeds when the Xcode target name has spaces.

example:
```sh
#!/bin/sh
SOURCEMAP_FILE="ArchiveIntermediates/Jane Doe/IntermediateBuildFilesPath/Jane Doe.build/Release-iphoneos/JaneDoe.build/DerivedSources/main.jsbundle.map"
BN_SOURCEMAP_FILE="$(basename $SOURCEMAP_FILE)"
echo $BN_SOURCEMAP_FILE
```

output:
```
Jane Jane main.jsbundle.map
```

```sh
#!/bin/sh
SOURCEMAP_FILE="ArchiveIntermediates/Jane Doe/IntermediateBuildFilesPath/Jane Doe.build/Release-iphoneos/JaneDoe.build/DerivedSources/main.jsbundle.map"
BN_SOURCEMAP_FILE="$(basename "$SOURCEMAP_FILE")"
echo $BN_SOURCEMAP_FILE
```

output:
```
main.jsbundle.map
```

## Changelog:
[iOS] [Fixed] - Fix support for SOURCEMAP_FILE path containing spaces

## Related
#40937

Pull Request resolved: #42220

Reviewed By: christophpurrer

Differential Revision: D52650491

Pulled By: arushikesarwani94

fbshipit-source-id: e42b8a0d018b37fb558abd53d765fbdd676c51a2
gokul1099 pushed a commit to gokul1099/react-native-visionos that referenced this pull request Jan 17, 2024
)

Summary:
Properly handle targets with spaces in their names

Use quotes around the argument to basename path. Ensures this succeeds when the Xcode target name has spaces.

example:
```sh
#!/bin/sh
SOURCEMAP_FILE="ArchiveIntermediates/Jane Doe/IntermediateBuildFilesPath/Jane Doe.build/Release-iphoneos/JaneDoe.build/DerivedSources/main.jsbundle.map"
BN_SOURCEMAP_FILE="$(basename $SOURCEMAP_FILE)"
echo $BN_SOURCEMAP_FILE
```

output:
```
Jane Jane main.jsbundle.map
```

```sh
#!/bin/sh
SOURCEMAP_FILE="ArchiveIntermediates/Jane Doe/IntermediateBuildFilesPath/Jane Doe.build/Release-iphoneos/JaneDoe.build/DerivedSources/main.jsbundle.map"
BN_SOURCEMAP_FILE="$(basename "$SOURCEMAP_FILE")"
echo $BN_SOURCEMAP_FILE
```

output:
```
main.jsbundle.map
```

## Changelog:
[iOS] [Fixed] - Fix support for SOURCEMAP_FILE path containing spaces

## Related
facebook#40937

Pull Request resolved: facebook#42220

Reviewed By: christophpurrer

Differential Revision: D52650491

Pulled By: arushikesarwani94

fbshipit-source-id: e42b8a0d018b37fb558abd53d765fbdd676c51a2
blakef pushed a commit that referenced this pull request Jan 25, 2024
Summary:
Properly handle targets with spaces in their names

Use quotes around the argument to basename path. Ensures this succeeds when the Xcode target name has spaces.

example:
```sh
#!/bin/sh
SOURCEMAP_FILE="ArchiveIntermediates/Jane Doe/IntermediateBuildFilesPath/Jane Doe.build/Release-iphoneos/JaneDoe.build/DerivedSources/main.jsbundle.map"
BN_SOURCEMAP_FILE="$(basename $SOURCEMAP_FILE)"
echo $BN_SOURCEMAP_FILE
```

output:
```
Jane Jane main.jsbundle.map
```

```sh
#!/bin/sh
SOURCEMAP_FILE="ArchiveIntermediates/Jane Doe/IntermediateBuildFilesPath/Jane Doe.build/Release-iphoneos/JaneDoe.build/DerivedSources/main.jsbundle.map"
BN_SOURCEMAP_FILE="$(basename "$SOURCEMAP_FILE")"
echo $BN_SOURCEMAP_FILE
```

output:
```
main.jsbundle.map
```

## Changelog:
[iOS] [Fixed] - Fix support for SOURCEMAP_FILE path containing spaces

## Related
#40937

Pull Request resolved: #42220

Reviewed By: christophpurrer

Differential Revision: D52650491

Pulled By: arushikesarwani94

fbshipit-source-id: e42b8a0d018b37fb558abd53d765fbdd676c51a2
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.

7 participants