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

Generalize node search logic #30906

Closed
wants to merge 5 commits into from
Closed

Generalize node search logic #30906

wants to merge 5 commits into from

Conversation

pontusab
Copy link

@pontusab pontusab commented Feb 7, 2021

Summary

This PR fixes the node path issue in current RC 0.64.0-rc.3 react-native-community/releases#214 (comment)

Basically extracting the logic to find the correct node path and reusing it in the generate-specs.sh file.

Changelog

[Internal] [Fixed] - Fix node path

Test Plan

Build the app in Xcode and it should build successfully using with and without nvm.

@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 Feb 7, 2021
@analysis-bot
Copy link

analysis-bot commented Feb 7, 2021

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

Base commit: f9a7d3a

@pull-bot
Copy link

pull-bot commented Feb 24, 2021

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 815d664

@analysis-bot
Copy link

analysis-bot commented Feb 24, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,896,944 -6,293
android hermes armeabi-v7a 8,389,344 -11,072
android hermes x86 9,385,451 -6,600
android hermes x86_64 9,328,141 -8,206
android jsc arm64-v8a 10,631,045 -6,214
android jsc armeabi-v7a 10,106,506 -11,027
android jsc x86 10,681,049 -6,532
android jsc x86_64 11,263,994 -8,136

Base commit: f9a7d3a

@EdwardDrapkin
Copy link

Why not just run which node?

@ivanmoskalev
Copy link
Contributor

@EdwardDrapkin This pull request fixes issues that occurred when Node was installed with nvm.

@EdwardDrapkin
Copy link

Hard-coding a specific fix for nvm won't solve issues for other more esoteric options, e.g. nvs or graalvm. Why not just run which node at some point when node is available and export that as a variable? A solution with hardcoded support for three of the dozens of options isn't really generalized and there's got to be a solution that is.

@ivanmoskalev
Copy link
Contributor

ivanmoskalev commented Feb 25, 2021

@EdwardDrapkin The which node approach has been tried originally, and it didn't really work. You can read the thread that is linked to this PR, if you like.

In a nutshell: generate-specs.sh and react-native-xcode.sh are run as part of an Xcode build step using /bin/sh, which doesn't use your_favourite_shell's profile. That's why you have to call the likes of $HOME/.nvm/nvm.sh manually in the script.

It is easily reproducible:

$ /bin/sh
sh-3.2$ which node # Nothing.

I agree that there should be a better solution for Xcode integrations, and that current Xcode-related scripts feel rather hacky. But I personally don't see any better solution just yet. You'd have to do some form of the lookup dance anyway.

grabbou added a commit that referenced this pull request Mar 1, 2021
@grabbou
Copy link
Contributor

grabbou commented Mar 1, 2021

Note: make sure to add find-node.sh to package.json "files" section.

@pontusab
Copy link
Author

pontusab commented Mar 2, 2021

Note: make sure to add find-node.sh to package.json "files" section.

Fixed!

@pontusab
Copy link
Author

Out in RN 0.64!

@pontusab pontusab closed this Mar 15, 2021
@kelset
Copy link
Collaborator

kelset commented Mar 15, 2021

@pontusab I don't think the PR should be closed, unless I'm mistaken @grabbou manually ported this to the 0.64 branch but this means that main doesn't have the fix yet - reason why we should keep this open and merge it asap :)

@pontusab
Copy link
Author

Ah gotcha!

@geraintwhite
Copy link
Contributor

It would be worth adding unset npm_config_prefix to the top of find-node.sh to fix this issue

# LICENSE file in the root directory of this source tree.

set -e

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixes react-native-community/upgrade-support#138 (comment)

Suggested change
unset npm_config_prefix

@eugenehp
Copy link

@pontusab
This node-find.sh pulls old version of node from NVM, even if there's a newer version of node available in the system. In my case via n.

@pontusab
Copy link
Author

pontusab commented Apr 9, 2021

@hramos Hey I notice this is now on main added by you, should we close this then?

@airtonix
Copy link

So why aren't we just doing :

case $SHELL
  zsh) source ~/.zshrc
  bash) source ~/.bashrc
  fish) source ~/.fishrc 
esac

or something like that.

@sota000 sota000 added this to Needs Triage in React Native Pull Requests via automation Jul 27, 2021
@sota000
Copy link
Contributor

sota000 commented Aug 31, 2021

As far as I can tell, these changes have been merged. Closing the PR. Thanks for the contribution!

@pontusab pontusab closed this Sep 1, 2021
React Native Pull Requests automation moved this from Needs Triage to Closed Sep 1, 2021
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.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet