Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Node.js installation in the CI script, switching from apt-get on Linux to a direct tarball download from nodejs.org. This is a positive change that increases control and consistency. The macOS installation logic is also cleaned up. I've added a few suggestions to further improve the script's robustness and ensure deterministic builds across platforms.
| NODE_VERSION="14" | ||
| nvm install $NODE_VERSION |
There was a problem hiding this comment.
To ensure deterministic and reproducible builds on macOS, it's best to install a specific version of Node.js, similar to how it's done for Linux (14.21.3). Using nvm install 14 will install the latest version in the v14 line, which can change over time and potentially introduce unexpected issues.
| NODE_VERSION="14" | |
| nvm install $NODE_VERSION | |
| NODE_VERSION="14.21.3" | |
| nvm install $NODE_VERSION |
ci/env/install-dependencies.sh
Outdated
| sudo apt-get install -y nodejs | ||
| return | ||
| fi | ||
| if [[ ! -n "${BUILDKITE-}" ]] ; then |
There was a problem hiding this comment.
| if [[ "$(uname -m)" == "x86_64" ]]; then | ||
| NODE_URL="https://nodejs.org/dist/v${NODE_VERSION_FULL}/node-v${NODE_VERSION_FULL}-linux-x64.tar.xz" | ||
| NODE_SHA256="05c08a107c50572ab39ce9e8663a2a2d696b5d262d5bd6f98d84b997ce932d9a" | ||
| else # aarch64 | ||
| NODE_URL="https://nodejs.org/dist/v${NODE_VERSION_FULL}/node-v${NODE_VERSION_FULL}-linux-arm64.tar.xz" | ||
| NODE_SHA256="f06642bfcf0b8cc50231624629bec58b183954641b638e38ed6f94cd39e8a6ef" | ||
| fi |
There was a problem hiding this comment.
The current logic assumes that any non-x86_64 architecture is aarch64. To make the script more robust and prevent it from downloading the wrong binary on other architectures, it's better to add an explicit check for aarch64 and fail with an error message for any other unsupported architecture.
| if [[ "$(uname -m)" == "x86_64" ]]; then | |
| NODE_URL="https://nodejs.org/dist/v${NODE_VERSION_FULL}/node-v${NODE_VERSION_FULL}-linux-x64.tar.xz" | |
| NODE_SHA256="05c08a107c50572ab39ce9e8663a2a2d696b5d262d5bd6f98d84b997ce932d9a" | |
| else # aarch64 | |
| NODE_URL="https://nodejs.org/dist/v${NODE_VERSION_FULL}/node-v${NODE_VERSION_FULL}-linux-arm64.tar.xz" | |
| NODE_SHA256="f06642bfcf0b8cc50231624629bec58b183954641b638e38ed6f94cd39e8a6ef" | |
| fi | |
| if [[ "$(uname -m)" == "x86_64" ]]; then | |
| NODE_URL="https://nodejs.org/dist/v${NODE_VERSION_FULL}/node-v${NODE_VERSION_FULL}-linux-x64.tar.xz" | |
| NODE_SHA256="05c08a107c50572ab39ce9e8663a2a2d696b5d262d5bd6f98d84b997ce932d9a" | |
| elif [[ "$(uname -m)" == "aarch64" ]]; then | |
| NODE_URL="https://nodejs.org/dist/v${NODE_VERSION_FULL}/node-v${NODE_VERSION_FULL}-linux-arm64.tar.xz" | |
| NODE_SHA256="f06642bfcf0b8cc50231624629bec58b183954641b638e38ed6f94cd39e8a6ef" | |
| else | |
| echo "Unsupported architecture for nodejs download: $(uname -m)" >&2 | |
| exit 1 | |
| fi |
| curl -fsSL "${NODE_URL}" -o /tmp/node.tar.xz | ||
| echo "$NODE_SHA256 /tmp/node.tar.xz" | sha256sum -c - | ||
| sudo mkdir -p "$NODE_DIR" | ||
| sudo tar -xf /tmp/node.tar.xz -C "$NODE_DIR" --strip-components=1 | ||
| rm /tmp/node.tar.xz |
There was a problem hiding this comment.
Using a hardcoded filename in /tmp can be problematic. It's safer to use mktemp to create a unique temporary file. Additionally, using a trap ensures that the temporary file is cleaned up automatically, even if the script fails midway. This improves the script's robustness.
| curl -fsSL "${NODE_URL}" -o /tmp/node.tar.xz | |
| echo "$NODE_SHA256 /tmp/node.tar.xz" | sha256sum -c - | |
| sudo mkdir -p "$NODE_DIR" | |
| sudo tar -xf /tmp/node.tar.xz -C "$NODE_DIR" --strip-components=1 | |
| rm /tmp/node.tar.xz | |
| local tmpfile | |
| tmpfile=$(mktemp) | |
| trap 'rm -f "${tmpfile}"' RETURN | |
| curl -fsSL "${NODE_URL}" -o "${tmpfile}" | |
| echo "$NODE_SHA256 ${tmpfile}" | sha256sum -c - | |
| sudo mkdir -p "$NODE_DIR" | |
| sudo tar -xf "${tmpfile}" -C "$NODE_DIR" --strip-components=1 |
68afdf0 to
97a194a
Compare
install from tarball from official source, rathar than deb. Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
97a194a to
ab46af4
Compare
|
yeah.. seems that this at least fixed the npm issue.. I am merging this. |
install from tarball from official source, rathar than deb. Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
install from tarball from official source, rathar than deb. Signed-off-by: Lonnie Liu <lonnie@anyscale.com> Signed-off-by: jinbum-kim <jinbum9958@gmail.com>
install from tarball from official source, rathar than deb. Signed-off-by: Lonnie Liu <lonnie@anyscale.com> Signed-off-by: 400Ping <jiekaichang@apache.org>
install from tarball from official source, rathar than deb. Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
install from tarball from official source, rathar than deb.