Skip to content

github(nix): don't build jj twice when testing#3118

Merged
thoughtpolice merged 1 commit intomainfrom
aseipp/push-wtlumozmzpms
Feb 24, 2024
Merged

github(nix): don't build jj twice when testing#3118
thoughtpolice merged 1 commit intomainfrom
aseipp/push-wtlumozmzpms

Conversation

@thoughtpolice
Copy link
Copy Markdown
Member

@thoughtpolice thoughtpolice commented Feb 22, 2024

When running the nix build, the buildRustPackage function -- which builds the jj crates -- calls cargo build --release with flags like HOST_CXX set. This is called the buildPhase. Then, it runs the checkPhase, which calls cargo nextest, in our case. However, it does not set HOST_CXX, for some reason.

The intent of buildRustPackage is that the buildPhase and checkPhase have their compilation options fully aligned so that they reuse the local cargo target/ cache directory, avoiding a full recompilation. However, the lack of HOST_CXX above among others causes a cache miss, and a bunch of cascading recompilations. The net impact is that we compile all of the codebase once in buildPhase, then again in checkPhase, making the Nix CI build 2x slower on average than the other Linux runners; 2-3 minutes versus 7 minutes, on average.

Instead, re-introduce a 'check' into the Flake directly, which overrides the jujustsu package, but stubs out the buildPhase. This means it will only run checkPhase, which is what we want. Then, modify the CI to run nix flake check again, like it used to in the past.

Unfortunately, this doesn't fix the cache miss when running nix build yourself, it recompiles from scratch in both phases still. That needs to be fixed in the future, but it's tolerable for now.

This reverts most of 71a3045.

@thoughtpolice thoughtpolice force-pushed the aseipp/push-wtlumozmzpms branch 2 times, most recently from 482efbb to a8e77a9 Compare February 22, 2024 20:06
When running the `nix build`, the `buildRustPackage` function -- which builds
the `jj` crates -- calls `cargo build --release` with flags like `HOST_CXX`
set. This is called the `buildPhase`. Then, it runs the `checkPhase`, which
calls `cargo nextest`, in our case. However, it does not set `HOST_CXX`, for
some reason.

The intent of `buildRustPackage` is that the `buildPhase` and `checkPhase`
have their compilation options fully aligned so that they reuse the local cargo
`target/` cache directory, avoiding a full recompilation. However, the lack
of `HOST_CXX` above among others causes a cache miss, and a bunch of cascading
recompilations. The net impact is that we compile all of the codebase once in
`buildPhase`, then again in `checkPhase`, making the Nix CI build 2x slower on
average than the other Linux runners; 2-3 minutes versus 7 minutes, on average.

Instead, re-introduce a 'check' into the Flake directly, which overrides the
`jujustsu` package, but stubs out the `buildPhase`. This means it will only run
`checkPhase`, which is what we want. Then, modify the CI to run `nix flake check`
again, like it used to in the past.

Unfortunately, this doesn't fix the cache miss when running `nix build`
yourself, it recompiles from scratch in both phases still. That needs to be
fixed in the future, but it's tolerable for now.

This reverts most of 71a3045.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
@thoughtpolice thoughtpolice force-pushed the aseipp/push-wtlumozmzpms branch from c835bea to 1b421c1 Compare February 24, 2024 19:47
@thoughtpolice thoughtpolice enabled auto-merge (rebase) February 24, 2024 19:48
@thoughtpolice thoughtpolice merged commit 3f7b5a7 into main Feb 24, 2024
@thoughtpolice thoughtpolice deleted the aseipp/push-wtlumozmzpms branch February 24, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants