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

Fixes Global Node Prefix Error #31740

Closed
wants to merge 1 commit into from
Closed

Fixes Global Node Prefix Error #31740

wants to merge 1 commit into from

Conversation

redreceipt
Copy link
Contributor

Summary

Some users have node installed globally which sets a PREFIX by default (so it knows where to put pkgs). We are looking for the "right" node on the next step anyway and if the user is using nvm, PREFIX breaks it.

closes #31181

Changelog

[iOS] [Fixed] - Ignores global npm prefix

Test Plan

yarn ios now works, even if there's a usr/local/bin/npm

Screen Shot 2021-06-17 at 10 14 08 AM

@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 Jun 17, 2021
@redreceipt redreceipt mentioned this pull request Jun 17, 2021
Merged
@analysis-bot
Copy link

analysis-bot commented Jun 17, 2021 •
edited
Loading

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

Base commit: d3c7e20

@analysis-bot
Copy link

analysis-bot commented Jun 17, 2021 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,706,017 +0
android hermes armeabi-v7a 7,236,134 +0
android hermes x86 8,126,071 +0
android hermes x86_64 8,091,037 +0
android jsc arm64-v8a 9,625,917 +0
android jsc armeabi-v7a 8,543,561 +0
android jsc x86 9,640,254 +0
android jsc x86_64 10,248,905 +0

Base commit: d3c7e20

@redreceipt redreceipt mentioned this pull request Jul 2, 2021
Closed
@spsaucier-bakkt
Copy link

This was the only fix to get the newer RN versions building for me after multiple days of troubleshooting - highly recommended.

@Saad-Bashar
Copy link

Same here, worked for me.

@charlesbdudley charlesbdudley self-assigned this Sep 20, 2021
@charlesbdudley
Copy link
Contributor

This makes sense to me. Can we update the comment to be a little more descriptive and less specific to nvm (taking into consideration other version managers)?

@redreceipt
Copy link
Contributor Author

This makes sense to me. Can we update the comment to be a little more descriptive and less specific to nvm (taking into consideration other version managers)?

I'm not sure what to add. You want me to take out nvm from the PR description?

@charlesbdudley
Copy link
Contributor

Ah, I meant the comment in the code. Something along the lines of why we're unsetting PREFIX for future readers.

redreceipt
redreceipt commented Sep 20, 2021
View reviewed changes
scripts/generate-specs.sh Outdated Show resolved Hide resolved
@redreceipt
Copy link
Contributor Author

Gotcha! I did the best I could with my limited understanding of Node 😂 let me know if that's confusing. I'll fix the conflicts as well.

@charlesbdudley
Copy link
Contributor

That looks great. Could you please rebase on main and resolve these conflicts and I'll get it imported. Thanks!

@redreceipt
Copy link
Contributor Author

redreceipt commented Sep 20, 2021 •
edited
Loading

ok @charlesbdudley looks like the original script was removed from main so I made that update. And I'm not sure what that @pull-bot comment means but hopefully when the CI passes, we're good to go

@charlesbdudley
Copy link
Contributor

charlesbdudley commented Sep 23, 2021 •
edited
Loading

I think something may have gone awry related to the repo switching the default branch from master to main since the time this PR was originally opened. You might be able to fix this by rebasing on top of main. For example, I'm not sure why scripts/generate-specs.sh is marked as deleted in your PR when it doesn't exist in main.

@redreceipt
Copy link
Contributor Author

Shoot. Ok, I'll figure it out. I made this PR with the Github GUI originally so I need to figure out how to rebase (new for me) on top of a new remote from a fork (also new for me) 😬

@redreceipt
Copy link
Contributor Author

ok @charlesbdudley I think I got it

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

@redreceipt
Copy link
Contributor Author

Did this get merged in? I can't tell what happened?

@charlesbdudley
Copy link
Contributor

Did this get merged in? I can't tell what happened?

Good call. Looks like @facebook-github-bot didn't mark this appropriately but it was merged into main here: 6334ac3

@charlesbdudley charlesbdudley added the Merged This PR has been merged. label Oct 1, 2021
@redreceipt redreceipt deleted the patch-1 branch October 1, 2021 18:20
danilobuerger pushed a commit to feastr/react-native that referenced this pull request Nov 13, 2021
@redreceipt @danilobuerger
Fixes Global Node Prefix Error (facebook#31740)
e0b9991 
Summary:
Some users have `node` installed globally which sets a `PREFIX` by default (so it knows where to put pkgs). We are looking for the "right" node on the next step anyway and if the user is using `nvm`, `PREFIX` breaks it.

closes facebook#31181

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Ignores global npm prefix

Pull Request resolved: facebook#31740

Test Plan:
`yarn ios` now works, even if there's a `usr/local/bin/npm`

<img width="493" alt="Screen Shot 2021-06-17 at 10 14 08 AM" src="https://user-images.githubusercontent.com/2659478/122413946-c2f57200-cf54-11eb-817c-bd3c07ac50bf.png">

Reviewed By: yungsters

Differential Revision: D31237363

Pulled By: charlesbdudley

fbshipit-source-id: 4ee9c04f8b8ab4e815bafbe2d02e589d621577b4
danilobuerger pushed a commit to feastr/react-native that referenced this pull request Nov 19, 2021
@redreceipt @danilobuerger
Fixes Global Node Prefix Error (facebook#31740)
4f936c7 
Summary:
Some users have `node` installed globally which sets a `PREFIX` by default (so it knows where to put pkgs). We are looking for the "right" node on the next step anyway and if the user is using `nvm`, `PREFIX` breaks it.

closes facebook#31181

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Ignores global npm prefix

Pull Request resolved: facebook#31740

Test Plan:
`yarn ios` now works, even if there's a `usr/local/bin/npm`

<img width="493" alt="Screen Shot 2021-06-17 at 10 14 08 AM" src="https://user-images.githubusercontent.com/2659478/122413946-c2f57200-cf54-11eb-817c-bd3c07ac50bf.png">

Reviewed By: yungsters

Differential Revision: D31237363

Pulled By: charlesbdudley

fbshipit-source-id: 4ee9c04f8b8ab4e815bafbe2d02e589d621577b4
danilobuerger pushed a commit to feastr/react-native that referenced this pull request Jan 31, 2022
@redreceipt @danilobuerger
Fixes Global Node Prefix Error (facebook#31740)
91d31b3 
Summary:
Some users have `node` installed globally which sets a `PREFIX` by default (so it knows where to put pkgs). We are looking for the "right" node on the next step anyway and if the user is using `nvm`, `PREFIX` breaks it.

closes facebook#31181

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Ignores global npm prefix

Pull Request resolved: facebook#31740

Test Plan:
`yarn ios` now works, even if there's a `usr/local/bin/npm`

<img width="493" alt="Screen Shot 2021-06-17 at 10 14 08 AM" src="https://user-images.githubusercontent.com/2659478/122413946-c2f57200-cf54-11eb-817c-bd3c07ac50bf.png">

Reviewed By: yungsters

Differential Revision: D31237363

Pulled By: charlesbdudley

fbshipit-source-id: 4ee9c04f8b8ab4e815bafbe2d02e589d621577b4
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. Merged This PR has been merged.
Projects
None yet
Milestone
No milestone
6 participants
@redreceipt @analysis-bot @spsaucier-bakkt @Saad-Bashar @charlesbdudley @facebook-github-bot