Improved handling of node installation (& miscellaneous improvements)

[ChanceHarrison]

Note: This PR has changed somewhat significantly since inception. The "very minor" changes listed below have been removed to keep the focus on changes to node/npm install improvements. The original text otherwise follows, but please see the comments for relevant context.

Summary

Very Minor Changes

• Escape quotes that are themselves in a quoted string. Before, these quotes would not print, but it did not cause any errors or other unexpected behavior.
• Moved a comment to (what I think is) the right place

NodeJS Changes

• Remove the nodesource JS installation (in the case of edison boards) from the install script, as it is already done in the openaps-packages script and is not needed until then
• The biggest change of this PR. Instead of Installing nodejs and npm from the package manager, we instead install the node version manager n since in my first attempt to install OpenAPS found the scripts failing due to issues with getting JS setup (particularly with getting npm to a proper version and keeping it there). There may be an easier fix, but n seems like a more robust solution. I chose n over nvm because of issues with nvm in multi-user/root environments. n is an actual binary and does a proper global install of node that should avoid those issues. I changed the setup script to reflect the preference of n over nvm. This change also includes a subtle but important modification to the npm self-update command. It now, instead of updating itself to the latest version of npm, it should update itself to the latest version of npm that is compatible with the installed version of node. Trying to install the latest version of npm with our relatively ancient version(s?) of node does not work.

Copy and pasted code comment that I (re)moved (in a recent commit) but wanted to save for posterity:

# Raspbian Buster has (at the time of writing) npm 5.8 in the repo, which is not compatible with the repo nodejs version of 10.24
# An npm self-upgrade (as is attempted below) is successful (bringing npm to major verison 8), but between there and this script trying to `npm install -g json` npm has somehow reverted to the package manager version and become broken (any invocation of npm immediately crashes with a syntax error)
# At this point, the root cause is unclear and I don't want to deal with it, so I'm deferring to `n`
# `n` instead of `nvm` to avoid issues with nvm not being available to all users (see https://stackoverflow.com/questions/21215059/cant-use-nvm-from-root-or-sudo)

If you would like me to squash/rebase any of my commits or remove some (perhaps excessive) code comments, I'd be happy to do so.

Please let me know your thoughts/suggestions. I look forward to making future contributions.

Testing

I used this modified code to conduct my own install and it worked well for me (a lot better than regular dev at the moment). Some of these changes are non-functional and could likely be merged separately, but I could understand the desire for some testing around the JS changes. It shouldn't change anything for existing users who re-run oref0-setup.sh; their existing version of node is likely fine.

Perhaps contributors that are doing fresh installs (and just updating their rigs) can do so with these changes included?

[scottleibrand]

Was this designed for and tested on a Raspberry Pi rig? I recently re-flashed and reinstalled all my Edison rigs using dev, and that worked fine, so not sure this is needed there. I don't currently have an operational Pi rig, so haven't tested that lately...

[ChanceHarrison]

Was this designed for and tested on a Raspberry Pi rig? I recently re-flashed and reinstalled all my Edison rigs using dev, and that worked fine, so not sure this is needed there. I don't currently have an operational Pi rig, so haven't tested that lately...

Yes, this was designed for and tested on a Pi rig (A Pi Zero W, to be precise). Even if the JS changes are not needed for Edison, my hope and expectation is that these changes provide an equally effective solution that would reduce the need to treat the two platforms differently.

Edit: With that said, I believe there is still some work to be done to make it work ideally for both. @cluckj mentioned in another comment that the latest version available through n on the Edison is 8, so trying to install version 10 fails.

[scottleibrand]

Ok. LMK when y'all have tested it to your satisfaction on the Pi, and I can re-flash and test it on an Edison.

[ChanceHarrison]

I have spent some time thinking about this, and I would appreciate your input on the following:

For our two major architectures, x86 (Edison) and armv6 (Pi 0W), their support was downgraded to "Experimental" as of versions 10 and 12, respectively. So, n by default only provides the following versions:

$ n --arch x86 --latest
9.11.2
$ n --arch armv6 --latest
11.15.0

(On that note, @cluckj can you check and see what the output of n --latest says on an Edison? I'm not sure why it only gave you v8 if x86 has support for v9)

I suppose there is no harm in using these old versions and we can leave it at that, but I do want to throw out an alternative for consideration: Use unofficial builds when they are available (with the old but technically supported versions as a fallback). I believe n can be directed to use a custom source and we would be off to the races with modern Node. Albeit, largely untested and at-your-own-risk Node, which may be antithetical to our goals of building safe and reliable software, but perhaps that would be offset by the value(?) of a modern Node runtime?

Regardless of the decision on that matter, this PR should be ready to move once the following changes are made:

• Updating the call to n to install using the latest (supported version) alias instead of a specific numbered version
• Uninstalling an existing version (nvm, package manager, etc.) of node/npm if installing via n
• Rebasing my commits to clean up the commit history - is this encouraged?

[sharbi]

Hi Chance, unfortunately this build failed on the rpi zero 2. Had exactly the same issues with crashing with each attempt at an npm install. I have the version of npm as 8.1.2 which is failing with node version 10.24.

Global install of latest node via n prior to starting the installation worked. Just wondering about how to do a check for newer hardware to do a check and install most up to date node.

[ChanceHarrison]

Hi @sharbi!

I have the version of npm as 8.1.2 which is failing with node version 10.24.

That version of npm is too new for that version of node. They won't work together.

I would be eager to figure out how that version of npm got installed. I don't think n would install an incompatible version of npm when installing a version of node.

Was this tested on a clean image of Raspbian? If npm and/or node were already installed from package manager or nvm, I would expect them to conflict.

Do you happen to have the output of the script during install?

Just wondering about how to do a check for newer hardware to do a check and install most up to date node.

I haven't pushed the change yet, but I think the best solution is to just do n install current. I don't believe n is capable of installing an unsupported version as it does not build from source, it'll grab the latest compatible version for the platform. That should avoid the need for device checks, but it's always a fallback option.

[sharbi]

Fresh flash of Raspbian caused the same error. Can't quite figure out how it happens. Output is below:

Installing bash completion script /etc/bash_completion.d/python-argcomplete npm does not support Node.js v10.24.0 You should probably upgrade to a newer version of node as we can't make any promises that npm will work with this version. You can find the latest version at https://nodejs.org/ /usr/local/lib/node_modules/npm/lib/npm.js:32 #unloaded = false ^

SyntaxError: Invalid or unexpected token at Module._compile (internal/modules/cjs/loader.js:723:23) at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10) at Module.load (internal/modules/cjs/loader.js:653:32) at tryModuleLoad (internal/modules/cjs/loader.js:593:12) at Function.Module._load (internal/modules/cjs/loader.js:585:3) at Module.require (internal/modules/cjs/loader.js:692:17) at require (internal/modules/cjs/helpers.js:25:18) at module.exports (/usr/local/lib/node_modules/npm/lib/cli.js:22:15) at Object. (/usr/local/lib/node_modules/npm/bin/npm-cli.js:2:25) at Module._compile (internal/modules/cjs/loader.js:778:30) Couldn't install npm json

Checking versions after running the code shows node 10.24 and unable to see npm version due to node being incompatible. Also hasn't globally installed n...so not sure if it's just not going through the if statement. Will have a proper play around later and see if I can figure out what's happening.

[ChanceHarrison]

Hi @sharbi

I think I know why it happens. This line of openaps-install.sh does not download the version of openaps-packages.sh on this PR. It downloads it from $BRANCH (if provided as an argument when invoking the install script) or master. So the changes aren't actually being tested.

My advice: re-test, but instead of piping the install script directly from curl to bash, I would clone the repo, switch to this PR branch (see these GitHub docs for context). The tl;dr is git fetch origin pull/{pull_id}/head:{local_branch_name} where {pull_id} is 1419 for this PR, and {local_branch_name} can be whatever you want; something like pr-1419-local would work. Then you can git switch (or git checkout, if you prefer) to that branch and modify the relevant line:

Old line in openaps-install.sh:

curl -s https://raw.githubusercontent.com/openaps/oref0/$BRANCH/bin/openaps-packages.sh | bash -

New line:

bash {relative or absolute path to openaps-packages.sh}

Once those changes are made, run openaps-install.sh (ideally in a root shell, but it would be interesting to know if there is a pass/fail difference depending on if it is run as root or not) and let's see if it works!

Hopefully it is clear what the issue is now and what changes need to be made locally to test them. Ideally this should be a pain point, but we can definitely work on improving this experience if necessary.

[sharbi]

Hi cool that all makes sense - should have remembered this from last testing I was doing!

Installation works first time, but then fails setup for some other reason (fails with nightscout autoconfigure-device-crud) which feels like a separate issue. Strangely then can't re-run installation.

Again n and npm not globally installed and the node version installed was 8.17.0.

[ChanceHarrison]

fails setup for some other reason (fails with nightscout autoconfigure-device-crud) which feels like a separate issue

Indeed, that does sound like it would be a separate issue, but impossible to say for sure at this point.

Again n and npm not globally installed and the node version installed was 8.17.0.

Both the version and the installation status defy my expectations. In the current iteration, n installs node 10, so I'm not sure how node 8 got installed. Perhaps from package manager?

If you have the full input/output of a clean installation attempt (e.g. no previous installations) using the scripts from this PR branch, I'd really appreciate it if you could put that in a GitHub Gist and link it here.

[sharbi]

Sorry - scrap that. Being a muppet in real time. Solved and now installed with n + node 1.

Setup still not working so will need to start work on that...

Okay finally sorted - needed to run oref0-setup from local download as well, just kept running the standard setup code. Hopefully would be resolved when these files are merged! Sorry for causing all the confusion, seems to be all good now!

[dcacklam]

For buster & bullseye you can just 'apt-get install' it.... Moving towards more recent Linux distro versions would alleviate a lot of these issues (albeit edison is kind-of kernel limited).

[ChanceHarrison]

@dcacklam Perhaps the issue has resolved itself with time, or the issue was in my/our implementation all along, but when I opened this PR, the repository versions of node and npm did not seem to be compatible. Perhaps the wrong packages were being installed or npm was being upgraded to the latest version (which would of course be incompatible with the older version of node that gets installed) (see [edit: ]a7c1cd8), which I think is an important fix that needs to be merged regardless of the fate of the other changes).

Certainly, perhaps I've fallen prey to the trap that is added complexity, but I do appreciate the platform-agnostic solution that comes with using something like n, being almost entirely absolved of concerns re: distro package repository shenanigans. Perhaps it's a short-sighted trade. Perhaps that part can be split into its own PR, as there are other changes here that I believe would be hard to disagree with.

[scottleibrand]

@ChanceHarrison Would you like to refactor this to split out the n stuff from the other improvements so we can merge it to 0.7.2-dev?

[ChanceHarrison]

@scottleibrand Consider it done. Refactored, rebased onto upstream/dev, and history is rewritten to clarify the changes that are being proposed. I don't recommend separating the changes any further, but let's talk if you think otherwise.

I will have to make the time to grab a spare SD card and try installing OpenAPS from scratch again to test my changes once more. When I do so, I'll be sure to write clear-and-concise testing instructions should others want to do the same (at least until the PR is merged into dev).

[SeregaYakovlev]

I fixed npm and node versions conflict during Openaps 0.7.1 installation: openaps/oref0#1434

[ChanceHarrison]

Some time ago, I had the opportunity to test these changes with someone who has an Edision rig who had been attempting to do a clean install of oref0.

I have a few takeaways that I wanted to note here:

• I've shared some git fetch commands previously intended to let users pull this PR to use it/test it that had an un-escaped dollar sign that was then interpreted by the terminal as a variable and resulted in creating a local git branch that starts with a dash, which to my knowledge is not an allowed name (and seems to make it non-intuitive to deal with). The fix was to use git branch -m -- <-old-branch-name> <new-branch-name>, to rename the branch to something proper, where the -- clearly delineates command flags from the branch names. Old hat to anyone who is familiar with Linux, but quite a cruel trap for anyone who isn't in the know.
• Attempting to install oref0 from the current master branch is still failing frequently, at least based on messages in the Gitter intend-to-bolus channel. Based on the number of installation failures (independent of rig type) being reported (makes you wonder how many more instances of failed installs are going unreported...), I hope that we can prioritize getting fixes (such as those included in this PR) merged into at least dev branch.
• It is difficult for users to test PR requests when they aren't merged into a branch because doing so requires first cloning the openaps/oref0 repo manually, fetching and checking out the PR branch (because GitHub lets you fetch PR directly from the receiving repository, which is pretty cool but is not very intuitive), and then finally editing openaps-install.sh as follows before running the install:

Lines 75 has to be changed from: curl -s https://raw.githubusercontent.com/openaps/oref0/$BRANCH/bin/openaps-packages.sh | bash - to: bash [local-path-to-openaps-packages] where the bracket portion is replaced with the literal (absolute or relative if you know what the pwd is when that line runs) path is to openaps-packages.sh. If this change is not made, the downloaded file will not have the changes included in the PR (which is only relevant if the PR made changes to the file, which this one has).

(as an aside, I think it might be valuable to instead only have a single file that needs to be downloaded to bootstrap the install instead of having the bootstrap script download another bootstrap script. I think there is value to having the bootstrap script install Git and clone the repo as first order of business so that everything that runs afterwards can use files that have already been stored locally)

Line 76: mkdir -p ~/src; cd ~/src && ls -d oref0 && (cd oref0 && git checkout $BRANCH && git pull) || git clone https://github.com/openaps/oref0.git can be entirely removed when testing PRs in this manner since the user has already manually cloned the repo and switched to the relevant branch before running the install. If this change is not made and the user does not specify the appropriate (PR) branch when invoking the install, git returns an error saying that checking out $BRANCH would overwrite uncommitted changes. I don't recall if this error causes the script to exit (the script is practically already done by this point anyway).

To re-iterate, I believe the edit to line 76 can be avoided if the appropriate branch name is provided at runtime, but it doesn't completely remove the need (which would be the goal) of manually editing files.

[scottleibrand]

Testing this now via: curl -s https://raw.githubusercontent.com/openaps/oref0/6fc2e3212497964d52f720aefd515e46dd90b9da/bin/openaps-packages.sh| bash -

[scottleibrand]

That didn't work. Errored out with:

ERROR: npm is known not to run on Node.js v8.17.0
You'll need to upgrade to a newer Node.js version in order to use this
version of npm. You can find the latest version at https://nodejs.org/
Couldn't install npm json

[ChanceHarrison]

Testing this now via: curl -s https://raw.githubusercontent.com/openaps/oref0/6fc2e3212497964d52f720aefd515e46dd90b9da/bin/openaps-packages.sh| bash -

@scottleibrand I believe that success is contingent on the state of the system prior to running setup.

If you did https://github.com/openaps/oref0/pull/1434#issuecomment-1242599247 and then ran this, the error is expected.

The quick workaround is to uninstall any existing nodejs/npm.

Reasoning:

You will find that the setup flow with the proposed changes should succeed in the following cases:

1. There is an existing unbroken* installation of nodejs/npm
2. nodejs/npm is not installed

Notably, setup does not adequately detect broken nodejs/npm installations*. I already added some logic to try to uninstall nodejs/npm and install known good versions, but that is only invoked as part of openaps-setup.sh in the check_nodejs_timing function as per the following block.

NODE_EXECUTION_TIME="$(\time --format %e node -e 'true' 2>&1)"
    if [ 1 -eq "$(echo "$NODE_EXECUTION_TIME > 10" |bc)" ]; then
    [...prompt the user to uninstall existing node and install with known good process...]

I didn't make this the default behavior to avoid fixing what isn't broken (messing with people's existing nodejs/npm installs) , but that might be a worthwhile tradeoff to avoid failed setups. Especially considering that running setup from the current dev/master consistently results in broken nodejs/npm installations*.

We can automate the "uninstall nodejs/npm" workaround early in setup to allow for known-good installation to take place. Do you agree?

* By a "broken" nodejs/npm installation, I am mostly referring to the case when the latest version of npm is installed when that version does not support whatever version of nodejs is installed. In such a state, npm is not able to fix itself and either nodejs needs to be updated (which isn't feasible for our target platforms; I don't believe they are receiving official nodejs support since v12-ish at the latest) or npm needs to be uninstalled and reinstalled.

[scottleibrand]

Ok, trying: sudo apt-get remove nodejs curl -s https://raw.githubusercontent.com/openaps/oref0/6fc2e3212497964d52f720aefd515e46dd90b9da/bin/openaps-packages.sh| bash -

[scottleibrand]

That seems to have worked to install node v9.11.2 and 5.10.0, but other stuff I tried with nvm seems to have left node v8.17.0 in place as well. Re-flashing the rig and trying again from scratch with your openaps-packages instead of master's.

[ChanceHarrison]

You can also take a look at https://github.com/nvm-sh/nvm#uninstalling--removal, nvm seems to make it pretty easy to uninstall, mostly just deleting its directory and removing what it added to ~/.bashrc. So, whether you go that route or the from-scratch route, I look forward to hearing how it goes.

[scottleibrand]

This is working a lot better than master, so I'm going to merge it to dev and keep working on getting this rig working. Right now it's erroring out during go get with:

../go/src/golang.org/x/sys/unix/syscall.go:83:7: undefined: unsafe.Slice
../go/src/golang.org/x/sys/unix/syscall_unix.go:118:7: undefined: unsafe.Slice
../go/src/golang.org/x/sys/unix/sysvshm_unix.go:33:7: undefined: unsafe.Slice

That might be an install script issue having to do with updated versions of go, or it might be that this rig's SSD is so far gone it can't keep from corrupting files even during install. I'll probably try another flash and fresh install from dev tomorrow.