Remove hash npm uses

[nscarlson]

Proposing removing the use of hash npm which breaks in some environments.

My understanding is that the meteor executable does not exist within this layer of the build process, therefore if hash npm evaluates to something truthy then meteor npm fails.

if hash npm 2>/dev/null; then
	npm_cmd='npm'
else
	npm_cmd='meteor npm'
fi

Is there any use case in which meteor npm install does need to run in this build layer? If not I'm assuming it's safe to remove the conditional logic based on hash npm and run npm install instead.

[GeoffreyBooth]

Is there any use case in which meteor npm install does need to run in this build layer? If not I’m assuming it’s safe to remove the conditional logic based on hash npm and run npm install instead.

meteor npm uses the verson of npm that came with Meteor’s private version of Node, so it may be a different version than the globally-installed npm. Most of the time this shouldn’t matter; npm install generally runs the same way across npm versions. One case where I can imagine it making a difference is when you have a package-lock.json file that’s using lockfile version 1 (npm < 7) versus version 2 (npm >= 7). However the conditional logic in question prefers the global npm, not Meteor’s version, I guess either by mistake or because when I wrote that code I presumed most people install dependencies via npm rather than meteor npm.

Proposing removing the use of hash npm which breaks in some environments.

This script runs inside a Docker container based on geoffreybooth/meteor-base—as in, it’s always in a known environment: a Docker image built on this repo’s base image. What other environments would this run in?

[nscarlson]

Proposing removing the use of hash npm which breaks in some environments.

This script runs inside a Docker container based on geoffreybooth/meteor-base—as in, it’s always in a known environment: a Docker image built on this repo’s base image. What other environments would this run in?

Completely agreed that given that it's always run built on the same base image, it should deterministically produce the same result. However for unknown reasons this failed consistently in my CircleCI environment exactly the same way as #96 documents, yet would build successfully on my Intel-based Mac. I'll hunt down specific version numbers tomorrow and provide build logs, and if @red-meadow and @sverrirsig would also be so kind to provide additional details that would be super.

[GeoffreyBooth]

Completely agreed that given that it's always run built on the same base image, it should deterministically produce the same result.

The changes seem fine to me, but I'd like to fully understand the issue (ideally with steps to reproduce) before merging anything in. Maybe there's a situation where npm doesn't resolve, and this PR doesn't fix that case. It feels curious.

[red-meadow]

The following steps are required to reproduce the issue in my case:

1. Clone the example folder to the server (can be a --bare app as well).
2. cd example ; docker build .:

Sending build context to Docker daemon  59.39kB
Step 1/14 : FROM geoffreybooth/meteor-base:2.5.6
 ---> cf5d6cabfe17
Step 2/14 : COPY ./app/package*.json $APP_SOURCE_FOLDER/
 ---> 581771d57a0c
Step 3/14 : RUN bash $SCRIPTS_FOLDER/build-app-npm-dependencies.sh
 ---> Running in f45188eb2a84

[-] Installing app NPM dependencies...

added 9 packages in 1.743s
Removing intermediate container f45188eb2a84
 ---> c7964443018a
Step 4/14 : COPY ./app $APP_SOURCE_FOLDER/
 ---> 2ccbcf5d8780
Step 5/14 : RUN bash $SCRIPTS_FOLDER/build-meteor-bundle.sh
 ---> Running in c6cc64a03b81

[-] Building Meteor application bundle...

    This container can use 32143M memory in total.
    If it aborts with an out-of-memory (OOM) or ‘non-zero exit code 137’ error message,
    please increase the container’s available memory.

    See https://github.com/meteor/meteor/issues/9568 for details.


Even with METEOR_ALLOW_SUPERUSER or --allow-superuser, permissions in your app
directory will be incorrect if you ever attempt to perform any Meteor tasks as
a normal user. If you need to fix your permissions, run the following command
from the root of your project:

  sudo chown -Rh <username> .meteor/local

Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db

Why you should do it regularly:
https://github.com/browserslist/browserslist#browsers-data-updating
Removing intermediate container c6cc64a03b81
 ---> 099ffb0be5c4
Step 6/14 : FROM node:14.18.1-alpine
 ---> f7229193551e
Step 7/14 : ENV APP_BUNDLE_FOLDER /opt/bundle
 ---> Using cache
 ---> 624e79326222
Step 8/14 : ENV SCRIPTS_FOLDER /docker
 ---> Using cache
 ---> b41285c1562b
Step 9/14 : RUN apk --no-cache add              bash            ca-certificates
 ---> Using cache
 ---> 401bc6852322
Step 10/14 : COPY --from=0 $SCRIPTS_FOLDER $SCRIPTS_FOLDER/
 ---> 43ac3dee8be9
Step 11/14 : COPY --from=0 $APP_BUNDLE_FOLDER/bundle $APP_BUNDLE_FOLDER/bundle/
 ---> 968551d6e4fa
Step 12/14 : RUN bash $SCRIPTS_FOLDER/build-meteor-npm-dependencies.sh
 ---> Running in 8eb5e14518fd

[-] Installing Meteor application server NPM dependencies...

/docker/build-meteor-npm-dependencies.sh: line 14: meteor: command not found
The command '/bin/sh -c bash $SCRIPTS_FOLDER/build-meteor-npm-dependencies.sh' returned a non-zero code: 127

uname output:

3.16.0-4-amd64 #1 SMP Debian 3.16.36-1+deb8u2 (2016-10-19) x86_64 GNU/Linux

Previously I used Meteor@2.2 and everything worked flawlessly.
Let me know if you need any additional information.

[GeoffreyBooth]

On my local Mac, docker build . is successful for current main:

~/Sites/meteor-base/example » docker build .
[+] Building 115.4s (17/17) FINISHED
 => [internal] load build definition from Dockerfile                                                                                                                                                                                                   0.0s
 => => transferring dockerfile: 1.26kB                                                                                                                                                                                                                 0.0s
 => [internal] load .dockerignore                                                                                                                                                                                                                      0.0s
 => => transferring context: 158B                                                                                                                                                                                                                      0.0s
 => [internal] load metadata for docker.io/library/node:14.18.1-alpine                                                                                                                                                                                 1.3s
 => [internal] load metadata for docker.io/geoffreybooth/meteor-base:2.5.6                                                                                                                                                                             0.0s
 => [auth] library/node:pull token for registry-1.docker.io                                                                                                                                                                                            0.0s
 => CACHED [stage-0 1/5] FROM docker.io/geoffreybooth/meteor-base:2.5.6                                                                                                                                                                                0.0s
 => [internal] load build context                                                                                                                                                                                                                      0.0s
 => => transferring context: 32.04kB                                                                                                                                                                                                                   0.0s
 => [stage-1 1/5] FROM docker.io/library/node:14.18.1-alpine@sha256:240e1e6ef6dfba3bb70d6e88cca6cbb0b5a6f3a2b4496ed7edc5474e8ed594bd                                                                                                                   0.0s
 => [stage-0 2/5] COPY ./app/package*.json /opt/src/                                                                                                                                                                                                   0.0s
 => [stage-0 3/5] RUN bash /docker/build-app-npm-dependencies.sh                                                                                                                                                                                       3.1s
 => [stage-0 4/5] COPY ./app /opt/src/                                                                                                                                                                                                                 0.0s
 => [stage-0 5/5] RUN bash /docker/build-meteor-bundle.sh                                                                                                                                                                                             93.8s
 => CACHED [stage-1 2/5] RUN apk --no-cache add   bash   ca-certificates                                                                                                                                                                               0.0s
 => CACHED [stage-1 3/5] COPY --from=0 /docker /docker/                                                                                                                                                                                                0.0s
 => [stage-1 4/5] COPY --from=0 /opt/bundle/bundle /opt/bundle/bundle/                                                                                                                                                                                 2.2s
 => [stage-1 5/5] RUN bash /docker/build-meteor-npm-dependencies.sh                                                                                                                                                                                    6.2s
 => exporting to image                                                                                                                                                                                                                                 2.2s
 => => exporting layers                                                                                                                                                                                                                                2.2s
 => => writing image sha256:3d9ee96b738f329eb73333166a30651c37acb1ac5b497be4e5afbf22f329d205                                                                                                                                                           0.0s

Use 'docker scan' to run Snyk tests against images to find vulnerabilities and learn how to fix them

It also works as part of the CI in this repo: https://github.com/disney/meteor-base/runs/4954930645?check_suite_focus=true

Perhaps you could clarify which environments fail and how?

[red-meadow]

Perhaps you could clarify which environments fail and how?

Could you explain what specific information is needed?

[GeoffreyBooth]

Perhaps you could clarify which environments fail and how?

Could you explain what specific information is needed?

A way for me to reproduce the failure. It works on my machine and on GitHub CI.

[blacelle]

We also faced this issue.

I consider the command if hash npm 2>/dev/null; then echo 'npm_ok'; else echo 'npm_KO'; fi.

• On my mac: npm_ok: fine, npm is installed globally
• on circleci host: npm_ko: fine, npm is not available out of Docker

Then I run the failing image:

docker run --rm -it c85b96adb936 sh: npm_ok: fine
docker run --rm -it c85b96adb936 bash: npm_KO: ARG. The script is indeed executed in bash #!/bin/bash

More commands regarding the environment:

bash-5.1# if hash npm 2>/dev/null; then echo 'npm_ok'; else echo 'npm_ko'; fi
npm_ko
bash-5.1# hash
hash: hash table empty
bash-5.1# hash npm
bash: hash: npm: not found
bash-5.1# npm

Usage: npm <command>

...

npm@6.14.15 /usr/local/lib/node_modules/npm
bash-5.1# hash
hits	command
   1	/usr/local/bin/npm
bash-5.1# hash npm
bash: hash: npm: not found
bash-5.1# hash /usr/local/bin/npm
hash /usr/local/bin/npm

For any reason, in bash, npm is not registered within hash.

---

In our case, it may relates to the update to

FROM node:14.18.2-alpine

bash --version
GNU bash, version 5.1.16(1)-release (x86_64-alpine-linux-musl)

Relates to https://github.com/bminor/bash/blob/master/CHANGES#L7760?

aaa. The shell no longer puts directory names into the command hash table.

---

The same protocol on my MacOS does not reproduce the issue, while bash is in the same version:

gerador docker run --rm -it ab57ee0f7caa bash
bash-5.1# bash --version
GNU bash, version 5.1.16(1)-release (x86_64-alpine-linux-musl)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
bash-5.1# hash
hits	command
   1	/bin/bash
bash-5.1# hash npm

[GeoffreyBooth]

In our case, it may relates to the update to

FROM node:14.18.2-alpine

Or more specifically, the RUN apk --no-cache add bash in the app Dockerfile. Perhaps I and GitHub CI had a cached layer for this step, with an older version of Bash; whereas when you run it without that cached layer, you get the very newest version of Bash which has the issue?

I deleted all cached images from my local machine and rebuilt everything, and I still don’t get the error. I tried to reproduce it in a simpler form:

» docker run -it node:14.18.2-alpine sh
# apk --no-cache add bash
fetch https://dl-cdn.alpinelinux.org/alpine/v3.15/main/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/v3.15/community/x86_64/APKINDEX.tar.gz
(1/4) Installing ncurses-terminfo-base (6.3_p20211120-r0)
(2/4) Installing ncurses-libs (6.3_p20211120-r0)
(3/4) Installing readline (8.1.1-r0)
(4/4) Installing bash (5.1.16-r0)
Executing bash-5.1.16-r0.post-install
Executing busybox-1.34.1-r3.trigger
OK: 10 MiB in 20 packages
# bash
bash-5.1# if hash npm 2>/dev/null; then echo 'npm_ok'; else echo 'npm_ko'; fi
npm_ok
bash-5.1# hash npm; echo $?
0

So it would appear that hash npm does find npm, and returns an exit code of 0. This is why the image and script works on my machine. The question is, why does hash npm return a nonzero exit code for you?

In the shell script in question, a nonzero exit code for hash npm would cause the npm command to become meteor npm; but because this is the app Docker image, not the Meteor image used in the previous stage, the meteor command doesn’t exist here and we’d get meteor: command not found; or exactly the issue posted in #96. So there’s no reason to keep the current code as it is, and I’ll merge in this PR. But it still doesn’t get to the root of the issue, which is why hash npm behaves differently in certain environments, that I can’t seem to reproduce in any environments I have access to.

[blacelle]

The bash issue has been reported to bash-bug.

https://lists.gnu.org/archive/html/bug-bash/2022-02/msg00179.html

[blacelle]

Here the conclusions from CircleCI support team:

---

Hello Benoit

Sorry for the misunderstanding about the workaround.

We have spoken with the team about this and the issue underlying issue with the OS being used.

The reason for this issue was a change with musl libc in Alpine since v3.14.

When using a version before 3.14 such as 3.13 it works, but when using 3.14 it fails by default if a version is not explicitly defined it uses the latest alpine 3.15

docker run -it node:14.18.2-alpine sh
/ # cat /etc/os-release
NAME="Alpine Linux"
ID=alpine
VERSION_ID=3.15.0
PRETTY_NAME="Alpine Linux v3.15"
HOME_URL="https://alpinelinux.org/"
BUG_REPORT_URL="https://bugs.alpinelinux.org/"
We have also found that if you use the latest available version of remote docker 20.10.11 the issue will no longer occur.

The available versions of remote docker that are available can be found here: https://circleci.com/docs/2.0/building-docker-images/#docker-version

  - setup_remote_docker:
      version: 20.10.11

If you have any questions please feel free to ask.

Kind Regards

Owen Oliver - Support Engineer @ CircleCI