Use $HOME variable in NVM_DIR

[DesignByOnyx]

Closes #1346

[ljharb]

could this be done inside nvm_install_dir? that would also make it more testable.

[DesignByOnyx]

I didn't want to affect other functions which use nvm_install_dir - I was intentionally trying to isolate this change to the place that needed it. I'm testing it now.

[ljharb]

That's a fair point. I'd prefer to have this change covered by automated tests, if possible.

[DesignByOnyx]

I had to use printf because the tests didn't like echo:

echo "${NVM_DIR:-"$HOME/.nvm"}" | sed "s:^$HOME:\$HOME:"

It wanted me to use ${expansion/search/replace} instead and I couldn't figure out an elegant way to do it - it was hard to read. printf essentially allowed me to say "shh, I'm doing it this way".

[ljharb]

Thanks, this looks great

[lkysow]

Hey Guys, I think this has introduced a bug:
nvm_install_dir is using the actual string $HOME instead of the environment variable now. This means NVM is being installed into a directory called $HOME.

Example:

# old way
echo "${NVM_DIR:-"$HOME/.nvm"}"
/ubuntu/.nvm

# new way
printf %s "${NVM_DIR:-"$HOME/.nvm"}" | sed "s:^$HOME:\$HOME:"
$HOME/.nvm

[ndcampbell]

We are having the same issue as @lkysow. Looks like this introduced a bug.

Specifically at https://github.com/DesignByOnyx/nvm/blob/1b2305232f878db80ded5450a5accc881d4e8752/install.sh#L81 where it creates the directory

[ljharb]

Thanks. Nobody should be using nvm off of master, only off of tagged releases, so this isn't urgent, but I'll take a look.

[ndcampbell]

The Jenkins NVM wrapper plugin explicitly is using Master at https://github.com/jenkinsci/nvm-wrapper-plugin/blob/f252c5fb55deeaf068ef4a21fc9bafbdc1c2b6f4/src/main/java/org/jenkinsci/plugins/nvm/NvmWrapperUtil.java#L122

That is how we encountered it

[ljharb]

Thanks, I'll file a bug. They shouldn't be doing that.

[ljharb]

Filed jenkinsci/nvm-wrapper-plugin#3 to try to fix that they're depending on an unstable version.

[DesignByOnyx]

@ljharb - my original PR isolated this change to the code which writes to the profile file. I can revert back to that if you like. Still need to fix the Jenkins thing, but the only place we really need literal $HOME is when writing to the profile. Let me know.

[ljharb]

@DesignByOnyx if you'd be willing to open a PR that fixes the issue without reverting the feature, that'd be great!