Installs Node.js and ends the setup process automatically #549

xcambar · 2014-10-13T12:17:51Z

Many people around me don't read enough of the README and expect Node.js to be installed after running the install script.

Based on a Gist I've written for my coworkers (https://gist.github.com/xcambar/2f88b912b7d40fe605c5), we thought it could be beneficial to NVM that the install script actually installs Node.js.

This is what this PR does, with a configurable Node.js version (defaulting to stable).

xcambar · 2014-10-13T13:30:10Z

Just a note: I looked at the build report and couldn't figure out why it did fail.
The test didn't even really fail but the runner declared that it timed out.

ljharb · 2014-10-13T17:06:52Z

Sometimes the tests time out; I'll rerun that one.

I'm pretty hesitant to add extra features to the (mostly untested) install script just to cover people that don't read the instructions. The install script installs nvm - I'm not sure why you'd have an expectation that it would install anything else?

xcambar · 2014-10-13T17:38:55Z

Well... The whole purpose of NVM is to have Node.js installed, it seems 😉

Anyway, users don't read the instructions, whether they are developers or not... hard truth, but truth nonetheless.

ljharb · 2014-10-13T18:01:19Z

I also don't think we can presume that "stable" is the thing the user wants to install - nor that the user necessarily wants the install script to make a network call or compile something. Think setup for automated test environments, for example.

xcambar · 2014-10-14T08:02:20Z

You are absolutely right. This is a very good example of why the PR makes the installed version customizable via the $NODE_VERSION environment variable.

I imagined it could be used just the same as NVM already does to select the correct test suite in Travis (https://github.com/creationix/nvm/blob/master/.travis.yml#L12-L31).

Maybe the PR could be turned upside down: The default behaviour of NVM being left unchanged but the presence of the NODE_VERSION environment variable would be used as a switch to automatically install the specified version of Node.js.

ljharb · 2014-10-14T08:09:49Z

That's not a bad idea.

xcambar · 2014-10-14T09:57:36Z

Done :)

amessinger · 2014-10-14T10:19:38Z

I kinda agree with @xcambar in that the main purpose of NVM is to get a NodeJS binary up and running and a given environment.
@ljharb I understand your concern that the step Xavier suggests might be out of the scope of this install script, but to me it is not: it's the first - and most important - use case of NVM.

My 2 cents.

ljharb · 2014-10-14T21:02:02Z

Sure, but this is the install script - it's main purpose is to get nvm installed. Separation of concerns is pretty important, considering you can do $INSTALL_COMMAND && . $NVM_DIR/nvm.sh && nvm install stable && nvm alias default stable on your own already.

ljharb · 2014-10-15T04:36:22Z

install.sh

@@ -48,6 +51,17 @@ install_nvm_from_git() {
  return
 }

+install_node() {
+  # Sourcing $PROFILE to take into account the new environment
+  source $PROFILE


Does this need to re-source the entire $PROFILE, or would sourcing $NVM_DIR/nvm.sh be sufficient?

xcambar · 2014-10-15T10:50:42Z

I've followed your comments.
Hope you'll like it.

wilmoore · 2014-10-17T03:06:21Z

install.sh

@@ -126,6 +146,11 @@ else
  else
    echo "=> Source string already in $PROFILE"
  fi
+  # Automatically install Node.js
+  if [ -z "$NODE_VERSION" ]; then
+    echo "=> Close and reopen your terminal to start using nvm"


Is this actually necessary? Doesn't nvm execute hash -r?

Only on "use" or "deactivate" - but what might be helpful here is to actually source nvm, rather than relying on the profile to do it?

...source nvm, rather than relying on the profile to do it?

👍

👍 too, but as it was the former behaviour of the install script, it felt out of scope to remove it without notice.

As we seem to agree on it, I'll add the source.

xcambar · 2014-10-17T07:46:19Z

Done.

Note that the source is done no matter if the snippet has been found in $PROFILE or not.
It covers the edge case where it may have been found in a file that is actually not sourced at login (eg. legacy .bash_profile while the user moved to zsh). Whatsmore, it doesn't hurt.

ljharb · 2014-12-21T22:03:25Z

Please rebase this on top of master - it's currently not mergeable.

xcambar · 2014-12-24T09:30:10Z

Will do in the next few days. Sounds good ?

ljharb · 2016-06-26T23:07:03Z

@xcambar do you still have any interest in completing this PR?

xcambar · 2016-06-27T11:25:12Z

Rebased and well.

ljharb · 2016-06-28T04:08:38Z

README.markdown

@@ -42,8 +42,8 @@ or Wget:

 <sub>The script clones the nvm repository to `~/.nvm` and adds the source line to your profile (`~/.bash_profile`, `~/.zshrc`, `~/.profile`, or `~/.bashrc`).</sub>

-You can customize the install source, directory and profile using the `NVM_SOURCE`, `NVM_DIR`, and `PROFILE` variables.
-Eg: `curl ... | NVM_DIR="path/to/nvm" bash`
+You can customize the install source, directory, profile and version using the `NVM_SOURCE`, `NVM_DIR`, `PROFILE` and `NODE_VERSION` variables.


please add an Oxford comma after "profile" and PROFILE

ljharb · 2016-06-28T04:09:46Z

install.sh

+  local CURRENT_NVM_NODE
+
+  CURRENT_NVM_NODE="$(nvm_version current)"
+  if [ "$(nvm_version $NODE_VERSION)" == "$CURRENT_NVM_NODE" ]; then


NODE_VERSION needs quotes here

ljharb · 2016-06-28T04:11:08Z

LGTM pending the above comments

ljharb · 2016-06-29T06:18:30Z

install.sh

+  fi
+
+  echo "=> Installing Node.js version $NODE_VERSION"
+  nvm install $NODE_VERSION


this var needs quotes

ljharb · 2016-06-29T07:10:25Z

install.sh

@@ -255,9 +286,13 @@ nvm_do_install() {
    fi
  fi

+  # Sourcing $PROFILE to take into account the new environment
+  source $NVM_PROFILE


sorry, one more - this needs to be quoted, and it must use . since source isn't as portable

ljharb · 2016-06-29T08:08:37Z

Thanks!

ljharb added feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions. labels Oct 14, 2014

ljharb reviewed Oct 15, 2014
View reviewed changes

wilmoore reviewed Oct 17, 2014
View reviewed changes

xcambar mentioned this pull request Oct 18, 2014

Testing the install script #552

Open

ljharb force-pushed the master branch from 2721eb7 to a7b1f93 Compare October 20, 2014 22:50

ljharb added the needs followup We need some info or action from whoever filed this issue/PR. label Dec 21, 2014

xcambar force-pushed the complete_install branch from 8aae2c5 to f1af1c1 Compare June 27, 2016 09:59

ljharb reviewed Jun 28, 2016
View reviewed changes

xcambar force-pushed the complete_install branch from f1af1c1 to 35fa736 Compare June 28, 2016 06:58

ljharb reviewed Jun 29, 2016
View reviewed changes

install.sh

fi

echo "=> Installing Node.js version $NODE_VERSION"

nvm install $NODE_VERSION

Copy link

Member

ljharb Jun 29, 2016

Choose a reason for hiding this comment

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

this var needs quotes

xcambar force-pushed the complete_install branch from 35fa736 to f5724e4 Compare June 29, 2016 07:08

ljharb reviewed Jun 29, 2016
View reviewed changes

Optionally installs Node.js if a version is provided

a24ff3e

xcambar force-pushed the complete_install branch from f5724e4 to a24ff3e Compare June 29, 2016 07:57

ljharb removed the needs followup We need some info or action from whoever filed this issue/PR. label Jun 29, 2016

ljharb merged commit a24ff3e into nvm-sh:master Jun 29, 2016

xcambar deleted the complete_install branch June 29, 2016 08:15

ljharb mentioned this pull request Apr 4, 2018

Error: Global NVM install error "Please report this!" #1773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Installs Node.js and ends the setup process automatically #549

Installs Node.js and ends the setup process automatically #549

xcambar commented Oct 13, 2014

xcambar commented Oct 13, 2014

ljharb commented Oct 13, 2014

xcambar commented Oct 13, 2014

ljharb commented Oct 13, 2014

xcambar commented Oct 14, 2014

ljharb commented Oct 14, 2014

xcambar commented Oct 14, 2014

amessinger commented Oct 14, 2014

ljharb commented Oct 14, 2014

ljharb Oct 15, 2014

xcambar commented Oct 15, 2014

wilmoore Oct 17, 2014

ljharb Oct 17, 2014

wilmoore Oct 17, 2014

xcambar Oct 17, 2014

xcambar commented Oct 17, 2014

ljharb commented Dec 21, 2014

xcambar commented Dec 24, 2014

ljharb commented Jun 26, 2016

xcambar commented Jun 27, 2016

ljharb Jun 28, 2016

ljharb Jun 28, 2016

ljharb commented Jun 28, 2016

ljharb Jun 29, 2016

ljharb Jun 29, 2016

ljharb commented Jun 29, 2016

Installs Node.js and ends the setup process automatically #549

Installs Node.js and ends the setup process automatically #549

Conversation

xcambar commented Oct 13, 2014

xcambar commented Oct 13, 2014

ljharb commented Oct 13, 2014

xcambar commented Oct 13, 2014

ljharb commented Oct 13, 2014

xcambar commented Oct 14, 2014

ljharb commented Oct 14, 2014

xcambar commented Oct 14, 2014

amessinger commented Oct 14, 2014

ljharb commented Oct 14, 2014

Choose a reason for hiding this comment

xcambar commented Oct 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcambar commented Oct 17, 2014

ljharb commented Dec 21, 2014

xcambar commented Dec 24, 2014

ljharb commented Jun 26, 2016

xcambar commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jun 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jun 29, 2016