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 a bug that block that installation of node in install.sh #1676

Merged
merged 3 commits into from
Dec 3, 2017

Conversation

a-magdy
Copy link
Contributor

@a-magdy a-magdy commented Nov 30, 2017

Hi,

I found a bug in nvm_install_node function, it overrides the variable NODE_VERSION, which blocks the installation of node in the install.sh script! More details below..

I did some tests locally with the install.sh script, and the I found that the problem in in the function nvm_install_node in install.sh script.

One solution is to change the name of the local variable to something other than NODE_VERSION

You could test it using these 2 scripts:

I ran tests on ubuntu 16.04 servers

  • Current version [0.33.6] test script:
    export NODE_VERSION=8 && sudo curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.33.6/install.sh | bash

Expected result: nvm should install node 8
Result: node 8 doesn't get installed

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 12492  100 12492    0     0  68946      0 --:--:-- --:--:-- --:--:-- 69016
=> nvm is already installed in /home/ubuntu/.nvm, trying to update using git
=> => Compressing and cleaning up git repository

=> nvm source string already in /home/ubuntu/.bashrc
=> bash_completion source string already in /home/ubuntu/.bashrc
=> Close and reopen your terminal to start using nvm or run the following to use it now:

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"  # This loads nvm
[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion"  # This loads nvm bash_completion
  • Proposed fix test script:
    export NODE_VERSION=8 && sudo curl -o- https://raw.githubusercontent.com/Quadric/nvm/fix-node-version-in-install-script/install.sh | bash

Result: node 8 is installed

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 12540  100 12540    0     0  48598      0 --:--:-- --:--:-- --:--:-- 48604
=> nvm is already installed in /home/ubuntu/.nvm, trying to update using git
=> => Compressing and cleaning up git repository

=> nvm source string already in /home/ubuntu/.bashrc
=> bash_completion source string already in /home/ubuntu/.bashrc
=> Installing Node.js version 8
v8.9.1 is already installed.
Now using node v8.9.1 (npm v5.5.1)
=> Node.js version 8 has been successfully installed
=> Close and reopen your terminal to start using nvm or run the following to use it now:

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"  # This loads nvm
[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion"  # This loads nvm bash_completion

I wrote a test for it, and ran "test/install_script" on original repo and my fix, and surprisingly the test passes on both!! (https://github.com/Quadric/nvm/blob/fix-node-version-in-install-script/test/install_script/nvm_install_with_node_version), it doesn't change the fact that it still fails when I provision an AWS AMI with packer or when I run the script directly on the newly created machine!

thanks for you efforts :) 👍

@ljharb ljharb added bugs Oh no, something's broken :-( installing nvm Problems installing nvm itself labels Dec 3, 2017
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, good catch.

@ljharb ljharb merged commit 8a8dcbb into nvm-sh:master Dec 3, 2017
ljharb added a commit to ljharb/nvm that referenced this pull request Dec 9, 2017
Fixes
 - fix unassigned variable (nvm-sh#1665, nvm-sh#1664)
 - Fix for $path used by zsh (nvm-sh#1669)
 - `set -u`: ensure `NVM_USE_OUTPUT` is always set (nvm-sh#1671)
 - `install.sh`: Fix a bug that block that installation of node in install.sh (nvm-sh#1676)
 - `nvm install-latest-npm`: fix node 4-4.6

Documentation
 - Make `nvm cache clear` message less ambiguous (nvm-sh#1644)
 - Added missing piece (nvm-sh#1658)
edwmurph pushed a commit to edwmurph/nvm that referenced this pull request Apr 9, 2018
…ll-script

`install.sh`: Fix a bug that block that installation of node in install.sh
edwmurph pushed a commit to edwmurph/nvm that referenced this pull request Apr 9, 2018
Fixes
 - fix unassigned variable (nvm-sh#1665, nvm-sh#1664)
 - Fix for $path used by zsh (nvm-sh#1669)
 - `set -u`: ensure `NVM_USE_OUTPUT` is always set (nvm-sh#1671)
 - `install.sh`: Fix a bug that block that installation of node in install.sh (nvm-sh#1676)
 - `nvm install-latest-npm`: fix node 4-4.6

Documentation
 - Make `nvm cache clear` message less ambiguous (nvm-sh#1644)
 - Added missing piece (nvm-sh#1658)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugs Oh no, something's broken :-( installing nvm Problems installing nvm itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants