Use node-build as main installation/building drive #272

augustobmoura · 2021-12-07T03:33:31Z

Heavily inspired by the #221 PR, the idea is to keep an internal installation of node-build and use theirs build/installation system that is far more mature than what we have. Aside from that, is a really good idea to centralize the know-how about building and installing node versions, and node-build seems like a great candidate for that type of project.

In this PR I refactor the install script to use node-build as the main installation/building driver. We manage an internal node-build repository and try to update it before every installation.

We are also adding two additional commands added in this PR for helping manage the local installation:

asdf nodejs update-nodebuild: For updating/cloning the local node-build repository
asdf nodejs nodebuild: A wrapper around the node-build command with the correct environment and configuration for working with asdf-nodejs

The README was heavily refactored to reflect the new building system and its possibilities.

Because node-build already has a built-in checksum offline strategy we don't need to query live keyservers for checking signatures anymore. A major pain point of the current installation process is how flaky and cumbersome is querying current keyservers.

Other asdf projects that uses similar strategies of "outsourcing" the installation process to community tools:

asdf-python: behind the scenes it uses pyenv's python-build
asdf-ruby: uses ruby-build
asdf-elixir: uses bob
asdf-erlang: uses kerl

Closes #241, closes #192, closes #190, closes #189, closes #171, closes #133, closes #128, closes #106, closes #92, closes #78, closes #26

also some minor text adjustments

augustobmoura · 2021-12-07T03:35:40Z

@Stratus3D, can you check this? The README changes in particular. This PR will fix a lot of old problems and compatibility issues.

Stratus3D · 2021-12-09T16:36:29Z

Hey @augustobmoura , sorry the late reply. I will take a look in a few hours.

Stratus3D · 2021-12-13T15:44:29Z

Stratus3D · 2021-12-13T15:49:52Z

README.md


 ```bash
-bash -c '${ASDF_DATA_DIR:=$HOME/.asdf}/plugins/nodejs/bin/import-previous-release-team-keyring'
+asdf nodejs update-nodebuild


nitpick: Is this command really necessary? If this runs before every install command, why would a user need to run it manually?

It's just seemed like a useful command in general, for scripting or just a shortcut before running commands directly on the internal nodebuild command via asdf nodejs nodebuild ...

Why not just update node-build when the user invokes asdf nodejs nodebuild ...?

The presence of this command may confuse users. For example, users may think there is a difference between the following sequences:

$ asdf nodejs update-nodebuild $ asdf install nodejs <version>

$ asdf install nodejs <version>

When in reality they both update the node-build version, and then install a version of nodejs. The only difference being with the first node-build is updated twice.

Why not just update node-build when the user invokes asdf nodejs nodebuild ...?

asdf nodejs nodebuild ... is just a peek at the current node-build installation. I don't think we should forcefully update it on the command invocation

The presence of this command may confuse users.

I agree with you. Maybe we should make it more clear that asdf is already auto-updating node-build on installations and that running asdf nodejs update-nodebuild is optional.

I did some changes to the documentation, check if that is better

README.md

bin/install

lib/commands/command-update-nodebuild.bash

Stratus3D · 2021-12-13T16:13:25Z

Overall code changes look great but I left a couple comments. I have checked out this code locally and will use it for the next several days. I'll report back if I encounter any issues.

Stratus3D · 2021-12-15T14:28:59Z

lib/commands/command-update-nodebuild.bash

+      printf "$(colored $RED ERROR): Pulling the node-build repository exited with code %s\n" "$pull_exit_code" >&2
+      printf "Please check if the git repository at %s doesn't have any changes or anything\nthat might not allow a git pull\n" "$ASDF_NODEJS_NODEBUILD_HOME" >&2
+      exit $pull_exit_code
+    fi


What if the user wants to do a build offline from a previously downloaded nodejs version?

Currently, the user could place the tarball of the source in $ASDF_DATA_DIR/downloads/nodejs/<version> and it should work. We might make this easier in the future, but node-build has some limitations around it

But if the script tries to download node-build first and it fails, the user will not be able to install nodejs even if the archive is already present on their system.

But now I realize this plugin doesn't implement the required bin/download callback (https://asdf-vm.com/plugins/create.html#environment-variables). That's on me, I never got around to adding it here.

But I think it's good to support users who are offline. If a user has an archive already downloaded to .asdf/downloads from a past build, they should be able to install if without internet access, so I think the logic here may need to be re-worked a bit so updating node-build is skipped when the user is offline.

Updating node-build is optional when installing new nodejs versions and will just pop a warning. See the function try_to_update_nodebuild on bin/install.

This file is just the custom command for calling it manually, it's used internally on install but we catch errors accordingly there

Stratus3D

Got an error when running this:

$ asdf install nodejs 17.2.0 
Cloning node-build...
/Users/trevorbrown/.asdf/plugins/nodejs/bin/install: line 52: nodebuild_args[@]: unbound variable

OSX 10.15 with zsh

augustobmoura · 2021-12-15T17:03:55Z

Got an error when running this:

$ asdf install nodejs 17.2.0 
Cloning node-build...
/Users/trevorbrown/.asdf/plugins/nodejs/bin/install: line 52: nodebuild_args[@]: unbound variable

OSX 10.15 with zsh

It's an error on Bash 3, I will fix it

Stratus3D

I'm getting a different error now:

asdf install nodejs 17.2.0
Trying to update node-build... ok
Usage: node-build [-kpv] <definition> <prefix>
       node-build --definitions
       node-build --version

  -c/--compile     Force compilation even if a matching binary exists
  -k/--keep        Do not remove source tree after installation
  -p/--patch       Apply a patch from stdin before building
  -v/--verbose     Verbose mode: print compilation status to stdout
  -4/--ipv4        Resolve names to IPv4 addresses only
  -6/--ipv6        Resolve names to IPv6 addresses only
  --definitions    List all built-in definitions
  --version        Show version of node-build

Stratus3D · 2021-12-16T16:11:02Z

Thanks for all your work on this! Sorry for having requested changes twice.

augustobmoura · 2021-12-16T17:29:11Z

I'm getting a different error now

That's a weird error. I will try to pinpoint it.

Thanks for all your work on this! Sorry for having requested changes twice.

No worries, I don't think we should rush this. The plugin is already working on master. Even though this would be a huge simplification of the plugin, it's not a top priority.

Stratus3D · 2021-12-17T14:34:21Z

looks good!

$ asdf install nodejs 17.2.0
Trying to update node-build... ok
Downloading node-v17.2.0-darwin-x64.tar.gz...
-> https://nodejs.org/dist/v17.2.0/node-v17.2.0-darwin-x64.tar.gz
Installing node-v17.2.0-darwin-x64...
Installed node-v17.2.0-darwin-x64 to /Users/trevorbrown/.asdf/installs/nodejs/17.2.0

Installing lodash npm package...
SUCCESS
Installing request npm package...
SUCCESS
Installing express npm package...
SUCCESS

$ asdf shell nodejs 17.2.0

$ node
Welcome to Node.js v17.2.0.
Type ".help" for more information.
> 
(To exit, press Ctrl+C again or Ctrl+D or type .exit)
>

augustobmoura · 2021-12-19T19:11:24Z

@Stratus3D the comment threads are fine? You think we should merge this?

Stratus3D · 2021-12-20T13:15:06Z

@Stratus3D the comment threads are fine? You think we should merge this?

Yes. I'm happy with things here if you are. If you want get others to review I'm fine with waiting though.

augustobmoura · 2021-12-20T15:58:17Z

If you want get others to review I'm fine with waiting though.

Nah it's okay, I just wanted to check if all questions were answered. I'm merging this

timdp · 2021-12-20T19:16:30Z

Note that this PR introduced an implicit dependency on bc, which isn't always installed. Was that intentional?

It looks like bc is only used to round a fractional number though. That can also be achieved with plain old Bash arithmetic.

Stratus3D · 2021-12-20T19:38:47Z

good point @timdp , I think we should fix this. Can you show us how to do this with plain old Bash? Thanks!

augustobmoura · 2021-12-20T19:41:08Z

Note that this PR introduced an implicit dependency on bc, which isn't always installed. Was that intentional?

bc should be present in every unix-like box. I don't see the need to mark it as a dependency because if we were to mark it, we should also mark other common commands.

It looks like bc is only used to round a fractional number though. That can also be achieved with plain old Bash arithmetic.

I did a bit of research on it, but nothing too deep. I agree that a pure bash solution should be the best, we are open to PRs if you have anything in mind

timdp · 2021-12-20T19:47:50Z

I know for a fact that it's not present on every Linux box. For one thing, our CI servers don't have it installed. That's what drove me to this PR.

Rounding without bc: https://stackoverflow.com/a/2395294

(Sent from my phone, please excuse brevity.)

augustobmoura · 2021-12-20T19:56:25Z

I know for a fact that it's not present on every Linux box. For one thing, our CI servers don't have it installed. That's what drove me to this PR.

When introducing new commands I usually use busybox/alpine containers as the baseline. And they have a basic implementation of bc. But yeah, if you are using a bare or slim container, it might not get all the POSIX commands by default.

Searching a bit more I found that just a $(($ASDF_CONCURRENCY / 2)) should suffice, as bash doesn't implement float numbers it already floors down the division. I will open a PR for it.

augustobmoura · 2021-12-20T20:13:45Z

@timdp the fix should be merged on master. Check if that solves the problem

timdp · 2021-12-20T20:21:42Z

CI seems happy with the new version. Thanks! 👍

The nodejs plugin is now using node-build and no longer needs to verify the sha of the install. See asdf-vm/asdf-nodejs#272 for more information

augustobmoura added 11 commits November 23, 2021 21:23

feat: add commands for managing an internal node-build installation

800e244

feat: modify install to use node-build behind the scenes

c4e335f

feat: remove previous integrity check logic

ee8daa2

docs: update README com new information about installation

67c382b

docs: add information on nodebuild commands

94bd796

fix: add correct default for concurrency

1a70296

typo: change defitions to definitions

1dd98c4

docs: add section for running the wrapped node-build command

07fe573

also some minor text adjustments

chore: add better error messages

e2e0db2

chore: better messages

58859ab

docs: better wording for the README

3e44540

Stratus3D reviewed Dec 13, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Stratus3D reviewed Dec 13, 2021

View reviewed changes

bin/install Outdated Show resolved Hide resolved

Stratus3D reviewed Dec 13, 2021

View reviewed changes

bin/install Outdated Show resolved Hide resolved

Stratus3D reviewed Dec 13, 2021

View reviewed changes

lib/commands/command-update-nodebuild.bash Show resolved Hide resolved

fix: fix typos & revert import of utils.sh

dcc73ae

augustobmoura requested a review from Stratus3D December 14, 2021 01:51

Stratus3D approved these changes Dec 15, 2021

View reviewed changes

Stratus3D reviewed Dec 15, 2021

View reviewed changes

Stratus3D requested changes Dec 15, 2021

View reviewed changes

augustobmoura added 2 commits December 15, 2021 14:07

fix: add fallback for empty arrays in bash3

363c0d1

docs: better explanation of manually updating node-build

e0d3804

augustobmoura requested a review from Stratus3D December 15, 2021 17:22

Stratus3D requested changes Dec 16, 2021

View reviewed changes

fix: add fallback for unbound variables in bash3

b58ae23

augustobmoura requested a review from Stratus3D December 17, 2021 00:11

Stratus3D approved these changes Dec 17, 2021

View reviewed changes

augustobmoura merged commit 6430bd8 into asdf-vm:master Dec 20, 2021

augustobmoura mentioned this pull request Dec 20, 2021

Remove dependency on bc in favor of a pure bash implementation #274

Merged

GeoffreyEmerson mentioned this pull request Dec 23, 2021

Broken build process after recent update #276

Closed

jbeard6 mentioned this pull request Jan 12, 2022

asdf-nodejs plugin removed import-release-team-keyring script thoughtbot/laptop#600

Closed

robjacoby mentioned this pull request Mar 8, 2022

Remove the requirement of gpg signing for nodejs cultureamp/homebrew-web-team-devtools#1

Closed

MetricMike mentioned this pull request Jun 20, 2023

Improve security of downloading the binary by checking the fingerprint MetricMike/asdf-awscli#28

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use node-build as main installation/building drive #272

Use node-build as main installation/building drive #272

augustobmoura commented Dec 7, 2021 •

edited

Loading

augustobmoura commented Dec 7, 2021 •

edited

Loading

Stratus3D commented Dec 9, 2021

Stratus3D commented Dec 13, 2021

Stratus3D Dec 13, 2021

augustobmoura Dec 14, 2021

Stratus3D Dec 15, 2021

Stratus3D Dec 15, 2021

augustobmoura Dec 15, 2021

augustobmoura Dec 15, 2021

Stratus3D commented Dec 13, 2021

Stratus3D Dec 15, 2021

augustobmoura Dec 15, 2021

Stratus3D Dec 16, 2021

augustobmoura Dec 16, 2021

Stratus3D left a comment

augustobmoura commented Dec 15, 2021

Stratus3D left a comment

Stratus3D commented Dec 16, 2021

augustobmoura commented Dec 16, 2021

Stratus3D commented Dec 17, 2021

augustobmoura commented Dec 19, 2021

Stratus3D commented Dec 20, 2021

augustobmoura commented Dec 20, 2021

timdp commented Dec 20, 2021

Stratus3D commented Dec 20, 2021

augustobmoura commented Dec 20, 2021

timdp commented Dec 20, 2021

augustobmoura commented Dec 20, 2021 •

edited

Loading

augustobmoura commented Dec 20, 2021

timdp commented Dec 20, 2021

Use node-build as main installation/building drive #272

Use node-build as main installation/building drive #272

Conversation

augustobmoura commented Dec 7, 2021 • edited Loading

augustobmoura commented Dec 7, 2021 • edited Loading

Stratus3D commented Dec 9, 2021

Stratus3D commented Dec 13, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stratus3D commented Dec 13, 2021

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

Stratus3D left a comment

Choose a reason for hiding this comment

augustobmoura commented Dec 15, 2021

Stratus3D left a comment

Choose a reason for hiding this comment

Stratus3D commented Dec 16, 2021

augustobmoura commented Dec 16, 2021

Stratus3D commented Dec 17, 2021

augustobmoura commented Dec 19, 2021

Stratus3D commented Dec 20, 2021

augustobmoura commented Dec 20, 2021

timdp commented Dec 20, 2021

Stratus3D commented Dec 20, 2021

augustobmoura commented Dec 20, 2021

timdp commented Dec 20, 2021

augustobmoura commented Dec 20, 2021 • edited Loading

augustobmoura commented Dec 20, 2021

timdp commented Dec 20, 2021

augustobmoura commented Dec 7, 2021 •

edited

Loading

augustobmoura commented Dec 7, 2021 •

edited

Loading

augustobmoura commented Dec 20, 2021 •

edited

Loading