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

handle nvm and .nvmrc #182

Merged
merged 1 commit into from
Sep 7, 2020
Merged

Conversation

andreineculau
Copy link
Contributor

  • Fixes:
  • Breaking change: [ ]

nvm https://github.com/nvm-sh/nvm is one node version manager. Two notable alternatives are https://github.com/tj/n and https://github.com/isaacs/nave . In the future, it would be worthwhile to look into n as it has no intricate shell setup as nvm and nave have, and it is also more active than nvm.

This PR adds support for nvm in 2 ways:

  • make calls will automatically have the PATH correctly set in order to hit the node version specified in the project's .nvmrc file (root of the repository only). See changes in core.common.mk
  • CI and developer environments, which will source exe-env.inc.sh, will also have nvm enabled, primarily for convenience, for those cases where node is called directly e.g. ng build, and not via make.

Note that the projects that depend on nvm would need to add source ${SUPPORT_FIRECLOUD_DIR}/ci/brew-install-node.nvm.inc.sh to their Brewfile.inc.sh - which will install both nvm (via homebrew) but also directly call nvm install <node version specified in .nvmrc file>

WARNING: the nvm-get-nvm-bin script is not super-super-fast, but still acceptable (~1s), and that each and every call to make would imply also one call to nvm-get-nvm-bin. Thus there's one (1) second added to each make call inside a project with a .nvmrc file.

PS: this PR is a port of functionality from https://github.com/rokmoln/support-firecloud . I have not tested the ported functionality.

cc @spschlegel @tobiiasl

@weetmuts
Copy link
Contributor

weetmuts commented Sep 2, 2020

Nice! But have you considered fetching the nvm script without using brew?

It seems like installing nvm is quite simple:
curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.35.3/install.sh | bash

Currently, to enhance stability of the linux build servers, when
OS is linux and FORCE_BREW is not set to true, then
install-core.inc.sh and similar files will use long term stable apt
packages and avoid brew, which changes quickly and thus
causes stability issues on our build servers.

When OS is not linux, or if FORCE_BREW=true then
the install scripts will use brew as before.

In this case, perhaps you could use the curl|bash combo
when OS=linux and FORCE_BREW!=true ?
and use brew in all other cases?

If so, then you could perhaps name the script
install-node.nvm.inc.sh ?

@andreineculau
Copy link
Contributor Author

@weetmuts 👋

A. on-topic

I can do that if you insist. No problem.
It will further increase the divergence mentioned in #174, but that's fine by me.


B. off-topic

The question that you put forward is about stability though, and there's "nothing" unstable about brew as to the highly active "formulas" bringing in the latest versions.

Having said that, one can install via brew the exact same nvm formula as it is today, and the exact same [email protected] with brew install https://raw.githubusercontent.com/Homebrew/homebrew-core/d31923bc16ee2577dfc5d699c5afcba4fddf1b06/Formula/nvm.rb # v0.35.3 instead of brew install nvm.

Even better, since we have docker images of ubuntu, bootstrapped with a (aka stable) version of brew, setting HOMEBREW_NO_AUTO_UPDATE=1 would mean that brew install nvm in such a docker image, would always install the same things, including whatever dependencies nvm might have (it doesn't have any, but if it did brew install https://.../nvm.rb would only pin the nvm version, not the version of its dependencies).

❗ NOTE: there's at least one bug in support-firecloud related to what I wrote above, namely that in a sf container, already bootstrapped with brew, instead of just skipping the "installation" of brew, we actually call brew update (see brew-bootstrap.inc.sh) which updates not only the brew executable, but also all the formulas, making the system "unstable" even if we actually do set HOMEBREW_NO_AUTO_UPDATE=1. Should be highlighted that similarly calls to apt-get update should be inhibited, even though the apt repositories are by virtue more stable (they simply get updated less often). A simple bugfix to remove brew update calls in brew-bootstrap.inc.sh would have similar outcomes as moving installs from brew to aptitude. Some other outcomes (like speed improvements 👏 ) can be achieved while maintaining or at least trying to maintain brew as the package manager (and not have flags like FORCE_BREW).

curl | bash is a 3rd way to install things, beyond brew and aptitude. Granted, sometimes you need to go special ways, jump hoops, git clone, make, etc. It shouldn't be the default way to go when the goal is to promote a canonical style, should it?

Worth mentioning also that doing strict versioning makes it hard/impossible to bootstrap a developer machine. If nvm 0.35.3 has a security bug, and I already updated to 0.36.3 - I will go back to 0.35.3 unintentionally whenever I run make bootstrap in a project.

Copy link
Contributor

@spschlegel spschlegel left a comment

Choose a reason for hiding this comment

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

I think I understand the changes and they look good to me :)

@weetmuts
Copy link
Contributor

weetmuts commented Sep 7, 2020

Thanks Andrei! I now understand the purpose of nvm and I approve of it being merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants