-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add --without-snapshot configure flag to ARM devices by default #766
Conversation
@@ -1022,6 +1022,12 @@ nvm_install_node_source() { | |||
local ADDITIONAL_PARAMETERS | |||
ADDITIONAL_PARAMETERS="$2" | |||
|
|||
local NVM_ARCH | |||
NVM_ARCH="$(nvm_get_arch)" | |||
if [[ $NVM_ARCH = *"arm"* ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this double bracket syntax is not portable - nvm
has to work in POSIX, which includes dash
, sh
, ksh
, zsh
, and bash
. Same, I believe, with the +=
below.
Can you please add an installation test for this, that mocks out |
I've updated for POSIX compatibility. |
nvm run $NVM_TEST_VERSION --version | grep $NVM_TEST_VERSION || "'nvm run $NVM_TEST_VERSION --version | grep $NVM_TEST_VERSION' failed" | ||
|
||
# Check V8 snapshot isn't compiled | ||
nvm run $NVM_TEST_VERSION -p process.config | grep "v8_use_snapshot: false" || "'nvm run $NVM_TEST_VERSION -p process.config | grep \"v8_use_snapshot: false\"' failed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I note that when I run this command on my own system, it says v8_use_snapshot: 1
- is there a chance it will sometimes be 0/1, and sometimes true/false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I'll update it to check the falsy value in node rather than trying to scrape it out with grep.
Weird that it's only failing on zsh :-/ |
Even weirder that zsh is my default shell and it runs fine on my machine. Weirder still that the issue seems to be with
Pretty stumped... |
nvm install -s $NVM_TEST_VERSION || die "'nvm install -s $NVM_TEST_VERSION' failed" | ||
|
||
# Use new version | ||
nvm use $NVM_TEST_VERSION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done automatically by nvm install
, so either it should be removed, or a || die
should be added in case this line fails.
Seems to be working fine on zsh now. |
Any plans to merge this in? |
Yes - I'm out of town until tonight, so I'll test and merge it tonight or tomorrow. |
Awesome. |
Sorry for the delay - this looks great, thanks! |
Add --without-snapshot configure flag to ARM devices by default
No problem. |
At the time of this writing, there is a problem with the Google V8 Snapshot feature causing node to segmentation fault on ARM devices. Snapshotting helps node start faster and is not a big-deal feature so it's safe to disable by default for ARM.