Compile benchmarks via Nix

[asymmetric]

This should speed up CI runs, as it reuses what's already been built and cached, and exploits build parallelism.

I've separated cargo build and cargo build --no-run in the second commit. The idea is that cargo build (which is available as .#benchmarks) could be used in the benchmarking workflow (although it would require patching/forking criterion-compare-action). Happy to revert in case of YAGNI.

Follow-up to #1104

[asymmetric]

It would be nice if crane had a lib.cargoBench.

[yannham]

Would that make sense to have benchmarks be a function taking an attribute set { noRun ? false } as a parameter, and appending conditionally the --no-run parameter? It's not very different, but somehow it confines the "domain knowledge" to the mkArtifact function (low-level commands and parameters). Any usage of benchmarks as a flake output then just has to decide noRun = true on noRun = false without having to leak the actual cargo commands to do so.

I think that would solve as well your other issues: you wouldn't have to override buildPhase but could directly act on buildPhaseCargoCommand, and you would'nt have to call mkArtifacts a second time.

[asymmetric]

That sounds like a very good plan! Only thing is that benchmarks can't be a generic function AFAICT, it needs to be a derivation, otherwise the CLI will chocke on e.g. nix flake show.

But this is solvable by moving the argument up to mkCraneArtifacts.

[yannham]

Oh indeed 👍 I thought this code was innside mkArtifacts, my bad.

[yannham]

Well in fact we are in mkArtifacts ! AFAICT, mkArtifacts is just an internal value of the flake. It turns out right now it's only composed of derivations, but I think you can make benchmarks a function if you'd like to 🙂 you would just have to apply it in packages and checks. Currently it doesn't make a big difference though, you've just extruded the function at the top of mkArtifacts. I think that's fine too. It's theoretically less flexible (we can't reuse the same mkArtifacts both with and without benchmark)

[asymmetric]

This is what it would have to look like in practice, but it doesn't work:

diff --git a/flake.nix b/flake.nix
index 6c608754..93f9f345 100644
--- a/flake.nix
+++ b/flake.nix
@@ -182,7 +182,7 @@
 
       # Given a rust toolchain, provide Nickel's Rust dependencies, Nickel, as
       # well as rust tools (like clippy)
-      mkCraneArtifacts = { rust ? mkRust { }, noRunBench ? false }:
+      mkCraneArtifacts = { rust ? mkRust { } }:
         let
           craneLib = crane.lib.${system}.overrideToolchain rust;
 
@@ -217,13 +217,13 @@
           nickel = buildPackage "nickel-lang";
           lsp-nls = buildPackage "nickel-lang-lsp";
 
-          benchmarks = craneLib.mkCargoDerivation {
+          benchmarks = { noRun ? false }: craneLib.mkCargoDerivation {
             inherit src cargoArtifacts;
 
             pnameSuffix = "-bench";
 
             buildPhaseCargoCommand = ''
-              cargo bench ${pkgs.lib.optionalString noRunBench "--no-run"}
+              cargo bench ${pkgs.lib.optionalString noRun "--no-run"}
             '';
 
             doInstallCargoArtifacts = false;
@@ -386,14 +386,17 @@
           done
         '';
       };
+      arts = mkCraneArtifacts { };
 
     in
     rec {
       packages = {
-        inherit (mkCraneArtifacts { })
+        inherit (arts)
           nickel
-          benchmarks
           lsp-nls;
+
+        benchmarks = (arts.benchmarks { });
+
         default = pkgs.buildEnv {
           name = "nickel";
           paths = [ packages.nickel packages.lsp-nls ];
@@ -420,12 +423,14 @@
       };
 
       checks = {
-        inherit (mkCraneArtifacts { noRunBench = true; })
-          benchmarks
+        inherit (arts)
           clippy
           lsp-nls
           nickel
           rustfmt;
+
+        benchmarks = arts.benchmarks { noRun = true; };
+
         # An optimizing release build is long: eschew optimizations in checks by
         # building a dev profile
         nickelWasm = buildNickelWasm { profile = "dev"; };

❯ nix flake check
warning: Git tree '/home/asymmetric/code/nickel' is dirty
warning: Using saved setting for 'extra-substituters = https://tweag-nickel.cachix.org' from ~/.local/share/nix/trusted-settings.json.
warning: ignoring untrusted flake configuration setting 'extra-substituters'.
Pass '--accept-flake-config' to trust it
warning: Using saved setting for 'extra-trusted-public-keys = tweag-nickel.cachix.org-1:GIthuiK4LRgnW64ALYEoioVUQBWs0jexyoYVeLDBwRA=' from ~/.local/share/nix/trusted-settings.json.
warning: ignoring untrusted flake configuration setting 'extra-trusted-public-keys'.
Pass '--accept-flake-config' to trust it
error: value is a function while a set was expected

       at /nix/store/hap5a6iw5rccl21adfxh5b3lk2c8qnmj-source/pkgs/build-support/mkshell/default.nix:19:49:

           18|     (attrs.${name} or [ ]) ++
           19|     (lib.subtractLists inputsFrom (lib.flatten (lib.catAttrs name inputsFrom)));
             |                                                 ^
           20|
(use '--show-trace' to show detailed location information)

Seems to be caused by benchmarks being a function.

[yannham]

Ah, found the issue:

nickel/flake.nix

Line 243 in 68a7ab0

inputsFrom = builtins.attrValues (mkCraneArtifacts { inherit rust; });

We could solve it but it's starting to drag us away from the original goal of this PR. Sorry for the ping-pong. Let's do as in your previous attempt, and we'll see later about this function stuff. In any case the current flake already applies mkCraneArtifacts several times, so this PR doesn't make anything worse in this respect.

[asymmetric]

This is here because we don't need any of the outputs.

[yannham]

Thanks again! I guess the next step would be the make an output for typechecking the bench sources as well.