build-support/rust: add cargoPurityFlag parameter #187838

ghost · 2022-08-22T09:08:55Z

Description of changes

Builds within a derivation must not access the network. Cargo offers two different flags to achieve this:

--frozen, which we currently use.
--offline, which is less picky about the Cargo.lock file.

Both of these prevent access to the network. However, adding a [patch] section to your Cargo.toml will cause cargo build --frozen to fail with the following error message:

If you want to try to generate the lock file without accessing the network, remove the --frozen flag and use --offline instead.

In order for nixpkgs users to be able to take advantage of Cargo [patch] stanzas, we need to provide a way for them to request --offline be used instead of --frozen. This commit adds a cargoPurityFlag parameter which does that.

Simply changing --frozen to --offline across the board seemed too risky to me. The Cargo manual has the following warning:

Beware that this may result in different dependency resolution than online mode. Cargo will restrict itself to crates that are downloaded locally, even if there might be a newer version as indicated in the local copy of the index.

https://doc.rust-lang.org/cargo/commands/cargo-build.html?highlight=frozen#manifest-options

https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#the-patch-section

Things done

nixos-discourse · 2022-08-22T09:49:53Z

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

https://discourse.nixos.org/t/is-it-possible-to-override-cargosha256-in-buildrustpackage/4393/8

ghost · 2022-08-22T09:53:14Z

The Discourse link above is the primary motivation for this. Here is a somewhat-messy example of the case where cargoPurityFlag is needed.

nixos-discourse · 2022-08-22T18:39:30Z

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

https://discourse.nixos.org/t/rust-development-on-nixos/20866/5

Cargo-driven builds within a Rust derivation must not access the network. Cargo offers two different flags to achieve this: * `--frozen`, which we currently use. * `--offline`, which is less picky about the `Cargo.lock` file. Both of these prevent access to the network. However, adding a `[patch]` section to your `Cargo.toml` will cause `cargo build --frozen` to fail, whereas `cargo build --offline` will still succeed. In order for nixpkgs users to be able to take advantage of Cargo `[patch]` stanzas, we need to provide a way for them to requrest `--offline` be used instead of `--frozen`. This commit adds a `cargoPurityFlag` parameter which does that. Simply changing `--frozen` to `--offline` across the board seemed too risky to me. The Cargo manual has the following warning: > Beware that this may result in different dependency resolution than > online mode. Cargo will restrict itself to crates that are > downloaded locally, even if there might be a newer version as > indicated in the local copy of the index. https://doc.rust-lang.org/cargo/commands/cargo-build.html?highlight=frozen#manifest-options https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#the-patch-section

ghost · 2022-12-09T07:03:29Z

Rebased.

pkgs/build-support/rust/hooks/default.nix

As suggested in #187838 (comment)

zowoq · 2022-12-10T10:50:44Z

pkgs/build-support/rust/build-rust-package/default.nix

@@ -44,6 +44,11 @@
 , useNextest ? false
 , depsExtraArgs ? {}

+# You must use "--offline" instead of "--frozen" if `Cargo.lock` is
+# valid-but-outdated, for example when adding a `[patch]` stanza to
+# `Cargo.toml` without regenerating `Cargo.lock`.


If you're adding a patch stanza why can't you regenerate the lock at the same time?

For that matter why use a patch stanza? Patch toml and lock, add to cargoPatches.

Because you're adding the patch stanza from inside a derivation!

This lets us do for crate dependencies of a buildRustPackage what we can already do for ordinary packages in nixpkgs -- override a specific dependency throughout all of nixpkgs.

My own motivating example case is the ring crate, which is basically "openssl/boringssl for Rust". It's a deep, deep dependency of a huge chunk of the ecosystem. Ring does not build on PowerPC, and the Ring maintainer has ignored a patch submitted by IBM for over two full years. There needs to be a way for people to route around this sort of nonsense.

But this is generally useful on all platforms, for the same reasons why .override is useful. At least until crate2nix conquers the universe.

Here is what the example usage looks like:

78b4b5b

This shows what "replace this crate with that crate in this one package" looks like:

https://github.com/NixOS/nixpkgs/blob/78b4b5bf68d41f412944a2ee846715dfb090d045/pkgs/applications/networking/irc/tiny/default.nix#L26-L29

I'm working on a more general "replace this crate with that crate in every use of buildRustPackage".

This lets us do for crate dependencies of a buildRustPackage what we can already do for ordinary packages in nixpkgs -- override a specific dependency throughout all of nixpkgs.

How does that work?

For the example you posted you can do this:

Patch toml and lock, add to cargoPatches.

zowoq · 2022-12-10T11:14:28Z

I'm very, very hesitant on letting this into nixpkgs. We're using frozen for a reason, I don't see that we want to let people switch packages to offline as they'll never be switched back.

ghost · 2022-12-10T11:19:54Z

I don't see that we want to let people switch packages to offline as they'll never be switched back.

The motivating use is [patch] stanzas, which can't be used inside a derivation without --offline.

If the [patch] mapping were passed as an argument to buildRustPackage, and it used --offline if and only if the [patch] mapping is non-empty, would that resolve your concern? Then people would be unable to use --offline except in situations where --frozen doesn't work at all.

zowoq · 2022-12-10T11:37:10Z

would that resolve your concern

Somewhat.

he motivating use is [patch] stanzas

Why is using somedep = { git = "https://github.com/fork/repo", rev = "abc123" } and adding the toml and lock to cargoPatches unsuitable?

zowoq · 2022-12-10T11:57:36Z

We want people to use cargoPatches for packages in nixpkgs, I really don't see how we'll be able to avoid people bypassing cargoPatches with this.

ghost · 2022-12-10T12:22:57Z

would that resolve your concern

Somewhat.

I will prepare a concrete proposal that will limit the use of --offline to builds that use [patch] stanzas. I hope you can take the time to review it. Marking this as draft in the meantime.

he motivating use is [patch] stanzas

Why is using somedep = { git = "https://github.com/fork/repo", rev = "abc123" } and adding the toml and lock to cargoPatches unsuitable?

Because you have to regenerate the Cargo.lock file; that has two problems:

Regenerating Cargo.lock files is an extremely-impure, extremely-not-hermetic operation. It depends on the state of crates.io at the instant you regenerate it. It isn't reproducible.
There are 962 packages in nixpkgs that use buildRustPackage. If even 10% of them depend on ring (likely to be true soon if not already due to rustls) then that's ~96 lockfiles to regenerate, and 96 cargoPatch files. I don't think we want to clutter up nixpkgs with that...

We want people to use cargoPatches for packages in nixpkgs

For patching a "leaf crate" ([bin] crate), sure, absolutely.

However I don't think we want people using cargoPatches for widely-depended-upon library crates. The amount of cargoPatching needed explodes combinatorially.

zowoq · 2022-12-10T12:44:11Z

I will prepare a concrete proposal that will limit the use of --offline to builds that use [patch] stanzas.

At this stage I don't see that this would change my mind, frankly I think merging it will make nixpkgs worse off overall.

With more complicated cases like treewide patching it may be more compelling but even after you update it with your proposal this PR is only a first step towards that.

I really don't think this should be merged until we can review the entire thing.

I realize you may not want to put in all of that work in without some assurance that it will be merged but I don't see that I can offer that.

github-actions bot added the 6.topic: rust label Aug 22, 2022

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 2501-5000 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Aug 22, 2022

ghost marked this pull request as ready for review August 22, 2022 09:53

ghost requested review from zowoq, Mic92 and LnL7 as code owners August 22, 2022 09:53

ofborg bot added 10.rebuild-darwin: 5001+ and removed 10.rebuild-darwin: 2501-5000 labels Dec 9, 2022

SuperSandro2000 reviewed Dec 9, 2022

View reviewed changes

pkgs/build-support/rust/hooks/default.nix Outdated Show resolved Hide resolved

Adam Joseph added 2 commits December 10, 2022 02:38

avoid exposing any new attrs in build-support/rust/build-rust-package

d70a3f0

As suggested in #187838 (comment)

fix duplicate argument

4284ebe

zowoq reviewed Dec 10, 2022

View reviewed changes

ghost marked this pull request as draft December 10, 2022 12:23

ghost mentioned this pull request Mar 7, 2023

RFC: Migrate all Rust packages to importCargoLock #217084

Closed

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2023

ghost closed this Jan 23, 2024

ghost deleted the pr/rust/cargoPurityFlag branch January 23, 2024 06:49

Artturin mentioned this pull request Jan 29, 2024

List of @amjoseph-nixpkgs Pull requests before account removal #284796

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build-support/rust: add cargoPurityFlag parameter #187838

build-support/rust: add cargoPurityFlag parameter #187838

ghost commented Aug 22, 2022 •

edited by ghost

Loading

nixos-discourse commented Aug 22, 2022

ghost commented Aug 22, 2022 •

edited by ghost

Loading

nixos-discourse commented Aug 22, 2022

ghost commented Dec 9, 2022

zowoq Dec 10, 2022 •

edited

Loading

ghost Dec 10, 2022 •

edited by ghost

Loading

ghost Dec 10, 2022 •

edited by ghost

Loading

zowoq Dec 10, 2022

zowoq Dec 10, 2022

zowoq commented Dec 10, 2022

ghost commented Dec 10, 2022

zowoq commented Dec 10, 2022 •

edited

Loading

zowoq commented Dec 10, 2022

ghost commented Dec 10, 2022

zowoq commented Dec 10, 2022

build-support/rust: add cargoPurityFlag parameter #187838

build-support/rust: add cargoPurityFlag parameter #187838

Conversation

ghost commented Aug 22, 2022 • edited by ghost Loading

Description of changes

Things done

nixos-discourse commented Aug 22, 2022

ghost commented Aug 22, 2022 • edited by ghost Loading

nixos-discourse commented Aug 22, 2022

ghost commented Dec 9, 2022

zowoq Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

ghost Dec 10, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 10, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

zowoq Dec 10, 2022

Choose a reason for hiding this comment

zowoq Dec 10, 2022

Choose a reason for hiding this comment

zowoq commented Dec 10, 2022

ghost commented Dec 10, 2022

zowoq commented Dec 10, 2022 • edited Loading

zowoq commented Dec 10, 2022

ghost commented Dec 10, 2022

zowoq commented Dec 10, 2022

ghost commented Aug 22, 2022 •

edited by ghost

Loading

ghost commented Aug 22, 2022 •

edited by ghost

Loading

zowoq Dec 10, 2022 •

edited

Loading

ghost Dec 10, 2022 •

edited by ghost

Loading

ghost Dec 10, 2022 •

edited by ghost

Loading

zowoq commented Dec 10, 2022 •

edited

Loading