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

Add support for ~/nvm/default-packages file #1463

Merged
merged 1 commit into from Apr 28, 2017

Conversation

Projects
None yet
5 participants
@westonganger
Copy link
Contributor

commented Mar 28, 2017

This PR adds support for a file containing default packages to be installed with every new version install.

While the reinstall-packages option is great, this file based approach is nice because I can check this file into my personal dotfiles repo.

Note: this idea was adapted from https://github.com/rbenv/rbenv-default-gems. I wanted a similar thing from my node version manager setup.

@ljharb
Copy link
Collaborator

left a comment

Thanks for the contribution! This is an interesting idea. I also like the file-based approach.

I'm concerned about adding the complexity of this feature to an already complex install process.

What about npm linked items, which nvm reinstall-packages installs?

I do see in your example how a version range might be specified; what happens if an item is invalid? What happens if an item has spaces in it (which could cause npm to treat it as multiple arguments)?

What about packages where the package is only compatible with a certain node version range, or, where the version number that is compatible depends on the node version? For example, I wouldn't want to install eslint in a node version older than 4.

This would also need unit tests to be able to be merged.

nvm.sh Outdated
DEFAULT_PACKAGES="${DEFAULT_PACKAGES}${line[0]} "
done < "${NVM_DIR}/default-packages"

if [ "$DEFAULT_PACKAGES" != "" ]; then

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 29, 2017

Collaborator

this should probably use [ -z "${DEFAULT_PACKAGES}" ]

nvm.sh Outdated
while IFS=" " read -r -a line; do

# Skip empty lines.
[ "${#line[@]}" -gt 0 ] || continue

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 29, 2017

Collaborator

would [ -z "${line[@]}" ] work?

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

I agree we should not go about adding a bunch of unnecessary complexity with this. But from what I have added so far is not very complicated.

For the lines with spaces in them, that should be invalid I assumed... We can easily provide an explanatory error for this but honestly this is something that is along the lines of garbage in, garbage out. The user should simply write a correct default-packages file.

For the node version compatibility with packages specified. Adding this compatibility check, in my opinion, is adding too much complexity to be worth it unless you know of a dead simple way. But regardless, the most common use case for a version manager is upgrading to newer versions, not installing really old versions. For these cases we should provide a --skip-default-packages option or alternatively they can simply comment out the offending package from their file before performing an install of an old incompatible version.

I will add the unit tests if you plan on merging this eventually.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2017

  • I added the changes you requested.
  • I have also now added the --skip-default-packages option.
  • If default-packages contains the space in a line you mentioned before or if the default packages install fails then it will now provide an error to the user telling them to check their default-packages files for errors.
  • I started adding tests but got a little confused as I havent done this testing before. So its still failings the newly added tests right now. Maybe if someone could help me out a little with the tests.
@ljharb
Copy link
Collaborator

left a comment

Thanks! Sorry for the delay.

  • The doctoc check is failing because when editing the readme, you'll need to run npm run doctoc to update the table of contents.
  • shellcheck is failing because you're using arrays, and nvm is implemented in POSIX, not bash/zsh - no arrays are available, sadly.
  • The other failures seem like they might be legitimate; but probably easiest to clear up the others first and rerunning, before trying to debug.
If you have a list of default packages you want installed every time you install a new version we support that too. You can add any valid package, version, repo, url for installing an npm package.

```sh
# ~/.nvm/default-packages

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 7, 2017

Collaborator

let's do $NVM_DIR/default-packages here, just to try to avoid hard-coding the default

@@ -216,6 +216,19 @@ nvm install 6 --reinstall-packages-from=5
nvm install v4.2 --reinstall-packages-from=iojs
```

### Default global packages from file while installing

If you have a list of default packages you want installed every time you install a new version we support that too. You can add any valid package, version, repo, url for installing an npm package.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 7, 2017

Collaborator

"for installing an npm package - anything npm would accept as a package argument on the command line."?

nvm.sh Outdated
PROVIDED_REINSTALL_PACKAGES_FROM="$(nvm_echo "$1" | command cut -c 27-)"
REINSTALL_PACKAGES_FROM="$(nvm_version "$PROVIDED_REINSTALL_PACKAGES_FROM")" ||:
;;
--copy-packages-from=*)

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 7, 2017

Collaborator

i think adding these lines means we can remove the ones on lines ~2467?

# commented-package
webpack/webpack

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 7, 2017

Collaborator

let's pick some smaller packages for the tests (than webpack or gulp) and still-relevant ones (unlike bower) - how about rimraf, object-inspect, things like that?

cleanup

cat > ../../../default-packages << EOF
not-a-package-name

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 7, 2017

Collaborator

this could be a valid package name - maybe not~a~package~name or something?

not-a-package-name
EOF

OUTPUT="$(nvm exec 6.10.1 npm list -g | grep gulp)"

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 7, 2017

Collaborator

npm ls -g can take a --depth=0 argument which should help the test be faster and more accurate

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2017

I have added the changes you requested.

By the way, Is there a way I can run only the unit tests or only a specific unit test?

.gitignore Outdated
@@ -11,6 +11,7 @@ test/**/test_output

node_modules/
npm-debug.log
.yarn-lock

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 9, 2017

Collaborator

yarn.lock should be gitignored; but I'm not sure what .yarn-lock is nor why it would occur in a project not using yarn?

This comment has been minimized.

Copy link
@westonganger

westonganger Apr 9, 2017

Author Contributor

Just because I installed the devDependencies using yarn its faster, I thought about this already before adding it to this file. Its just a .gitignore file, let it ignore.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 12, 2017

Collaborator

Project-level gitignores should only contain project-related ignores; anything else should be in a global gitignore - otherwise you get every IDE's dotfiles polluting every repo.

If you wanted to ignore yarn.lock I'd understand, but .yarn-lock isn't a file that exists, as far as I'm aware.

nvm.sh Outdated
@@ -2419,13 +2421,51 @@ nvm() {
PROVIDED_REINSTALL_PACKAGES_FROM="$(nvm_echo "$1" | command cut -c 22-)"
REINSTALL_PACKAGES_FROM="$(nvm_version "$PROVIDED_REINSTALL_PACKAGES_FROM")" ||:
;;
--skip-default-packages)
SKIP_DEFAULT_PACKAGES=true ||:

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 9, 2017

Collaborator

||: shouldn't be required here, since you're not doing anything that could possibly have a nonzero exit code.

nvm.sh Outdated
*)
ADDITIONAL_PARAMETERS="$ADDITIONAL_PARAMETERS $1"
;;
esac
shift
done

if [ -z "$SKIP_DEFAULT_PACKAGES" ] && [ -f "${NVM_DIR}/default-packages" ]; then
local DEFAULT_PACKAGES=""

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 9, 2017

Collaborator

please always declare local vars in a separate line from initializing them; this line as-is won't work in ksh which doesn't support local.

nvm.sh Outdated
{
nvm_echo "$DEFAULT_PACKAGES" | command xargs npm install -g --quiet
} || {
nvm_echo "Failed installing default packages. Please check if your default-packages file or a package in it has problems!"

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 9, 2017

Collaborator

this is error output; it should use nvm_err instead of nvm_echo


die () { echo "$@" ; cleanup ; exit 1; }
cleanup () {
rm -rf "$(nvm_version_path v6.10.1)" ../../../default-packages

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 9, 2017

Collaborator

tests are often run inside a working nvm installation; if I have a pre-existing default-packages file, this test will clobber it.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2017

@westonganger typically i just cd into the directory and ./test_file to run a single one

nvm.sh Outdated
[ -z "${line}" ] || continue

# Skip comment lines that begin with `#`.
[ "${line:0:1}" != "#" ] || continue

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 9, 2017

Collaborator

the shellcheck job failed; it says string indexing isn't supported in dash.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2017

That makes testing much faster. Thanks!

I have now updated this with changes you requested.

@ljharb

ljharb approved these changes Apr 12, 2017

Copy link
Collaborator

left a comment

Thanks, this looks great!

There's some minor changes I can make on my own as I merge this.

.gitignore Outdated
@@ -11,6 +11,7 @@ test/**/test_output

node_modules/
npm-debug.log
.yarn-lock

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 12, 2017

Collaborator

Project-level gitignores should only contain project-related ignores; anything else should be in a global gitignore - otherwise you get every IDE's dotfiles polluting every repo.

If you wanted to ignore yarn.lock I'd understand, but .yarn-lock isn't a file that exists, as far as I'm aware.

gulp
webpack
bower@1.7.1

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 12, 2017

Collaborator

let's avoid using bower as an example here too (i can update this myself prior to merging tho, not a blocker)

nvm.sh Outdated
*)
ADDITIONAL_PARAMETERS="$ADDITIONAL_PARAMETERS $1"
;;
esac
shift
done

if [ -z "$SKIP_DEFAULT_PACKAGES" ] && [ -f "${NVM_DIR}/default-packages" ]; then
local DEFAULT_PACKAGES

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 12, 2017

Collaborator

i'll remove this extra blank line also

# commented-package
stevemao/left-pad

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 12, 2017

Collaborator

cute

.gitignore Outdated
@@ -11,6 +11,7 @@ test/**/test_output

node_modules/
npm-debug.log
.yarn-lock

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 12, 2017

Collaborator

we should probably ignore default-packages as well, so contributors don't accidentally commit theirs.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2017

Although, tests are still failing - so this will need to wait until they can be made to pass.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2017

I added the changes you have mentioned and continued working on the tests.

I am kind of stuck. Getting the following error: xargs: npm: No such file or directory

nvm.sh Outdated
if [ -z "$DEFAULT_PACKAGES" ]; then
nvm_echo "Installing default global packages from ${NVM_DIR}/default-packages..."
{
nvm_echo "$DEFAULT_PACKAGES" | command xargs npm install -g --quiet

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 13, 2017

Collaborator

ah - you're trying to npm install before nvm use is ran on line 2488.

I think you should extract this block of code to a separate function; and then call that function on line 2495.5.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2017

I moved it to another function. Hopefully its what you meant.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Apr 13, 2017

That looks great; there's other seemingly unrelated tests failing, presumably because they do nvm install.

Have you tested locally with set -e, to ensure that your new code doesn't cause nvm to exit out when there's no default packages, or also when the list installs properly?

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2017

I have now done all the local testing and came up with some glaring holes, oops... I patched that all up, enhanced and fixed the tests. It now works like a charm under all conditions. I think this one is ready to go!

nvm.sh Outdated
@@ -2441,9 +2471,12 @@ nvm() {
FLAVOR="$(nvm_node_prefix)"
fi

if nvm_is_version_installed "$VERSION"; then
if nvm_is_version_installed "$VERSION" && nvm use "$VERSION"; then
nvm_err "$VERSION is already installed."

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 13, 2017

Collaborator

it's very intentional that the error message prints only if nvm_is_version_installed fails, and not if nvm use fails.

This comment has been minimized.

Copy link
@westonganger

westonganger Apr 13, 2017

Author Contributor

Good call was unsure about that one

nvm.sh Outdated
nvm_echo "$1" | command xargs npm install -g --quiet && return 0
} || {
nvm_err "Failed installing default packages. Please check if your default-packages file or a package in it has problems!"
return 1

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 13, 2017

Collaborator

maybe this should capture the exit code from npm install?

This comment has been minimized.

Copy link
@westonganger

westonganger Apr 13, 2017

Author Contributor

Ya this definately had weird style. I have updated it to be more standard.

@ljharb
Copy link
Collaborator

left a comment

the "fast" tests are still failing; oddly, they're nvm_get_checksum_alg and nvm_get_mirror which are simple unit tests.

nvm.sh Outdated
@@ -2511,6 +2546,9 @@ nvm() {
else
nvm_ensure_default_set "$provided_version"
fi
if [ -z "$SKIP_DEFAULT_PACKAGES" ] && [ -n "$DEFAULT_PACKAGES" ]; then
nvm_install_default_packages "$DEFAULT_PACKAGES"
fi
if [ ! -z "$REINSTALL_PACKAGES_FROM" ] \

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 13, 2017

Collaborator

not sure why i used ! -z here, but this could become -n too :-)

This comment has been minimized.

Copy link
@westonganger

westonganger Apr 13, 2017

Author Contributor

lol. I looked at that but was like shit it aint broken, better not touch it

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2017

I removed set -ex from those failing tests and they passed. However another different errored test popped up for bash only I can try to do the same for that test too.

Would you merge a different PR which removes all set -ex (or maybe just the x option) from the tests so that we dont have such a large output to sift through when viewing the test logs.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Apr 13, 2017

That means the set -e is likely what caused the tests to fail; that's a critical part of the tests. the set -x is just there so when there is a failure, it's easier to debug it. Please do restore those.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2017

Alright these are back. I dont know what to do about those tests nothing should have changed regarding those tests.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2017

Can this be merged now?

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Apr 25, 2017

@westonganger I'm not sure what to do about the tests either; the implication is that your changes have affected those code paths.

I can't merge it unless tests are passing.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2017

So both of these tests run successfully on my local machine, why do they fail on the build?

@ljharb ljharb referenced this pull request Apr 26, 2017

Closed

Post install hook #1342

@roberttod

This comment has been minimized.

Copy link

commented Apr 26, 2017

This looks really good! It would be great to manage the packages using nvm rather than editing the file, perhaps after this gets accepted.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2017

@westonganger so one of the failures is because your new function nvm_install_default_packages isn't being cleared by nvm unload. I've fixed that.

@ljharb

ljharb approved these changes Apr 28, 2017

@ljharb ljharb merged commit 7f3145b into nvm-sh:master Apr 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2017

Thanks for the contribution, and for bearing with the review!

@mightyiam
Copy link

left a comment

This is great. Thank you!

@augbog

This comment has been minimized.

Copy link

commented May 10, 2017

Thank you for adding this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.