Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Compile benchmarks via Nix #1119

Merged
merged 4 commits into from
Feb 16, 2023
Merged

Compile benchmarks via Nix #1119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4cdc71c
ci: compile benchmarks via nix
asymmetric Feb 13, 2023
7c9cb38
flake: separate benchmark compilation and running
asymmetric Feb 13, 2023
f55e6f4
flake: clean up benchmarks invocation
asymmetric Feb 15, 2023
bd88092
flake: remove doCheck in benchmarks package
asymmetric Feb 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 2 additions & 6 deletions .github/workflows/continuous-integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,5 @@ jobs:
run: |
nix flake check --print-build-logs

- name: Compile and typecheck benchmarks
run: |
nix develop
cargo bench --no-run

find benches -type f -name "*.ncl" -print0 | xargs -0 -I file nix run . -- -f file typecheck
- name: Typecheck benchmarks
run: find benches -type f -name "*.ncl" -print0 | xargs -0 -I file nix run . -- -f file typecheck
22 changes: 18 additions & 4 deletions flake.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@

# Given a rust toolchain, provide Nickel's Rust dependencies, Nickel, as
# well as rust tools (like clippy)
mkCraneArtifacts = { rust ? mkRust { } }:
mkCraneArtifacts = { rust ? mkRust { }, noRunBench ? false }:
let
craneLib = crane.lib.${system}.overrideToolchain rust;

Expand Down Expand Up @@ -217,6 +217,18 @@
nickel = buildPackage "nickel-lang";
lsp-nls = buildPackage "nickel-lang-lsp";

benchmarks = craneLib.mkCargoDerivation {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

inherit src cargoArtifacts;

pnameSuffix = "-bench";

buildPhaseCargoCommand = ''
cargo bench ${pkgs.lib.optionalString noRunBench "--no-run"}
'';

doInstallCargoArtifacts = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

rustfmt = craneLib.cargoFmt {
# Notice that unlike other Crane derivations, we do not pass `cargoArtifacts` to `cargoFmt`, because it does not need access to dependencies to format the code.
inherit src;
Expand Down Expand Up @@ -380,6 +392,7 @@
packages = {
inherit (mkCraneArtifacts { })
nickel
benchmarks
lsp-nls;
default = pkgs.buildEnv {
name = "nickel";
Expand Down Expand Up @@ -407,10 +420,11 @@
};

checks = {
inherit (mkCraneArtifacts { })
nickel
lsp-nls
inherit (mkCraneArtifacts { noRunBench = true; })
benchmarks
clippy
lsp-nls
nickel
rustfmt;
# An optimizing release build is long: eschew optimizations in checks by
# building a dev profile
Expand Down