Check for Node.js and yarn at bootstrap time#9546
Check for Node.js and yarn at bootstrap time#9546landkirk wants to merge 1 commit intowarpdotdev:masterfrom
Conversation
The `command-signatures-v2` crate compiles a TypeScript helper at `cargo build` time using `yarn`, but none of the bootstrap scripts verified that Node or yarn were on PATH. Fresh checkouts succeeded at bootstrap and then failed deep inside a build-script panic with inline setup instructions that were easy to miss. - Fail fast in `script/linux/install_build_deps`, `script/macos/bootstrap`, and `script/windows/bootstrap.ps1` if `node` or `yarn` is missing. Don't auto-run `corepack enable`: it can need sudo / an admin shell for system-installed Node, and Volta manages yarn itself. - Add a "Node.js setup" section to WARP.md covering version managers, system packages, the `yarn --version` / `corepack enable` flow, and the sudo/Volta caveats. - Trim the `command-signatures-v2/build.rs` panic to point at that section. Fixes warpdotdev#9544.
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
|
I'm checking this implementation PR for association with an explicitly linked issue. The PR that you've opened seems to contain implementation changes and is associated with issue #9544, but none of those associated issue are marked as Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds bootstrap-time checks for Node.js and yarn on Linux, macOS, and Windows, updates the command-signatures build panic to point at WARP.md, and documents Node.js setup.
Concerns
- The new checks only verify that
nodeandyarncommands exist. They still allow the documented stale-Node failure mode and the known incompatible/shadowing Yarn shim failure mode to pass bootstrap and fail later inbuild.rs.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| "$PWD"/script/install_rust | ||
|
|
||
| # Node + yarn are required to build the command-signatures-v2 crate. | ||
| if ! command -v node >/dev/null 2>&1; then |
There was a problem hiding this comment.
node exists, not the documented >=18.14.1 requirement, so older system Node installations still pass bootstrap and fail later in build.rs; validate node --version before continuing on all three bootstrap scripts.
| echo -e "${red}Build requires Node.js. See the 'Node.js setup' section in WARP.md.${reset}" | ||
| exit 1 | ||
| fi | ||
| if ! command -v yarn >/dev/null 2>&1; then |
There was a problem hiding this comment.
command -v yarn accepts incompatible shims like a brew/global Yarn 1 install, which this PR documents as a known build failure; run yarn --version in the command-signatures-v2/js package and reject incompatible majors or command failures in each platform bootstrap.
captainsafia
added
the
external-contributor
Warp Dev Github Integration
2 participants
Description
Fixes #9544.
The
command-signatures-v2crate compiles a TypeScript helper atcargo buildtime usingyarn, but none of the bootstrap scripts verified that Node or yarn were on PATH. Fresh checkouts succeeded at bootstrap and then failed deep inside abuild.rspanic with inline setup instructions that were easy to miss.What changed:
script/linux/install_build_deps,script/macos/bootstrap, andscript/windows/bootstrap.ps1ifnodeoryarnis missing.WARP.mdcovering version managers, system packages, theyarn --version/corepack enableflow, and the sudo / Volta caveats.command-signatures-v2/build.rspanic to point at that section instead of duplicating instructions inline.What this PR deliberately does not do:
corepack enable. Whether this works depends on how Node was installed: system-installed Node needs sudo / an admin shell, and Volta manages yarn itself so corepack would conflict. Running it unconditionally would silently break those setups.Testing
Manually verified on Linux:
nodeandyarnon PATH,./script/linux/install_build_depsproceeds normally.noderemoved from PATH, the script exits with the new error message before invoking cargo.nodepresent butyarnmissing, the script exits pointing at the WARP.md setup section.No automated test coverage — these are bootstrap shell/PowerShell scripts that aren't exercised in CI.