Skip to content

fix(cli): retry + cache prebuild downloads in build-dist; add docker test #4051

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.

Sign up for GitHub

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

Merged
saddlepaddle merged 1 commit into from
May 4, 2026
Merged

fix(cli): retry + cache prebuild downloads in build-dist; add docker test #4051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions packages/cli/scripts/build-dist-linux-docker.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#!/usr/bin/env bash
#
# Reproduce the GitHub Actions Linux CLI build inside a Docker container.
# Mirrors `.github/workflows/build-cli.yml` so we can validate the full
# install + build + smoke-test flow without cutting a release.
#
# Usage:
# packages/cli/scripts/build-dist-linux-docker.sh [linux-x64|linux-arm64]
#
# Outputs the tarball at packages/cli/dist/superset-<target>.tar.gz inside
# the container's copy of the repo and runs the same require() smoke test
# the CI workflow runs.
set -euo pipefail

TARGET="${1:-linux-x64}"
case "$TARGET" in
linux-x64) PLATFORM="linux/amd64"; NODE_ARCH="x64" ;;
linux-arm64) PLATFORM="linux/arm64"; NODE_ARCH="arm64" ;;
*) echo "Usage: $0 [linux-x64|linux-arm64]" >&2; exit 1 ;;
esac

REPO_ROOT="$(cd "$(dirname "$0")/../../.." && pwd)"
BUN_VERSION="$(cat "$REPO_ROOT/.bun-version")"
NODE_VERSION="22.22.2"

echo "[docker-build] target=$TARGET platform=$PLATFORM bun=$BUN_VERSION node=$NODE_VERSION"
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 4, 2026

Choose a reason for hiding this comment

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

P2 NODE_VERSION differs from the bundled runtime

The Docker script hardcodes NODE_VERSION="22.22.2" for the container's build tooling, while build-dist.ts defines NODE_VERSION = "22.13.0" as the Node runtime bundled into the distribution. These intentionally serve different roles, but they're both Node 22.x (ABI 127) so native modules compiled against 22.22.2 in the container will still be compatible with the bundled 22.13.0. However, if the CI workflow's build tool Node version ever diverges from 22.x (e.g., bumped to v24), the ABI assumption breaks and the smoke test would pass locally while CI fails. Consider extracting the build-tool Node version from .nvmrc / build-cli.yml or at least adding a comment linking the two constants.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/cli/scripts/build-dist-linux-docker.sh
Line: 26

Comment:
**`NODE_VERSION` differs from the bundled runtime**

The Docker script hardcodes `NODE_VERSION="22.22.2"` for the container's build tooling, while `build-dist.ts` defines `NODE_VERSION = "22.13.0"` as the Node runtime bundled into the distribution. These intentionally serve different roles, but they're both Node 22.x (ABI 127) so native modules compiled against 22.22.2 in the container will still be compatible with the bundled 22.13.0. However, if the CI workflow's *build tool* Node version ever diverges from 22.x (e.g., bumped to v24), the ABI assumption breaks and the smoke test would pass locally while CI fails. Consider extracting the build-tool Node version from `.nvmrc` / `build-cli.yml` or at least adding a comment linking the two constants.

How can I resolve this? If you propose a fix, please make it concise.

echo "[docker-build] repo: $REPO_ROOT"

# Mount the repo read-only and copy it into a writable workdir inside the
# container so the host's darwin-arm64 node_modules don't bleed in. The
# container does its own `bun install` against the lockfile.
docker run --rm --platform "$PLATFORM" \
-v "$REPO_ROOT:/host:ro" \
-e TARGET="$TARGET" \
-e NODE_ARCH="$NODE_ARCH" \
-e NODE_VERSION="$NODE_VERSION" \
-e RELAY_URL="${RELAY_URL:-https://relay.superset.sh}" \
-e SUPERSET_API_URL="${SUPERSET_API_URL:-https://api.superset.sh}" \
-e SUPERSET_WEB_URL="${SUPERSET_WEB_URL:-https://app.superset.sh}" \
"oven/bun:${BUN_VERSION}" bash -euxc '
apt-get update -qq
apt-get install -y --no-install-recommends \
curl python3 make g++ ca-certificates xz-utils rsync >/dev/null

curl -fsSL "https://nodejs.org/dist/v${NODE_VERSION}/node-v${NODE_VERSION}-linux-${NODE_ARCH}.tar.xz" \
| tar -xJ -C /usr/local --strip-components=1
node --version
bun --version

Comment on lines +48 to +49
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 4, 2026

Choose a reason for hiding this comment

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

P2 Single-shot curl | tar in the container setup

The whole motivation for this PR is that a transient 502 from GitHub/nodejs.org kills the pipeline. The Node.js installation step inside the container (curl -fsSL … | tar -xJ) uses bare curl with no retry flags, so a transient failure here still kills the docker run. Since this is a developer-local script (not CI), the impact is low, but it's inconsistent with the fix being applied to build-dist.ts. Adding --retry 3 --retry-delay 2 --retry-all-errors to this curl call would make it consistent.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/cli/scripts/build-dist-linux-docker.sh
Line: 48-49

Comment:
**Single-shot `curl | tar` in the container setup**

The whole motivation for this PR is that a transient 502 from GitHub/nodejs.org kills the pipeline. The Node.js installation step inside the container (`curl -fsSL … | tar -xJ`) uses bare curl with no retry flags, so a transient failure here still kills the docker run. Since this is a developer-local script (not CI), the impact is low, but it's inconsistent with the fix being applied to `build-dist.ts`. Adding `--retry 3 --retry-delay 2 --retry-all-errors` to this curl call would make it consistent.

How can I resolve this? If you propose a fix, please make it concise.

rsync -a --exclude=node_modules --exclude=dist --exclude=.next /host/ /work/
cd /work

# Mirrors `.github/workflows/build-cli.yml` Linux install step.
# Bun occasionally hits transient integrity-check failures on cold caches
# in Docker, retry once before giving up.
bun install --frozen --ignore-scripts || \
(rm -rf ~/.bun/install/cache && bun install --frozen --ignore-scripts)
PTY_DIR=$(ls -d node_modules/.bun/node-pty@*/node_modules/node-pty)
(cd "$PTY_DIR" && npx --yes node-gyp rebuild)
Comment on lines +58 to +59
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 4, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

ls -d glob for node-pty directory is fragile with multiple-match results.

If the Bun store contains more than one node-pty@* entry (e.g., after a version bump without a clean install), ls returns multiple lines, PTY_DIR becomes a multi-line string, and cd "$PTY_DIR" fails with "too many arguments" — a confusing error that doesn't point back to this line. Use a glob with explicit cardinality validation instead.

♻️ Proposed fix 
-    PTY_DIR=$(ls -d node_modules/.bun/node-pty@*/node_modules/node-pty)
-    (cd "$PTY_DIR" && npx --yes node-gyp rebuild)
+    mapfile -t PTY_DIRS < <(ls -d node_modules/.bun/node-pty@*/node_modules/node-pty 2>/dev/null || true)
+    if [[ ${`#PTY_DIRS`[@]} -ne 1 ]]; then
+      echo "Expected exactly 1 node-pty in Bun store, found ${`#PTY_DIRS`[@]}" >&2; exit 1
+    fi
+    (cd "${PTY_DIRS[0]}" && npx --yes node-gyp rebuild)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
PTY_DIR=$(ls -d node_modules/.bun/node-pty@*/node_modules/node-pty)
(cd "$PTY_DIR" && npx --yes node-gyp rebuild)
mapfile -t PTY_DIRS < <(ls -d node_modules/.bun/node-pty@*/node_modules/node-pty 2>/dev/null || true)
if [[ ${`#PTY_DIRS`[@]} -ne 1 ]]; then
echo "Expected exactly 1 node-pty in Bun store, found ${`#PTY_DIRS`[@]}" >&2; exit 1
fi
(cd "${PTY_DIRS[0]}" && npx --yes node-gyp rebuild)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/cli/scripts/build-dist-linux-docker.sh` around lines 58 - 59, The
PTY_DIR assignment uses `ls -d` which can produce multiple matches and break `cd
"$PTY_DIR"`; replace that with shell glob expansion into an array (matching
node_modules/.bun/node-pty@*/node_modules/node-pty), validate the array
cardinality (if zero -> exit with a clear error, if >1 -> exit with a clear
error instructing to clean the Bun store), then set PTY_DIR to the single
matched path and run the `(cd "$PTY_DIR" && npx --yes node-gyp rebuild)` step;
reference the PTY_DIR variable, the node-pty glob, and the node-gyp rebuild
invocation so reviewers can find and apply the change.

npm rebuild @parcel/watcher

cd packages/cli
bun run build:dist --target="$TARGET"

DIST="./dist/superset-${TARGET}"
"$DIST/bin/superset" --version
"$DIST/bin/superset" --help | head -5
"$DIST/lib/node" --version
NODE_PATH="$DIST/lib/node_modules" "$DIST/lib/node" -e "
for (const m of [\"better-sqlite3\", \"node-pty\", \"@parcel/watcher\", \"libsql\"]) {
require(m);
console.log(m, \"OK\");
}
"
echo "[docker-build] tarball: $(ls -la dist/superset-${TARGET}.tar.gz)"
'
51 changes: 45 additions & 6 deletions packages/cli/scripts/build-dist.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
readdirSync,
readFileSync,
realpathSync,
renameSync,
rmSync,
writeFileSync,
} from "node:fs";
Expand Down Expand Up @@ -137,6 +138,35 @@ async function exec(cmd: string, args: string[], cwd?: string): Promise<void> {
});
}

/**
* curl wrapper that retries on network/HTTP flake (GitHub Releases 5xx,
* connection resets, etc). Writes atomically: download to a .partial
* sibling first, then rename — so a previous half-written file can't be
* mistaken for a cache hit on the next run. `--retry-all-errors` covers
* 5xx as well as transport errors; without it curl only retries a small
* subset by default.
*/
async function curlDownload(url: string, destPath: string): Promise<void> {
const partial = `${destPath}.partial`;
rmSync(partial, { force: true });
await exec("curl", [
"-fsSL",
"--retry",
"6",
"--retry-delay",
"2",
"--retry-all-errors",
"--connect-timeout",
"15",
"--max-time",
"180",
"-o",
partial,
url,
]);
renameSync(partial, destPath);
}

async function downloadAndExtractNode(
target: Target,
destDir: string,
Expand All @@ -150,7 +180,7 @@ async function downloadAndExtractNode(

if (!existsSync(archivePath)) {
console.log(`[build-dist] downloading ${nodeDownloadUrl(target)}`);
await exec("curl", ["-fsSL", "-o", archivePath, nodeDownloadUrl(target)]);
await curlDownload(nodeDownloadUrl(target), archivePath);
}

if (!existsSync(extractedPath)) {
Expand Down Expand Up @@ -282,13 +312,22 @@ async function fixNativeBinariesForNode(
const bsqUrl =
`https://github.com/WiseLibs/better-sqlite3/releases/download/` +
`v${bsqVersion}/better-sqlite3-v${bsqVersion}-node-v${NODE_ABI}-${target}.tar.gz`;
console.log(`[build-dist] fetching Node-ABI better-sqlite3: ${bsqUrl}`);
const tmp = join(homedir(), ".superset-build-cache", `bsq-${target}`);
const cacheDir = join(homedir(), ".superset-build-cache");
if (!existsSync(cacheDir)) mkdirSync(cacheDir, { recursive: true });
const cachedTarball = join(
cacheDir,
`better-sqlite3-v${bsqVersion}-node-v${NODE_ABI}-${target}.tar.gz`,
);
if (!existsSync(cachedTarball)) {
console.log(`[build-dist] fetching Node-ABI better-sqlite3: ${bsqUrl}`);
await curlDownload(bsqUrl, cachedTarball);
} else {
console.log(`[build-dist] using cached better-sqlite3: ${cachedTarball}`);
}
const tmp = join(cacheDir, `bsq-${target}`);
rmSync(tmp, { recursive: true, force: true });
mkdirSync(tmp, { recursive: true });
const tarball = join(tmp, "bsq.tar.gz");
await exec("curl", ["-fsSL", "-o", tarball, bsqUrl]);
await exec("tar", ["-xzf", tarball, "-C", tmp]);
await exec("tar", ["-xzf", cachedTarball, "-C", tmp]);
rmSync(bsqDest, { recursive: true, force: true });
mkdirSync(bsqDest, { recursive: true });
cpSync(
Expand Down
Loading