Upgrade napi-rs CLI to v3 - #497
Conversation
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a minimal, tested change that switches the native build to @napi-rs/cli v3 via the explicit napi executable (correct npx --package … cmd usage). One low-severity note: the CLI version was loosened from an exact pin to a floating @3 range, which reduces build reproducibility.
| "update-version": "node bin/update-version.js && prettier --write ./lib/version.ts", | ||
| "build": "npm run update-version && tsc --project tsconfig.build.json", | ||
| "build:native": "bash -c 'cd ${DATABRICKS_SQL_KERNEL_REPO:-../../databricks-sql-kernel}/napi && npx --yes @napi-rs/cli@2.18.4 build --platform ${BUILD_PROFILE:---release} && cp index.* $OLDPWD/native/kernel/'", | ||
| "build:native": "bash -c 'cd ${DATABRICKS_SQL_KERNEL_REPO:-../../databricks-sql-kernel}/napi && npx --yes --package @napi-rs/cli@3 napi build --platform ${BUILD_PROFILE:---release} && cp index.* $OLDPWD/native/kernel/'", |
There was a problem hiding this comment.
🔵 Low — The napi-rs CLI version was loosened from an exact pin (@2.18.4) to a floating major range (@3). With --yes fetching the latest 3.x on each run, npm run build:native becomes non-reproducible: a future 3.x release could change CLI flags/output and break the native build (or silently alter the generated index.* artifacts) without any change to this repo. This repo otherwise favors exact pins (e.g. TypeScript is pinned to exact 5.5.4). Consider pinning to the specific 3.x version you validated against (e.g. @napi-rs/cli@3.x.y) so the build stays deterministic.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean extraction of the native build into bin/build-native.sh with sensible per-major CLI pinning; capturing driver_repo=$(pwd) up front is more robust than the old $OLDPWD trick. One low-severity note: the sed-based napi major detection is sensitive to the kernel Cargo.toml's exact formatting and will hard-fail on common variants (workspace-inherited or bare-string versions). Nit: adding quotes around ${BUILD_PROFILE:---release} is a fine improvement but would collapse a multi-token BUILD_PROFILE into a single arg (edge case only).
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-written refactor of build:native into bin/build-native.sh with proper set -euo pipefail, version detection, and loud failure on unsupported napi majors. One low-severity note about a quoting change to BUILD_PROFILE that could affect multi-token values (no impact on the default --release).
| esac | ||
|
|
||
| cd "${napi_dir}" | ||
| "${cli[@]}" build --platform "${BUILD_PROFILE:---release}" |
There was a problem hiding this comment.
🔵 Low — BUILD_PROFILE is now passed quoted as a single argument, whereas the previous inline command (... build --platform ${BUILD_PROFILE:---release} ...) left it unquoted and subject to word-splitting.
For the default value --release this is equivalent (single token, works as napi build --platform --release). But if a caller sets a multi-token BUILD_PROFILE (e.g. --release --features foo, or a distinct profile plus flag), the quoted form collapses it into one argv entry and the CLI will reject it, whereas the old behavior word-split it into separate flags. This is a subtle, silent behavior change for anyone who relied on multi-word BUILD_PROFILE.
If multi-token profiles are intended to be supported, use an array (e.g. read -ra build_profile <<< "${BUILD_PROFILE:---release}" and expand "${build_profile[@]}"); otherwise this is fine to leave as-is and worth a brief comment that BUILD_PROFILE must be a single flag.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Small, well-scoped build-tooling change that extracts the native build into bin/build-native.sh and selects the napi-rs CLI by detected major version. The napi build --platform --release invocation and cp index.* copy semantics are preserved correctly. One medium concern: the documented empty-BUILD_PROFILE debug path produces an empty array that can abort under set -u on older (macOS stock) bash.
| esac | ||
|
|
||
| read -r -a build_profile <<< "${BUILD_PROFILE:---release}" | ||
|
|
There was a problem hiding this comment.
🟡 Medium — When BUILD_PROFILE= is set (the documented debug-build path), read -r -a build_profile <<< "" produces an empty array. The final expansion "${build_profile[@]}" is then evaluated under set -u (set -euo pipefail, line 3).
On bash 4.3 and earlier, expanding an empty array as "${arr[@]}" under set -u raises build_profile[@]: unbound variable and aborts the script (this was only fixed in bash 4.4). macOS still ships bash 3.2.57 as /usr/bin/bash, and the shebang is #!/usr/bin/env bash, so a developer on stock macOS bash running the documented BUILD_PROFILE= npm run build:native debug workflow will hit an abort before napi is ever invoked.
The release default path is unaffected (the array holds --release). Guarding the expansion makes the empty case safe on all bash versions.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a small, well-written build-script extraction with robust napi major-version detection and correct v2/v3 CLI invocations. One low-severity note: the default operator changed from :- to -, so an explicitly-empty BUILD_PROFILE now yields a debug build instead of --release (unset behaves as before).
| ;; | ||
| esac | ||
|
|
||
| build_profile=${BUILD_PROFILE---release} |
There was a problem hiding this comment.
🔵 Low — The default operator changed from :- to -. ${BUILD_PROFILE---release} only substitutes --release when BUILD_PROFILE is unset; the original inline command used ${BUILD_PROFILE:---release} which also substituted when the variable was set-but-empty.
Consequence: running with BUILD_PROFILE= (explicitly empty) now falls through to the else branch (napi build --platform with no profile), producing a debug build, whereas previously it produced a --release build. The common unset case (e.g. CI) is unaffected since both default to --release.
If empty-means-debug is intentional (the whitespace-stripping if guard suggests it is), this is fine — but the divergence from the previous :- semantics is worth confirming so an empty override doesn't silently ship a debug native binary.
Upgrade the native build from
@napi-rs/cli@2.18.4to the exact@napi-rs/cli@3.8.2release and invoke itsnapiexecutable explicitly. This matches the SQL kernel's napi-rs v3 bindings, prevents the missing-environment-variables macro panic, and keeps the build deterministic.Tested by reproducing the failure with CLI 2.18.4, then successfully running
npm run build:nativewith CLI 3.8.2 against SQL kernelmainat0ecc09c9d0075c630df2adce1a6414051a16cc9e.