-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
buildRustPackage has excessive dependency rebuilds in checkPhase for unclear reasons #291222
Description
Problem description
Jujutsu is a Rust project that has a Nix flake in its repository. The flake is meant to provide a convenient nix profile install path, and is built using this Nix expression at its core. This is not a complicated expression; it runs both cargo build and cargo test, as buildRustPackage is meant to do.
At the moment, our CI system just runs nix build . on every commit, because the build naturally runs the tests, too, which is ideal. However, at the moment, it effectively rebuilds the whole project twice, once for test, and once for the release binaries, even with the same profile. The build log is included below. The manual reference says:
When using
buildRustPackage, thecheckPhaseis enabled by default and runscargo teston the package to build. To make sure that we don't compile the sources twice and to actually test the artifacts that will be used at runtime, the tests will be ran in the release mode by default.
— https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/rust.section.md#running-package-tests-running-package-tests
This seems like a strange regression or edge case, of some kind. Or maybe I just don't understand how buildRustPackage works; that's also quite possible.
In short, this pretty much bloats our nix build time by 2x more than necessary. You can see in the full build log that the cargo build --release phase took 2m51s, the cargo test build took 3m7s (because it compiled everything again + a little extra for the tests), and the test suite itself took only a minute or so.
This is a pretty severe impediment to our CI times and it makes Nix our slowest CI build by far. To stem the immediate bleeding, I created a PR that will:
- Separate out a new Flake check, used by CI, which is
- An
.overrideof the main expression, setting buildbuildPhase = "true";and also, - Setting the build profile to
testto cut down on build times, on top of all that.
These changes halve the amount of time our Continuous Integration takes, bringing it very close to normal Linux builds. Honestly, just setting the test profile is valuable enough for our CI times that keeping a nix flake check separate from the build is probably worth it.
However, while this patches up the CI build, which now uses flake check, the ordinary nix build still suffers from this flaw in our setup, so users who nix profile install (like me!) will still need to wait an extra 3+ minutes to upgrade their build.
Leading theory
When I was debugging this for 5 minutes this week, I think a part of the problem is that, for some reason, the cargo build step has several environment variable set like HOST_CC and HOST_CXX when the command is run, while these aren't applied to the cargo test command. When I looked at the debugging output, it seemed like Cargo is probably rebuilding stuff for reasons like that, because it has an impact on any foreign libraries with build.rs scripts that might need to interrogate the host compiler (and we have several crates that use C libraries and compile them on the fly, it's super common obviously.)
I haven't investigated this any further, but this is basically my leading theory, right now.
Ideal behavior
I would like our normal jujutsu expression to not rebuild everything twice when nix build is run.
Notes
- Full build log: https://gist.github.com/thoughtpolice/f7d2e2280af294b0909b1969246f85a9
- GHA build (will be GC'd at some point): https://github.com/martinvonz/jj/actions/runs/8028926504/job/21934651219