Skip to content

Bug: Sentry Module Collection Script Fails with Spaces in Node Path #4559

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

Merged
merged 6 commits into from
Feb 20, 2025

Conversation

avarayr
Copy link
Contributor

@avarayr avarayr commented Feb 18, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

⚠️ The bug resulted in failing local iOS builds if the user's node points to a folder with spaces in it

e.g.

$ which node

/Users/user/Library/Application Support/fnm/node-versions/v22.1.0/installation/bin/node
#                             ^^^ notice the space

Fixed the Sentry module collection script failing when Node binary path contains spaces (common when using node version managers like fnm on macOS where paths contain "Application Support").

Changed node binary existence check from type/-f to command -v, which properly handles paths with spaces and is more reliable for executable detection.

- type $nodePath >/dev/null 2>&1 || {
+ if ! command -v "$nodePath" >/dev/null 2>&1; then

# notice the nodePath being put in double quotes

💡 Motivation and Context

The script was failing during iOS builds when using node version managers like fnm due to spaces in the Node binary path (e.g., /Users/username/Library/Application Support/fnm/...). This affected developers using node version managers on macOS.

💚 How did you test it?

Tested on macOS with:

  • Node installed via fnm (path containing spaces)
  • Default system Node installation
  • Full path to Node binary
  • Both debug and release builds

📝 Checklist

  • [] I added tests to verify changes
  • [] No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

N/A - This is a self-contained fix for the node binary path detection issue.

may fix the following issues:
expo/eas-cli#1751

@avarayr
Copy link
Contributor Author

avarayr commented Feb 18, 2025

Also, The exit 0 did not successfully prevent the error from stopping the build.
I'm not sure why exactly, could be explained by expo/eas-cli#1751 (comment)

@lucas-zimerman
Copy link
Collaborator

lucas-zimerman commented Feb 18, 2025

Hi, thank you for your contribution!
could you change the changelog.md with the diff shown below?

## Unreleased

### Features

 - Adds Sentry Android Gradle Plugin as an experimental Expo plugin feature ([#4440](https://github.com/getsentry/sentry-react-native/pull/4440))

  To enable the plugin add the `enableAndroidGradlePlugin` in the `@sentry/react-native/expo` of the Expo application configuration.

  "plugins": [
    [
      "@sentry/react-native/expo",
      {
        "experimental_android": {
          "enableAndroidGradlePlugin": true,
        }
      }
    ],

  To learn more about the available configuration options visit [the documentation](https://docs.sentry.io/platforms/react-native/manual-setup/expo/expo-sagp/).

### Fixes

+- Sentry Module Collection Script Fails with Spaces in Node Path on iOS ([#4559](https://github.com/getsentry/sentry-react-native/pull/4559))
 - Various crashes and issues of Session Replay on Android. See the Android SDK version bump for more details. ([#4529](https://github.com/getsentry/sentry-react-native/pull/4529))

@lucas-zimerman
Copy link
Collaborator

I see no issues with the PR!, once the changelog is altered we could merge this PR 😊

@avarayr
Copy link
Contributor Author

avarayr commented Feb 18, 2025

Hi, thank you for your contribution! could you change the changelog.md with the diff shown below?

## Unreleased

### Features

 - Adds Sentry Android Gradle Plugin as an experimental Expo plugin feature ([#4440](https://github.com/getsentry/sentry-react-native/pull/4440))

  To enable the plugin add the `enableAndroidGradlePlugin` in the `@sentry/react-native/expo` of the Expo application configuration.

  "plugins": [
    [
      "@sentry/react-native/expo",
      {
        "experimental_android": {
          "enableAndroidGradlePlugin": true,
        }
      }
    ],

  To learn more about the available configuration options visit [the documentation](https://docs.sentry.io/platforms/react-native/manual-setup/expo/expo-sagp/).

### Fixes

+- Sentry Module Collection Script Fails with Spaces in Node Path on iOS ([#4559](https://github.com/getsentry/sentry-react-native/pull/4559))
 - Various crashes and issues of Session Replay on Android. See the Android SDK version bump for more details. ([#4529](https://github.com/getsentry/sentry-react-native/pull/4529))

Added, thanks!

Do you happen to have any insight why exiting with a zero status code (exit 0) still resulted in a failed build?

This is bound to happen again if the node is not found in the PATH, which is rare but it's a valid concern.

Build command

$ eas build -p ios --local 

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 🚀

Thank you for the fix.

@krystofwoldrich
Copy link
Member

Hi @avarayr,

Xcode exits with non-zero code also in the case that the std contained log with prefix error:. (More details in #3887)

I opened this PR to fix it.

#4570

@krystofwoldrich krystofwoldrich enabled auto-merge (squash) February 20, 2025 08:54
@krystofwoldrich krystofwoldrich merged commit 1e28462 into getsentry:main Feb 20, 2025
59 of 60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants