Migrate Nix build from import-cargo to Crane (#963)

* Refactor flake inputs because it's been annoying me for weeks now
* Fix a long-standing bug in mk_uniftype.rs
Can be reproduced by running `cargo test --workspace`.
This was never noticed before in CI because the `--workspace` flag is not passed.
This will be addressed in the Crane migration
* Remove utilities/Cargo.lock
This is probably a fragment of the past, back when Nickel did not use Cargo workspaces.
* Fix Clippy issues in tests
* Rustfmt and clippy are no longer in preview
* Update README with Nix flake shell command
* Migrate from import-cargo to crane for Nickel
Nickel WASM build is intentionally left intact and still uses the old
import-cargo.
This is because the change is already massive, and in practice the WASM build
relies more on `wasm-pack` than Cargo (even though under the hood, `wasm-pack`
uses Cargo).

If Crane looks good, both for DX and for CI, we will consider migrating Nickel
WASM to Crane too in a future PR.
* Remove support for Rust nightly channel
* Only Nix check for Rust `stable` channel
* Fix deprecated Nix build documentation commands
* Fix completion test

HACKING.md

@@ -94,7 +94,7 @@ Both methods are described below.
 At the root of the repository:
 
 ```shell
-$ nix build .#buildWasm
+$ nix build .#nickelWasm
 $ ls result/nickel-repl
 LICENSE  package.json nickel_lang_bg.js  nickel_lang_bg.wasm [..]
 ```

README.md

@@ -149,7 +149,9 @@ highlighting and NLS.
    - **With Nix**: If you have [Nix](https://nixos.org/nix) installed:
 
      ```console
-     nix-shell shell.nix
+     nix-shell
+     # Or if you use Nix Flakes
+     nix develop
      ```
 
      to be dropped in a shell, ready to build. You can use [our binary

flake.lock

@@ -1,6 +1,45 @@
 {
   "nodes": {
+    "crane": {
+      "inputs": {
+        "flake-compat": "flake-compat",
+        "flake-utils": "flake-utils",
+        "nixpkgs": [
+          "nixpkgs"
+        ],
+        "rust-overlay": "rust-overlay"
+      },
+      "locked": {
+        "lastModified": 1669605882,
+        "narHash": "sha256-TiQtL5sUI5rp28S63v+VX25qNjcrc8Xeu+shf3g7Tj4=",
+        "owner": "ipetkov",
+        "repo": "crane",
+        "rev": "24591d5f8cc979f7b243b88a2d39da09976970ad",
+        "type": "github"
+      },
+      "original": {
+        "owner": "ipetkov",
+        "repo": "crane",
+        "type": "github"
+      }
+    },
     "flake-compat": {
+      "flake": false,
+      "locked": {
+        "lastModified": 1650374568,
+        "narHash": "sha256-Z+s0J8/r907g149rllvwhb4pKi8Wam5ij0st8PwAh+E=",
+        "owner": "edolstra",
+        "repo": "flake-compat",
+        "rev": "b4a34015c698c7793d592d66adbab377907a2be8",
+        "type": "github"
+      },
+      "original": {
+        "owner": "edolstra",
+        "repo": "flake-compat",
+        "type": "github"
+      }
+    },
+    "flake-compat_2": {
       "flake": false,
       "locked": {
         "lastModified": 1668681692,
@@ -31,6 +70,21 @@
         "type": "github"
       }
     },
+    "flake-utils_2": {
+      "locked": {
+        "lastModified": 1667395993,
+        "narHash": "sha256-nuEHfE/LcWyuSWnS8t12N1wc105Qtau+/OdUAjtQ0rA=",
+        "owner": "numtide",
+        "repo": "flake-utils",
+        "rev": "5aed5285a952e0b949eb3ba02c12fa4fcfef535f",
+        "type": "github"
+      },
+      "original": {
+        "owner": "numtide",
+        "repo": "flake-utils",
+        "type": "github"
+      }
+    },
     "gitignore": {
       "inputs": {
         "nixpkgs": [
@@ -100,7 +154,7 @@
     },
     "pre-commit-hooks": {
       "inputs": {
-        "flake-compat": "flake-compat",
+        "flake-compat": "flake-compat_2",
         "flake-utils": [
           "flake-utils"
         ],
@@ -126,14 +180,40 @@
     },
     "root": {
       "inputs": {
-        "flake-utils": "flake-utils",
+        "crane": "crane",
+        "flake-utils": "flake-utils_2",
         "import-cargo": "import-cargo",
         "nixpkgs": "nixpkgs",
         "pre-commit-hooks": "pre-commit-hooks",
-        "rust-overlay": "rust-overlay"
+        "rust-overlay": "rust-overlay_2"
       }
     },
     "rust-overlay": {
+      "inputs": {
+        "flake-utils": [
+          "crane",
+          "flake-utils"
+        ],
+        "nixpkgs": [
+          "crane",
+          "nixpkgs"
+        ]
+      },
+      "locked": {
+        "lastModified": 1667487142,
+        "narHash": "sha256-bVuzLs1ZVggJAbJmEDVO9G6p8BH3HRaolK70KXvnWnU=",
+        "owner": "oxalica",
+        "repo": "rust-overlay",
+        "rev": "cf668f737ac986c0a89e83b6b2e3c5ddbd8cf33b",
+        "type": "github"
+      },
+      "original": {
+        "owner": "oxalica",
+        "repo": "rust-overlay",
+        "type": "github"
+      }
+    },
+    "rust-overlay_2": {
       "inputs": {
         "flake-utils": [
           "flake-utils"

flake.nix

@@ -1,13 +1,25 @@
 {
-  inputs.nixpkgs.url = "nixpkgs/nixos-unstable";
-  inputs.flake-utils.url = "github:numtide/flake-utils";
-  inputs.pre-commit-hooks.url = "github:cachix/pre-commit-hooks.nix";
-  inputs.pre-commit-hooks.inputs.nixpkgs.follows = "nixpkgs";
-  inputs.pre-commit-hooks.inputs.flake-utils.follows = "flake-utils";
-  inputs.rust-overlay.url = "github:oxalica/rust-overlay";
-  inputs.rust-overlay.inputs.nixpkgs.follows = "nixpkgs";
-  inputs.rust-overlay.inputs.flake-utils.follows = "flake-utils";
-  inputs.import-cargo.url = "github:edolstra/import-cargo";
+  inputs = {
+    nixpkgs.url = "nixpkgs/nixos-unstable";
+    flake-utils.url = "github:numtide/flake-utils";
+    pre-commit-hooks = {
+      url = "github:cachix/pre-commit-hooks.nix";
+      inputs.nixpkgs.follows = "nixpkgs";
+      inputs.flake-utils.follows = "flake-utils";
+    };
+    rust-overlay = {
+      url = "github:oxalica/rust-overlay";
+      inputs.nixpkgs.follows = "nixpkgs";
+      inputs.flake-utils.follows = "flake-utils";
+    };
+    # `import-cargo` is still used for the WASM build for now, though we expect to get rid of it in the future.
+    # See https://github.com/tweag/nickel/issues/967
+    import-cargo.url = "github:edolstra/import-cargo";
+    crane = {
+      url = "github:ipetkov/crane";
+      inputs.nixpkgs.follows = "nixpkgs";
+    };
+  };
 
   nixConfig = {
     extra-substituters = [ "https://tweag-nickel.cachix.org" ];
@@ -21,6 +33,7 @@
     , pre-commit-hooks
     , rust-overlay
     , import-cargo
+    , crane
     }:
     let
       SYSTEMS = [
@@ -33,7 +46,6 @@
       RUST_CHANNELS = [
         "stable"
         "beta"
-        "nightly"
       ];
 
       forEachRustChannel = fn: builtins.listToAttrs (builtins.map fn RUST_CHANNELS);
@@ -101,171 +113,127 @@
         , rustExtensions ? [
             "rust-src"
             "rust-analysis"
-            "rustfmt-preview"
-            "clippy-preview"
+            "rustfmt"
+            "clippy"
           ]
         , channel ? "stable"
         , target ? pkgs.rust.toRustTarget pkgs.stdenv.hostPlatform
         }:
-        let
-          _rust =
-            if channel == "nightly" then
-              pkgs.rust-bin.selectLatestNightlyWith
-                (toolchain: toolchain.${rustProfile}.override {
-                  extensions = rustExtensions;
-                  targets = [ target ];
-                })
-            else
-              pkgs.rust-bin.${channel}.latest.${rustProfile}.override {
-                extensions = rustExtensions;
-                targets = [ target ];
-              };
-        in
-        pkgs.buildEnv {
-          name = _rust.name;
-          inherit (_rust) meta;
-          buildInputs = [ pkgs.makeWrapper ];
-          paths = [ _rust ];
-          pathsToLink = [ "/" "/bin" ];
-          # XXX: This is needed because cargo and clippy commands need to
-          # also be aware of other binaries in order to work properly.
-          # https://github.com/cachix/pre-commit-hooks.nix/issues/126
-          postBuild = ''
-            for i in $out/bin/*; do
-              wrapProgram "$i" --prefix PATH : "$out/bin"
-            done
-
-          '';
-        };
-
-      pre-commit-builder =
-        { rust ? mkRust { }
-        , nickel ? buildNickel { inherit rust; }
-        , isHermetic
-        }: pre-commit-hooks.lib.${system}.run {
-          src = self;
-          hooks = {
-            nixpkgs-fmt = {
-              enable = true;
-              excludes = [
-                "lsp/client-extension/default.nix"
-                "lsp/client-extension/node-env.nix"
-                "lsp/client-extension/node-packages.nix"
-              ];
-            };
-
-            rustfmt = {
-              enable = true;
-              entry = pkgs.lib.mkForce "${rust}/bin/cargo-fmt fmt -- --check --color always";
-            };
-
-            markdownlint = {
-              enable = true;
-              excludes = [
-                "notes/(.+)\\.md$"
-                "^RELEASES\\.md$"
-              ];
-            };
-
-            # The default `clippy` pre-commit-hook is too standard, hence we create our own.
-            # Compared to the default `clippy` hook, we customize the following:
-            # - custom Clippy options
-            # - in case of hermetic run (mostly used by CI), we reuse the Nixified Rust dependencies from `nickel`
-            custom-clippy =
-              let
-                allow = [
-                  "clippy::new-without-default"
-                  "clippy::match_like_matches_macro"
-                ];
-
-                allowOpts = builtins.concatStringsSep " "
-                  (builtins.map (lint: "--allow \"${lint}\"") allow);
-
-                # Clippy requires access to the crate registry because many lints require dependency information.
-                # Similarly to `buildNickel`, we support 2 modes:
-                # - when run as a shell hook, we want to be fast, so we let Cargo do incremental compilation, and download dependencies as needed.
-                #   This is mainly used by developers, who care about speed at the expense of hermeticity.
-                # - when run as a flake check, we want to be fully reproducible, so we Nixify Cargo dependency resolution.
-                #   This is mainly used by CI, where the extra time is not such a big deal.
-                offlineOpts = pkgs.lib.optionalString isHermetic "--frozen --offline";
-
-                # When running in hermetic mode, all Rust dependencies must be already present.
-                # Hence we can't just call `cargo` directly, as it would try to download dependencies.
-                # Hence we do the following:
-                # - `source ${nickel.inputDerivation}` to set the environment variables `buildInputs` and `stdenv` just like in a Nickel build.
-                #   The `buildInputs` will contain the path to Nixified Rust dependencies (a.k.a. `cargoHome`), as well as other build inputs, like `Security` for Darwin.
-                #   ⚠️ This will also completely unset `PATH`, and *update* various environment variables used in downstream scripts, like `TMPDIR`.
-                # - `source $stdenv/setup` does a lot of Nix magic, including loading in the `PATH` everything in the `buildInputs` environment variable.
-                #
-                # 💡 The other commands are needed for Darwin builds to undo some changes of `source ${nickel.inputDerivation}`.
-                #    For Linux, these extra commands are unnecessary (because their value is "updated" from `/build` to `/build`), though harmless.
-                nickelBuildInputsSetup = pkgs.lib.optionalString isHermetic ''
-                  nix_build_top=$NIX_BUILD_TOP
-                  source ${nickel.inputDerivation}
-                  NIX_BUILD_TOP=$nix_build_top
-                  TEMP=$nix_build_top
-                  TEMPDIR=$nix_build_top
-                  TMP=$nix_build_top
-                  TMPDIR=$nix_build_top
-                  source $stdenv/setup
-                '';
-              in
-              {
-                enable = true;
-                files = "\\.rs$";
-                pass_filenames = false;
-                entry =
-                  "${pkgs.writeShellScript "clippy-hook" ''
-                    ${nickelBuildInputsSetup}
-                    cargo clippy --workspace ${offlineOpts} -- --no-deps --deny warnings ${allowOpts}
-                  ''}";
-              };
+        if channel == "nightly" then
+          pkgs.rust-bin.selectLatestNightlyWith
+            (toolchain: toolchain.${rustProfile}.override {
+              extensions = rustExtensions;
+              targets = [ target ];
+            })
+        else
+          pkgs.rust-bin.${channel}.latest.${rustProfile}.override {
+            extensions = rustExtensions;
+            targets = [ target ];
           };
+
+      pre-commit-builder = { rust ? mkRust { } }: pre-commit-hooks.lib.${system}.run {
+        src = self;
+        hooks = {
+          nixpkgs-fmt = {
+            enable = true;
+            # Excluded because they are generated by Node2nix
+            excludes = [
+              "lsp/client-extension/default.nix"
+              "lsp/client-extension/node-env.nix"
+              "lsp/client-extension/node-packages.nix"
+            ];
+          };
+
+          rustfmt = {
+            enable = true;
+            # Silly test to ensure Crane's `rustfmt` check was run
+            entry = pkgs.lib.mkForce "test -d ${(mkCraneArtifacts {inherit rust;}).rustfmt}/target";
+          };
+
+          markdownlint = {
+            enable = true;
+            excludes = [
+              "notes/(.+)\\.md$"
+              "^RELEASES\\.md$"
+            ];
+          };
+
         };
+      };
 
-      buildNickel =
-        { rust ? mkRust { }, withCargoHome ? true }:
-        pkgs.stdenv.mkDerivation {
-          name = "nickel-${version}";
+      # Build the various Crane artifacts (dependencies, packages, rustfmt, clippy) for a given Rust toolchain
+      mkCraneArtifacts = { rust ? mkRust { } }:
+        let
+          craneLib = crane.lib.${system}.overrideToolchain rust;
 
-          buildInputs =
-            [ rust ]
-            # cargoHome is used for a fully, hermetic, nixified build. This is
-            # what we want for e.g. nix-build. However, when hacking on Nickel,
-            # we rather provide the necessary tooling but let people use the
-            # native way (`cargo build`), because:
-            #
-            # - we don't care as much about hermeticity when iterating quickly
-            #   over the codebase
-            # - we get incremental build and a build order of magnitudes
-            #   faster
-            # - etc.
-            ++ pkgs.lib.optional withCargoHome cargoHome
-            ++ missingSysPkgs;
+          # Customize source filtering as Nickel uses non-standard-Rust files like `*.lalrpop`.
+          src =
+            let
+              mkFilter = regexp: path: _type: builtins.match regexp path != null;
+              lalrpopFilter = mkFilter ".*lalrpop$";
+              nclFilter = mkFilter ".*ncl$";
+              txtFilter = mkFilter ".*txt$";
+            in
+            pkgs.lib.cleanSourceWith {
+              src = pkgs.lib.cleanSource ./.;
 
-          src = self;
+              # Combine our custom filters with the default one from Crane
+              # See https://github.com/ipetkov/crane/blob/master/docs/API.md#libfiltercargosources
+              filter = path: type:
+                builtins.any (filter: filter path type) [
+                  lalrpopFilter
+                  nclFilter
+                  txtFilter
+                  craneLib.filterCargoSources
+                ];
+            };
 
-          buildPhase = ''
-            cargo build --workspace --exclude nickel-repl --release --frozen --offline
-          '';
+          # Args passed to all `cargo` invocations by Crane.
+          cargoExtraArgs = "--frozen --offline --workspace";
 
-          doCheck = true;
+        in
+        rec {
+          # Build *just* the cargo dependencies, so we can reuse all of that work (e.g. via cachix) when running in CI
+          cargoArtifacts = craneLib.buildDepsOnly {
+            inherit
+              src
+              cargoExtraArgs;
+          };
 
-          checkPhase = ''
-            cargo test --release --frozen --offline
-          '';
+          nickel = craneLib.buildPackage {
+            inherit
+              src
+              cargoExtraArgs
+              cargoArtifacts;
+          };
 
-          installPhase = ''
-            mkdir -p $out
-            cargo install --frozen --offline --path . --root $out
-            cargo install --frozen --offline --path lsp/nls --root $out
-            rm $out/.crates.toml
-          '';
+          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;
+
+            # We don't reuse the `cargoExtraArgs` in scope because `cargo fmt` does not accept nor need any of `--frozen`, `--offline` or `--workspace`
+            cargoExtraArgs = "--all";
+
+            # `-- --check` is automatically prepended by Crane
+            rustFmtExtraArgs = "--color always";
+          };
+
+          clippy = craneLib.cargoClippy {
+            inherit
+              src
+              cargoExtraArgs
+              cargoArtifacts;
+
+            cargoClippyExtraArgs = "--all-targets -- --deny warnings --allow clippy::new-without-default --allow clippy::match_like_matches_macro";
+          };
 
         };
 
       makeDevShell = { rust }: pkgs.mkShell {
-        inputsFrom = [ (buildNickel { inherit rust; withCargoHome = false; }) ];
+        # Trick found in Crane's examples to get a nice dev shell
+        # See https://github.com/ipetkov/crane/blob/master/examples/quick-start/flake.nix
+        inputsFrom = builtins.attrValues (mkCraneArtifacts { inherit rust; });
+
         buildInputs = [
           pkgs.rust-analyzer
           pkgs.nodejs
@@ -273,7 +241,7 @@
           pkgs.nodePackages.markdownlint-cli
         ];
 
-        shellHook = (pre-commit-builder { inherit rust; isHermetic = false; }).shellHook + ''
+        shellHook = (pre-commit-builder { inherit rust; }).shellHook + ''
           echo "=== Nickel development shell ==="
           echo "Info: Git hooks can be installed using \`pre-commit install\`"
         '';
@@ -370,35 +338,32 @@
     in
     rec {
       packages = {
-        default = packages.build;
-        build = buildNickel { };
-        buildWasm = buildNickelWasm { };
-        dockerImage = buildDocker packages.build; # TODO: docker image should be a passthru
+        nickel = (mkCraneArtifacts { }).nickel;
+        default = packages.nickel;
+        nickelWasm = buildNickelWasm { };
+        dockerImage = buildDocker packages.nickel; # TODO: docker image should be a passthru
         inherit vscodeExtension;
         inherit userManual;
         inherit stdlibDoc;
       };
 
-      devShells = {
+      devShells = (forEachRustChannel (channel: {
+        name = channel;
+        value = makeDevShell { rust = mkRust { inherit channel; rustProfile = "default"; }; };
+      })) // {
         default = devShells.stable;
-      } // (forEachRustChannel
-        (channel: {
-          name = channel;
-          value = makeDevShell { rust = mkRust { inherit channel; rustProfile = "default"; }; };
-        }
-        ));
+      };
 
       checks = {
+        inherit (mkCraneArtifacts { })
+          nickel
+          clippy
+          rustfmt;
         # wasm-opt can take long: eschew optimizations in checks
-        wasm = buildNickelWasm { optimize = false; };
+        nickelWasm = buildNickelWasm { optimize = false; };
         inherit vscodeExtension;
-        pre-commit = pre-commit-builder { isHermetic = true; };
-      } // (forEachRustChannel (channel:
-        {
-          name = "nickel-against-${channel}-rust-channel";
-          value = buildNickel { rust = mkRust { inherit channel; }; };
-        }
-      ));
+        pre-commit = pre-commit-builder { };
+      };
     }
     );
 }

lsp/README.md

@@ -100,7 +100,7 @@ section about the Nix setup).
   - Build with Nix:
 
       ```console
-      nix build github:tweag/nickel#vscodeExtension.vsix
+      nix build github:tweag/nickel#vscodeExtension
       ```
 
   - Then, in VSCode, use "Extension: Install from VSIX" in the vscode command

lsp/client-extension/README.md

@@ -9,7 +9,7 @@ This directory contains the Visual Studio Code Nickel LSP extension code.
 From the root of the Nickel project:
 
 ```shell
-nix build .\#vscodeExtension.vsix
+nix build .\#vscodeExtension
 ```
 
 The VSIX extension will be at `./result-vsix/nls-client.vsix`.

lsp/nls/src/requests/completion.rs

@@ -527,7 +527,7 @@ mod tests {
             }
         }
 
-        let files = Files::new();
+        let mut files = Files::new();
         let file_id = files.add("test", "test");
 
         let a = make_linearization_item(

src/eval/tests.rs

@@ -186,13 +186,13 @@ fn imports() {
     // let x = import "does_not_exist" in x
     match mk_import("x", "does_not_exist", mk_term::var("x"), &mut vm).unwrap_err() {
         ImportError::IOError(_, _, _) => (),
-        _ => assert!(false),
+        _ => panic!(),
     };
 
     // let x = import "bad" in x
     match mk_import("x", "bad", mk_term::var("x"), &mut vm).unwrap_err() {
         ImportError::ParseErrors(_, _) => (),
-        _ => assert!(false),
+        _ => panic!(),
     };
 
     // let x = import "two" in x

src/typecheck/mk_uniftype.rs

@@ -51,7 +51,7 @@ macro_rules! mk_uty_row {
     (($id:expr, $ty:expr) $(,($ids:expr, $tys:expr))* $(; $tail:expr)?) => {
         $crate::typecheck::UnifRecordRows::Concrete(
             $crate::types::RecordRowsF::Extend {
-                row: $crate::typecheck::RecordRowF {
+                row: $crate::types::RecordRowF {
                     id: Ident::from($id),
                     types: Box::new($ty.into()),
                 },