github(nix): don't build jj twice when testing
#3118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thoughtpolice
force-pushed
the
aseipp/push-wtlumozmzpms
branch
2 times, most recently
from
482efbb to
a8e77a9
Compare
Ralith
approved these changes
Feb 23, 2024
thoughtpolice
force-pushed
the
aseipp/push-wtlumozmzpms
branch
from
a8e77a9 to
c835bea
Compare
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 <[email protected]>
thoughtpolice
force-pushed
the
aseipp/push-wtlumozmzpms
branch
from
c835bea to
1b421c1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When running the
nix build, thebuildRustPackagefunction -- which builds thejjcrates -- callscargo build --releasewith flags likeHOST_CXXset. This is called thebuildPhase. Then, it runs thecheckPhase, which callscargo nextest, in our case. However, it does not setHOST_CXX, for some reason.The intent of
buildRustPackageis that thebuildPhaseandcheckPhasehave their compilation options fully aligned so that they reuse the local cargotarget/cache directory, avoiding a full recompilation. However, the lack ofHOST_CXXabove among others causes a cache miss, and a bunch of cascading recompilations. The net impact is that we compile all of the codebase once inbuildPhase, then again incheckPhase, 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
jujustsupackage, but stubs out thebuildPhase. This means it will only runcheckPhase, which is what we want. Then, modify the CI to runnix flake checkagain, like it used to in the past.Unfortunately, this doesn't fix the cache miss when running
nix buildyourself, 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.