Repository navigation
id: ci-check profile for the nix test phase (#3195 phase 1) - #3674
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 13 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4051de0b60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # instead of release: same opt-level, no LTO / codegen-units=1 / | ||
| # strip costs for test binaries. The release binary build | ||
| # (cargoBuildType) is unchanged. | ||
| checkType = "ci-check"; |
There was a problem hiding this comment.
For the documented nix build and nix build .#id-lib entry points, this setting has no effect: pkgs/id/flake.nix independently defines id-web and id-lib (lines 616–728), both with doCheck = true but neither with checkType. A repo-wide search of Nix sources found no import of this default.nix; the root integration also imports pkgs/id/flake.nix directly. Consequently, normal Nix package builds still compile tests with the expensive release profile, defeating the optimization this change introduces. Set checkType on those flake derivations as well, or share their configuration with this derivation.
AGENTS.md reference: pkgs/id/AGENTS.md:L86-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0948694: checkType = "ci-check" now set on both flake derivations (id-web and id-lib in pkgs/id/flake.nix), which are the actual nix build / nix build .#id-lib entry points; Cargo.toml comment updated to match. A repo-wide search confirms those two plus default.nix are the only buildRustPackage sites for pkgs/id. Load-gated before/after timings (load <20) still pending — task 5375 parked on that gate; @codex review requested on the new head.
Codex P2 on PR #3674: default.nix is not imported by the flake entry points; pkgs/id/flake.nix defines id-web and id-lib independently with doCheck=true but no checkType, so nix build still compiled tests in release. Set checkType = ci-check on both.
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Phase 1 of #3195 (queue task 5375), per the triage pin in task 5367: the nix
buildRustPackagecheck phase was compiling the test suite under the release profile — thin LTO,codegen-units = 1, symbol stripping — and[profile.test]is ignored in release mode, so every check build paid full release codegen for throwaway test binaries.Changes
pkgs/id/Cargo.toml: add[profile.ci-check]—inherits = "release"(opt-level 3 kept, so test behaviour andFUEL_BUDGETSassertions stay release-shaped) withlto = false,codegen-units = 256,strip = false.pkgs/id/default.nix:checkType = "ci-check"on thebuildRustPackagederivation, so the cargo check hook runscargo test --profile ci-check. The release binary build (cargoBuildType) is untouched. Thepkgs/id/flake.nixintegration-test runner compiles in the debug profile already, so it needs no change.Verified this shift
cargo metadata --no-depsaccepts the manifest; the parsed profile is exactly{inherits: release, lto: false, codegen-units: 256, strip: false}.nix-instantiate --evalof the derivation showscargoCheckType = "ci-check".Still owed before merge (load-gated, tracked on #3195 / task 5375)
cargo test --profile ci-check -v): test harness shows ci-check flags, binary keeps release flags.FUEL_BUDGETSassertions passing.cargo test --no-runin a clean target dir) recorded on pkgs/id: build-time review — runtime performance first, then build time (cold check build ~16 min, tests run 12 s) #3195 with laptop load beside each number — only timings taken at load <20 count as signal.Refs #3195
Need help on this PR? Tag
@codesmith-botwith what you need.