Skip to content

Commit

Permalink
github(nix): don't build jj twice when testing
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
thoughtpolice committed Feb 22, 2024
1 parent c32b68e commit a8e77a9
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 2 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/nix-linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,4 @@ jobs:
fetch-depth: 0
- uses: DeterminateSystems/nix-installer-action@cd46bde16ab981b0a7b2dce0574509104543276e
- uses: DeterminateSystems/magic-nix-cache-action@eeabdb06718ac63a7021c6132129679a8e22d0c7
- run: nix build --print-build-logs --show-trace
- run: nix flake check -L --show-trace
19 changes: 18 additions & 1 deletion flake.nix
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
version = "unstable-${self.shortRev or "dirty"}";

buildFeatures = [ "packaging" ];
cargoBuildFlags = ["--bin" "jj"]; # don't build and install the fake editors
cargoBuildFlags = [ "--bin" "jj" ]; # don't build and install the fake editors
useNextest = true;
src = filterSrc ./. [
".*\\.nix$"
Expand Down Expand Up @@ -110,11 +110,28 @@
};
default = self.packages.${system}.jujutsu;
};

apps.default = {
type = "app";
program = "${self.packages.${system}.jujutsu}/bin/jj";
};

formatter = pkgs.nixpkgs-fmt;

checks.jujutsu = self.packages.${system}.jujutsu.overrideAttrs ({ ... }: {
# FIXME (aseipp): when running `nix flake check`, this will override the
# main package, and nerf the build and installation phases. this is
# because for some inexplicable reason, the cargo cache gets invalidated
# in between buildPhase and checkPhase, causing every nix CI build to be
# 2x as long.
buildPhase = "true";
installPhase = "touch $out";
# NOTE (aseipp): buildRustPackage also, by default, runs `cargo check`
# in `--release` mode, which is far slower; the existing CI builds all
# use the default `test` profile, so we should too.
cargoCheckType = "test";
});

devShells.default = pkgs.mkShell {
buildInputs = with pkgs; [
# The CI checks against the latest nightly rustfmt, so we should too.
Expand Down

0 comments on commit a8e77a9

Please sign in to comment.