fix(bootstrap): fail fast when Node.js / yarn are missing (#9544)#9669
fix(bootstrap): fail fast when Node.js / yarn are missing (#9544)#9669lonexreb wants to merge 3 commits intowarpdotdev:masterfrom
Conversation
…#9544) `command-signatures-v2`'s build.rs invokes `yarn build`, so a fresh checkout without Node/yarn fails partway through `cargo build`. The existing build.rs panic mostly works but is buried under a long cc-rs / build-script trace, and there's no canonical "how to install Node for Warp" guide. This change makes the failure surface at bootstrap time on all three platforms and consolidates the install guidance in one place: - script/macos/bootstrap, script/linux/bootstrap, script/windows/ bootstrap.ps1: after the rust toolchain check, fail fast if `node` or `yarn` is not on PATH and point users at WARP.md. We deliberately do not auto-install Node or auto-run `corepack enable` — corepack can require sudo/admin on a system-installed Node, and Volta users manage yarn themselves. - WARP.md: new "Node.js setup" section under "Platform Setup" covering the version requirement (≥ 18.14.1), recommended install paths (nvm/fnm/volta/asdf/mise or system package manager), the yarn-already-on-PATH check, and the corepack/Volta caveats. - crates/command-signatures-v2/build.rs: trimmed the panic message to point at the new WARP.md section instead of duplicating the instructions inline. Sanity-checked the bash control flow with `bash -n` and ran the no-node case manually on this checkout (the new branch on a Node-less shell exits with the WARP.md pointer before any other work happens). PowerShell pwsh isn't installed locally; relying on CI's Windows lane to validate the .ps1 path.
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @warpdotdev/oss-maintainers. Comment 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 Node.js/yarn setup guidance to WARP.md, updates the command-signatures-v2 build failure text to point at that guidance, and adds bootstrap-time checks for node and yarn on macOS, Linux, and Windows.
Concerns
- No blocking correctness or security concerns found in the changed lines.
- No matching individual stakeholder is listed for these root, script, and command-signatures paths; the catch-all rule is a team, so recommended_reviewers is empty for workflow fallback.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
captainsafia
requested review from
a team and
peicodes
and removed request for
a team
…-node-yarn-check # Conflicts: # script/windows/bootstrap.ps1
|
@lonexreb Sorry for the delay in reviewing this. Once CI passes I'll merge this change. |
PSScriptAnalyzer's PSUseBOMForUnicodeEncodedFile rule fired on script/windows/bootstrap.ps1 because the comment I added in this PR contained an em-dash, and the file (per repo convention for script/windows/*.ps1) ships without a UTF-8 BOM. Fix: replace the em-dash with a colon. The comment reads identically and the file becomes pure ASCII, so the BOM rule no longer applies. No behavior change; lint-only fix.
The bootstrap paths that Aloke referenced are the OS bootstrap scripts (script/linux/bootstrap, script/macos/bootstrap, script/windows/bootstrap.ps1) from PR #9669, not the app-level bootstrap.rs and bundled/bootstrap/ paths. Co-Authored-By: Oz <[email protected]>
Per Aloke's recommendation, add script/linux/bootstrap, script/macos/bootstrap, and script/windows/bootstrap.ps1 with @vorporeal @zachbai @alokedesai @bnavetta @acarl005 as stakeholders (matching the files changed in PR #9669). Co-Authored-By: Oz <[email protected]>
|
Pushed a fix for the Miscellaneous checks failure on the previous commit. Root cause: PSScriptAnalyzer's Fix (commit 84e42ed): replaced the em-dash with a colon. The file is now pure ASCII (verified with CI on the new commit is currently |
2 participants
Description
Resolves #9544.
command-signatures-v2'sbuild.rsinvokesyarn build, so a fresh checkout without Node/yarn fails partway throughcargo buildrather than at bootstrap time. The existing build-script panic eventually surfaces the right hint, but it's buried under a long cc-rs / build-script trace and there's no canonical "how to install Node for Warp" guide for new contributors to follow.This change makes the failure surface at bootstrap on all three platforms and consolidates install guidance in one place.
Linked Issue
ready-to-specorready-to-implement. (Bootstrap scripts don't check for Node.js/yarn, leading to confusingcargo buildfailures #9544 isready-to-implement.)Resolves #9544.
Changes
script/macos/bootstrapnodeoryarnis missing and point at WARP.md.script/linux/bootstrapinstall_test_deps/install_linuxdeploy.script/windows/bootstrap.ps1Get-Command -Type Application(matches the existing cargo / gcloud probes).WARP.mdcrates/command-signatures-v2/build.rsWhy no auto-install / auto
corepack enableThe issue author was explicit, and the rationale held up on review:
corepack enableon a system-installed Node typically needssudo/ Administrator rights — fragile for a bootstrap script.corepack enableactively conflicts.Failing fast with a single, well-scoped pointer to WARP.md is the lowest-blast-radius fix.
Testing
bash -n script/macos/bootstrapandbash -n script/linux/bootstrap— both parse cleanly.PATHmunged to dropnode/yarn); the new check fires and exits with the WARP.md pointer before any of the subsequentbrew install/gcloudsteps run, which is the desired behavior. With both binaries present, the check is a no-op..ps1path is unrun. CI's Windows lane will exercise it; the syntax mirrors the existingcargo/gcloudGet-Command -Type Applicationprobes immediately above and below it.build.rsedit is a string-only change inside an existingpanic!).Agent Mode
CHANGELOG-IMPROVEMENT: Bootstrap scripts now fail fast with a clear pointer to WARP.md when Node.js / yarn aren't on PATH, instead of letting the failure surface mid-
cargo build. Thanks @lonexreb!