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.

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

rustPlatform.buildRustPackage: support direct use of Cargo.lock #122158

Merged
merged 3 commits into from
May 28, 2021

Conversation

danieldk
Copy link
Contributor

@danieldk danieldk commented May 8, 2021

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.

Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

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.

pkgs/build-support/rust/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/rust/import-cargo-lock.nix Outdated Show resolved Hide resolved
@Mic92
Copy link
Member

Mic92 commented May 8, 2021

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
Copy link
Contributor Author

danieldk commented May 8, 2021

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=";
};

@danieldk danieldk requested a review from andir May 8, 2021 09:46
Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

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

doc/languages-frameworks/rust.section.md Outdated Show resolved Hide resolved
pkgs/build-support/rust/import-cargo-lock.nix Outdated Show resolved Hide resolved
@danieldk danieldk force-pushed the import-cargo-lock branch 2 times, most recently from f3540bb to d6e6ccb Compare May 10, 2021 13:01
@danieldk danieldk requested a review from andir May 12, 2021 08:42
@nixos-discourse
Copy link

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
Copy link
Contributor Author

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
Copy link
Member

andir commented May 17, 2021

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
Copy link
Contributor Author

danieldk commented May 17, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@alyssais any objections against merging this?

@NixOS NixOS deleted a comment from nixos-discourse May 22, 2021
@ckauhaus
Copy link
Contributor

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

@danieldk
Copy link
Contributor Author

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

Copy link
Contributor

@asymmetric asymmetric left a comment

Choose a reason for hiding this comment

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

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

doc/languages-frameworks/rust.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/rust.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/rust.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/rust.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/rust.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/rust.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/rust.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/rust.section.md Outdated Show resolved Hide resolved
pkgs/build-support/rust/import-cargo-lock.nix Show resolved Hide resolved
pkgs/build-support/rust/import-cargo-lock.nix Show resolved Hide resolved
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 upstream:

- We use fetchgit in place of builtins.fetchGit.
- Sync to current cargo vendoring.
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.
@danieldk
Copy link
Contributor Author

@ofborg build tests.importCargoLock

@danieldk danieldk merged commit 1da0b1d into NixOS:master May 28, 2021
@danieldk danieldk deleted the import-cargo-lock branch May 28, 2021 10:07
@danieldk
Copy link
Contributor Author

Thanks for all the reviews!

@nixos-discourse
Copy link

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

@asymmetric asymmetric mentioned this pull request Jun 28, 2021
11 tasks
zhaofengli added a commit to zhaofengli/colmena that referenced this pull request Aug 26, 2021
Support for directly passing the Cargo.lock was added in
<NixOS/nixpkgs#122158>.
@bergkvist
Copy link
Member

bergkvist commented Jan 12, 2023

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
Copy link
Member

Mic92 commented Jan 13, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants