rustPlatform.buildRustPackage: support direct use of Cargo.lock

[danieldk]

Motivation for this change

This change introduces the cargoLock argument to buildRustPackage,
which can be used in place of cargo{Sha256,Hash} or cargoVendorDir. It
uses the importCargoLock function to build the vendor
directory. Differences compared to cargo{Sha256,Hash}:

• Requires a Cargo.lock file.
• Does not require a Cargo hash.
• Retrieves all dependencies as fixed-output derivations.

This makes buildRustPackage much easier to use as part of a Rust
project, since it does not require updating cargo{Sha256,Hash} for
every change to the lock file.

To introduce this functionality, the importCargoLock function was
also added. This function can be used to create an output path that is
a cargo vendor directory. In contrast to e.g. fetchCargoTarball all
the dependent crates are fetched using fixed-output derivations. The
hashes for the fixed-output derivations are gathered from the
Cargo.lock file.

Usage is very simple, e.g.:

importCargoLock {
  lockFile = ./Cargo.lock;
}

would use the lockfile from the current directory.

The implementation of this function is based on Eelco Dolstra's
import-cargo:

https://github.com/edolstra/import-cargo/blob/master/flake.nix

Compared to cargo-import:

• We use fetchgit in place of builtins.fetchGit. This requires specifying output hashes, which can be done through the outputHash attribute (see the documentation). Since crates.io does not allow uploads with non-crates.io dependencies, git dependencies do not occur much in practive.
• Sync to current cargo vendoring.

cc @Ma27, @edolstra

Related issue: #89563

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[andir]

This is the right direction. Less abuse of FODs for fetching a random set of dependencies.

If we merge this in we need at least one test that checks that the feature isn't broken. Any small packaging test might do as long as it makes use of outputHash and whatever else we might end up with.

[Mic92]

I also agree with this approach but would let the review up to @andir as he has way more experience of subtile implementation details of Rust's dependency resolution.

[danieldk]

If we merge this in we need at least one test that checks that the feature isn't broken. Any small packaging test might do as long as it makes use of outputHash and whatever else we might end up with.

Added three tests:

• A Rust crate with a crates.io dependency
• A Rust crate with a git dependency with no rev specified in Cargo.toml.
• A Rust crate with a git dependency with a rev specified in Cargo.toml.

I used rand as the dependency, since it it is part of a Cargo workspace and uses dependencies from the same workspace.

To avoid that a user has to specify the hashes for all the dependencies from a workspace crate, I changed importCargoLock such that when a user specifies:

outputHash = {
  "rand-0.8.3" = "sha256-CnMJWpSnU6slmCJhbxaXq0ikxVZDk4SQwmFYNlSEQnk=";
};

rand-0.8.3 is mapped to its git commit SHA for rand-0.8.3 from the lockfile. Each git dependency that uses the same git commit SHA will then use the same output hash. I think this is more ergonomic than asking the user to use something like

outputHash = {
  "f0e01ee0a7257753cc51b291f62666f4765923ef" = "sha256-CnMJWpSnU6slmCJhbxaXq0ikxVZDk4SQwmFYNlSEQnk=";
};

[andir]

In general it looks nice. I haven't tested it yet and thus there is no +1. I left a few minor remarks.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/why-dont-nix-hashes-use-base-16/11325/27

[danieldk]

In general it looks nice. I haven't tested it yet and thus there is no +1. I left a few minor remarks.

Did you find the time to test this?

[andir]

In general it looks nice. I haven't tested it yet and thus there is no +1. I left a few minor remarks.

Did you find the time to test this?

I just tried to convert a random package (in a hacky read from derivation way) from nixpkgs to this. I picked bitwarden_rs and applied this patch:

diff --git a/pkgs/tools/security/bitwarden_rs/default.nix b/pkgs/tools/security/bitwarden_rs/default.nix
index 5676e4005c1..da18f9fa58d 100644
--- a/pkgs/tools/security/bitwarden_rs/default.nix
+++ b/pkgs/tools/security/bitwarden_rs/default.nix
@@ -17,6 +17,15 @@ in rustPlatform.buildRustPackage rec {
     sha256 = "1ncy4iwmdzdp8rv1gc5i4s1rp97d94n4l4bh08v6w4zdpx0zn8b9";
   };

+  cargoLock = {
+    lockFile = src + "/Cargo.lock";
+    outputHash = {
+      "data-url-0.1.0" = "1lx660kx4143nppjc7cyfbhmmvy4ifljvb2imcqdxkqbqfaj7sbq";
+      "devise-0.3.0" = "0li9n5pviky3i4sb4h7p78vydgbx1nl3hrlynf42n5mvl8wc9fbj";
+      "rocket-0.5.0-dev" = "19lyljqhi6xszg86zl6wzd14bnjdb56dkcgqg2s3qz7bs8dgm4mi";
+    };
+  };
+
   nativeBuildInputs = [ pkg-config ];
   buildInputs = with lib; [ openssl ]
     ++ optionals stdenv.isDarwin [ Security CoreServices ]
@@ -25,7 +34,7 @@ in rustPlatform.buildRustPackage rec {

   RUSTC_BOOTSTRAP = 1;

-  cargoSha256 = "139by5y2ma3v52nabzr5man1qy395rchs2dlivkj9xi829kg4mcr";
+  # cargoSha256 = "139by5y2ma3v52nabzr5man1qy395rchs2dlivkj9xi829kg4mcr";
   cargoBuildFlags = [ featuresFlag ];

   checkPhase = ''

but it fails like this:

building '/nix/store/mhqcy8lx04ikyhvs4sx3y8mwpb61fk29-bitwarden_rs-1.20.0.drv'...
unpacking sources
unpacking source archive /nix/store/a02x61417w9yq2c3lf9c995azpk0ck22-source
source root is source
Executing cargoSetupPostUnpackHook
unpacking source archive /nix/store/nlhb6r908hj0skrhifqd4wk51l0n3gb8-cargo-vendor-dir
Finished cargoSetupPostUnpackHook
patching sources
Executing cargoSetupPostPatchHook
Validating consistency between /build/source//Cargo.lock and /build/cargo-vendor-dir/Cargo.lock
Finished cargoSetupPostPatchHook
configuring
building
Executing cargoBuildHook
++ env CC_x86_64-unknown-linux-gnu=/nix/store/zzvq5qwlm2xikawfqxb0q8gl2bw391a9-gcc-wrapper-10.2.0/bin/cc CXX_x86_64-unknown-linux-gnu=/nix/store/zzvq5qwlm2xikawfqxb0q8gl2bw391a9-gcc-wrapper-10.2.0/bin/c++ CC_x86_64-unknown-linux-gnu=/nix/store/zzvq5qwlm2xikawfqxb0q8gl2bw391a9-gcc-wrapper-10.2.0/bin/cc CXX_x86_64-unknown-linux-gnu=/nix/store/zzvq5qwlm2xikawfqxb0q8gl2bw391a9-gcc-wrapper-10.2.0/bin/c++ cargo build -j 24 --target x86_64-unknown-linux-gnu --frozen --release --features sqlite
error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`

Caused by:
  failed to load source for dependency `data-url`

Caused by:
  Unable to update https://github.com/servo/rust-url?rev=540ede02d0771824c0c80ff9f57fe8eff38b1291#540ede02

Caused by:
  failed to update replaced source https://github.com/servo/rust-url?rev=540ede02d0771824c0c80ff9f57fe8eff38b1291#540ede02

Caused by:
  found a virtual manifest at `/build/cargo-vendor-dir/devise_codegen-0.3.0/Cargo.toml` instead of a package manifest

Should it be this way?

[danieldk]

Should it be this way?

Looks good. The issue is in workspaces where crates are not top-level directories. I have it building now, but I need to clean up the changes.

[danieldk]

Should it be this way?

Looks good. The issue is in workspaces where crates are not top-level directories. I have it building now, but I need to clean up the changes.

@andir this should work now. For crates from workspaces cargo metadata is now used to find the correct path of the crate within the workspace.

[danieldk]

I think that this should already work with the hookification of buildRustPackage that I did earlier this year. cargo-setup-hook picks up the cargoDeps in multi-language derivations and importCargoLock should work as a drop-in replacement for fetchCargoTarball. But I’ll test this tomorrow to ensure that it works properly.

Works with the hooks as expected. I have added a Rust/Python project using buildPythonPackage as an additional test case:

https://github.com/NixOS/nixpkgs/blob/ff470c3a09fe95673108c9d7e3ad31a12d61d5ca/pkgs/build-support/rust/test/import-cargo-lock/maturin/default.nix#L20

Added this to the documentation now as well.

[andir]

From my point of view this looks good to go in. I was about to mention the setup hook but that has already been taken care of 👍.

If @alyssais is happy with the setup hook then lets merge this!

[danieldk]

@alyssais any objections against merging this?

[ckauhaus]

This feature would be a significant step forward in packaging usability.

[danieldk]

I would hate for this to get stuck, so unless there are objections, I'll merge this at Friday 28, 12:00 CET.

[asymmetric]

This would be great to have. Left a couple of (mostly stylistic) comments.

[danieldk]

@ofborg build tests.importCargoLock

[danieldk]

Thanks for all the reviews!

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nvfetcher-generate-nix-sources-expr-for-the-latest-version-of-packages/13530/3

[bergkvist]

A pattern I find myself using when some upstream project doesn't have a Cargo.lock, and I have to provide my own:

# ...
pkgs.rustPlatform.buildRustPackage {
  # ...
  cargoLock.lockFile = ./Cargo.lock;
  cargoPatches = [
    (pkgs.runCommand "Cargo.lock.patch" { buildInputs = [ pkgs.git ]; } ''
      cp ${./Cargo.lock} Cargo.lock
      git init && git add Cargo.lock
      git diff --cached > $out
    '')
  ];
}

That way I don't need to manually provide the Cargo.lock patch. Maybe this patch generation could be automated in rustPlatform.buildRustPackage when cargoLock.lockFile is specified?

If I omit the cargoPatches entry, I get an error message during the build:

Executing cargoSetupPostPatchHook
Validating consistency between /build/source/Cargo.lock and /build/cargo-vendor-dir/Cargo.lock
/nix/store/49y3r0gr9m6k20d91kl6dgp4b9a6m72v-diffutils-3.8/bin/diff: /build/source/Cargo.lock: No such file or directory
ERROR: Missing Cargo.lock from src. Expected to find it at: /build/source/Cargo.lock
Hint: You can use the cargoPatches attribute to add a Cargo.lock manually to the build.

[Mic92]

Probably it shouldn't be even apply a patch but just copy the whole thing over?