Remove cmake-era setup from the Nix shells and CONTRIBUTING.md - #38543
Remove cmake-era setup from the Nix shells and CONTRIBUTING.md#38543robobun wants to merge 2 commits into
Conversation
The top-level CMake build was deleted in #28640. The Nix dev shells still export CMAKE_C_COMPILER, CMAKE_CXX_COMPILER, CMAKE_AR, CMAKE_RANLIB and CMAKE_SYSTEM_PROCESSOR, and flake.nix still sets CMAKE_BUILD_TYPE and ENABLE_CCACHE. Nothing reads any of these now: scripts/build passes the compiler, archiver and build type to the nested cmake builds as explicit -D arguments, picks the target arch from clang's default target, and enables ccache whenever it is found on PATH. CC, CXX, AR, RANLIB and LD stay, since cargo build scripts (cc-rs) read them when cargo is run directly from the shell. CONTRIBUTING.md gets the same factual fixes docs/project/contributing.mdx received in #33706: the Nix snippet no longer tells contributors to export CMAKE_SYSTEM_PROCESSOR or mentions a .#pure dev shell that was never defined, the libatomic workaround uses the build script's --static-libatomic=off instead of a cmake -D define, and the troubleshooting text refers to `bun run build` rather than the removed `make setup` / `bun setup` and submodule clone.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: the stale Nix was not available to re-enter the shells; the nix hunks are whole-line deletions inside otherwise unchanged strings and attrsets. Note that CI: the diff touches only |
There was a problem hiding this comment.
LGTM — docs and dev-shell cleanup only, no runtime code touched.
What was reviewed:
- Confirmed
--static-libatomicis a realboolFieldsentry inscripts/build.tsand wired throughscripts/build/config.ts:905, so the new CONTRIBUTING.md command is correct. - Confirmed nested-cmake in
scripts/build/source.tspassesCMAKE_C_COMPILER/CMAKE_AR/CMAKE_BUILD_TYPEetc. as explicit-Dargs from the resolved toolchain, so the removed shell exports have no remaining reader. - Grepped for the removed variable names outside
vendor/; the only hits are the build scripts producing them as-Dargs, a fixture, and unrelatedCMAKE_*_COMPILER_LAUNCHERhandling inenv_loader.rs. flake.nixnever defineddevShells.pure, so dropping the.#purereference from CONTRIBUTING.md is a straight doc fix.
Extended reasoning...
Overview
This PR removes CMake-era environment setup that became dead when #28640 replaced the CMake build with scripts/build/. It touches three files: flake.nix and shell.nix (drops five CMAKE_* exports from shellHook plus the CMAKE_BUILD_TYPE/ENABLE_CCACHE mkShell attrs) and CONTRIBUTING.md (drops the nix develop .#pure / CMAKE_SYSTEM_PROCESSOR snippet, replaces make setup/bun setup with bun run build, replaces -DUSE_STATIC_LIBATOMIC=OFF with --static-libatomic=off, and rewords "clone submodules" to "downloads dependencies"). Every line removed is either a shell env export with no reader or a documented command that the current build system does not accept.
Security risks
None. No source, build script, or CI configuration is modified. The Nix changes only remove environment variables from a developer shell; CC/CXX/AR/RANLIB/LD/LD_LIBRARY_PATH are untouched. The CONTRIBUTING.md changes are prose and command examples.
Level of scrutiny
Low. This is a docs + local-dev-shell hygiene change with no effect on the shipped binary or CI. The correctness question is purely "does anything still read these variables?" — I verified that scripts/build/source.ts:1148-1194 forwards the compiler, archiver, ranlib, and build type to nested cmake as explicit -D arguments derived from the resolved cfg toolchain (not from env), that scripts/build.ts declares staticLibatomic in its bool flags, and that a repo-wide grep for the removed names (excluding vendor/) finds only producers and unrelated *_LAUNCHER handling. flake.nix at HEAD defines only devShells.default, so the removed .#pure reference was already broken.
Other factors
No CODEOWNERS entries cover CONTRIBUTING.md, flake.nix, or shell.nix. There are no prior human review comments to address, and the bug-hunting pass found nothing. The PR description's own verification (git grep for remaining readers, --configure-only --static-libatomic=off producing a build.ninja without -l:libatomic.a) matches what I checked. The author notes nix develop itself was not re-run for lack of Nix, but the edits are pure deletions of exports/attrs inside otherwise-unchanged string literals and attrsets — there is no Nix-syntax risk here.
There was a problem hiding this comment.
LGTM — docs and dev-shell cleanup removing stale CMake-era env exports and commands.
What was reviewed:
- Confirmed
scripts/build/never reads the removedCMAKE_*/ENABLE_CCACHEenv vars — all remaining hits are code that produces them as explicit-Dargs to nested cmake, comments, or fixture data. - Verified
--static-libatomic=offis the real flag (staticLibatomicinscripts/build.tsboolFields,scripts/build/config.ts:905). - CONTRIBUTING.md now matches
docs/project/contributing.mdxon the Nix snippet and libatomic command;flake.nixnever defined a.#pureshell. - Nix changes are pure line deletions inside otherwise-unchanged
shellHookstrings and themkShellattrset — no syntax risk.
Extended reasoning...
Overview
This PR touches three files: CONTRIBUTING.md, flake.nix, and shell.nix. It removes environment-variable exports (CMAKE_C_COMPILER, CMAKE_CXX_COMPILER, CMAKE_AR, CMAKE_RANLIB, CMAKE_SYSTEM_PROCESSOR) and mkShell attributes (CMAKE_BUILD_TYPE, ENABLE_CCACHE) that were only consumed by the deleted CMake build system, and updates CONTRIBUTING.md to drop references to nix develop .#pure (never existed), make setup/bun setup, submodule cloning, and -DUSE_STATIC_LIBATOMIC=OFF — replacing them with the current bun run build / --static-libatomic=off equivalents that docs/project/contributing.mdx already documents.
Security risks
None. No runtime, build-script, or native code is touched. The changes are contributor documentation and Nix dev-shell environment variables, which affect only the local shell of a developer who opts into nix develop.
Level of scrutiny
Low. This is a docs + dev-environment cleanup with no shipped code impact. I verified the factual claims: grepping the tree (excluding vendor/) for the removed variable names shows only scripts/build/source.ts and deps/webkit.ts writing them as explicit -D cmake args from the resolved toolchain, comments in tools.ts/config.ts/flags.ts, the cross-compile toolchain file generator xmac.mjs, an unrelated env_loader.rs ccache-launcher path (different variables), and fixture data in zstd.test.ts. Nothing reads them from the process environment. The --static-libatomic=off replacement is the real flag name per scripts/build.ts:443 and scripts/build/config.ts:903-905. docs/project/contributing.mdx:15,19,370 already carries the identical wording, so this brings the two contributor docs into agreement.
Other factors
The Nix edits are pure whole-line deletions inside multi-line ''...'' shellHook strings and a two-attribute deletion from the mkShell attrset — the surrounding string concatenation and Nix syntax are unchanged, so there is no evaluation risk even though nix develop was not re-run. No CODEOWNERS entries cover these files. No prior human review comments to address. Per the repo's own "Docs, types, and comments" guidance, docs-only changes with no test to add are expected.
Problem
flake.nixandshell.nixstill exportCMAKE_C_COMPILER,CMAKE_CXX_COMPILER,CMAKE_AR,CMAKE_RANLIBandCMAKE_SYSTEM_PROCESSORfrom theirshellHook, andflake.nixstill setsCMAKE_BUILD_TYPE = "Debug"andENABLE_CCACHE = "1"(flake.nix:142-146, 171-172; shell.nix:84-88).CONTRIBUTING.md(line 12-13) tells contributors toexport CMAKE_SYSTEM_PROCESSOR=$(uname -m)beforebun bd, and offers anix develop .#pureshell thatflake.nixhas never defined (it only hasdevShells.default).CONTRIBUTING.mdalso still documentsbun run build -DUSE_STATIC_LIBATOMIC=OFF(line 366) and refers tomake setup/bun setup/ cloning submodules (lines 153, 308, 316).git grep -nE 'env(\.|\[)CMAKE_|ENABLE_CCACHE' -- scriptsis empty.-Darguments from the resolved toolchain,scripts/build/source.ts:1148-1152and:1194, so the environment is never consulted for them.clangTargetArch,scripts/build/tools.ts:192), with no environment override.cfg.ccache,scripts/build/source.ts:1178);ENABLE_CCACHEand theCMAKE_BUILD_TYPEenv lookup were features of the deletedoptionxcmake macro.-DUSE_STATIC_LIBATOMIC=OFFis not a--flag, soscripts/build.tsparseArgstreats it as the first exec arg: the build runs with static libatomic anyway and the define is handed to the freshly built binary.Fix
CMAKE_*exports from both shell hooks and theCMAKE_BUILD_TYPE/ENABLE_CCACHEattributes fromflake.nix.CC,CXX,AR,RANLIB,LD,NIX_CFLAGS_LINK,LD_LIBRARY_PATHandpkgs.cmakestay: cc-rs build scripts read the first group whencargo check/bun run watchare run straight from the shell, andscripts/build/configure.ts:50still requires cmake for the nested dep builds.CONTRIBUTING.mdthe same factual fixesdocs/project/contributing.mdxalready received in docs: fact-check sweep across 120 pages #33706: Nix snippet reduced tonix develop+bun bd, libatomic workaround becomesbun run build --static-libatomic=off, troubleshooting text saysbun run build, and the build section says dependencies are downloaded rather than cloned as submodules. The two files now agree on every command they document; the remaining differences between them are wording only.staticLibatomicinscripts/build.tsboolFields,scripts/build/config.ts:902-905).bun scripts/build.ts --configure-only --static-libatomic=offconfigures and the generatedbuild.ninjano longer links-l:libatomic.a; the default configure still does.git grep -n 'CMAKE_SYSTEM_PROCESSOR\|ENABLE_CCACHE\|CMAKE_C_COMPILER\|CMAKE_AR\|CMAKE_RANLIB' -- ':!vendor'hits onlyscripts/buildcode that produces these values itself (source.ts-Dargs,deps/webkit.ts,xmac.mjs), comments, and an oldpackage.jsonused as fixture data intest/js/bun/util/zstd.test.ts.prettier --check CONTRIBUTING.mdpasses.nix developitself was not re-run; the nix change only deletes whole lines inside otherwise unchangedshellHookstrings and the mkShell attrset.flake.lockstill pins the nixpkgs from Add Nix flake for development environment #23406 (2025-10-07), which predates thepkgs.nodejs_26that Upgrade reported Node.js version to 26.3.0 #31991 put atflake.nix:35, sonix developcurrently fails at evaluation with or without this PR; Fix up the nix shell loading and build issues #37073 (open) updates the lock and makes the build honourCC/CXX/AR/RANLIB/LD. Fix up the nix shell loading and build issues #37073 keeps the lines removed here and edits other regions of both files, so the two apply in either order. This PR only removes lines that have no reader under any outcome of that work.Background
scripts/build/replaced the CMake build in delete cmake #28640. It resolves the toolchain itself (tools.ts) and writes abuild.ninja; the only cmake it still runs is fornested-cmakedependencies (today just a local WebKit build), and it passes every toolchain setting to that cmake on the command line.optionxwas a macro in the deletedcmake/Globals.cmakethat let any build option (ENABLE_CCACHE,CMAKE_BUILD_TYPE, ...) be supplied through the environment. That is the mechanism the Nix shells were feeding; it no longer exists.CMAKE_<LANG>_COMPILER,CMAKE_ARandCMAKE_SYSTEM_PROCESSORare cmake cache/internal variables, not environment variables cmake reads (cmake readsCC/CXXfrom the environment), so exporting them only ever had an effect through project-specific code, of which none remains.mkShellturns unknown attributes such asCMAKE_BUILD_TYPE = "Debug"into environment variables of the dev shell; removing them removes only those variables.no test proof · iteration 0 · docs-only change; test-proof not applicable