rustPlatform.buildRustPackage: support finalAttrs style

[amesgen]

Description of changes

This PR explores supporting the new finalAttrs derivation style for buildRustPackage, similar to what is suggested in #119942 (comment). It is basically an extended version of #179392, and in particular also would close #107070.

A few notes:

• This change should be entirely opt-in and cause no rebuilds.

• This PR adds an ad-hoc way to prevent some attrs (called "intermediate args" here) to be passed to the underlying derivation call by specifying removeFromBuilderEnv. Another option would be to allow an arbitrary function (similar to apply in the module system), but that might be too powerful/confusing. Also see rustPlatform.buildRustPackage: support finalAttrs style #194475 (comment).

• Rust packages can be incrementally converted and then enjoy the benefits outlined in stdenv.mkDerivation: overlay style overridable recursive attributes #119942

  As an example, I converted difftastic to the new style, such that changing its version is now much DRYer:

  pkgs.difftastic.overrideAttrs (oldAttrs: {
    version = "0.37.0";
    src = oldAttrs.src.overrideAttrs (_: {
      outputHash = "sha256-LwDoIvhZj/1fHg2eWgadwTcegeOKPpY8aCAugLfKtDE=";
    });
    cargoHash = "sha256-j7PVzGCButQhxVXVrtWhT6a6F1SLNe0jAy8oGqr9NvQ=";
    cargoLock = null;
    postPatch = "";
  })   

  Also note that passthru.tests.version then automatically points to the new package.

• The "overlay" in buildRustPackage has a few curiosities, i.e. we have to be careful to avoid infinite recursion errors:

  • It is not possible to use the { a ? "", ...}@finalAttrs syntax. Even {...}@finalAttrs causes infinite recursion.
  • Asserts involving attributes from finalAttrs have to be "hidden" inside of values. Right now, I moved them to the respective attributes/variables; another option would be to put them into a dedicated asserts attribute, i.e. asserts = assert (all asserts here); "";.

  Maybe there are better solutions for these problems around, but they don't seem too terrible.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[farcaller]

I came to this from #107070 and I must say that the fact that I can actually see and fix the cargoSha256 hash mismatches in overrides is a major UX improvement.

[ghost]

Hey, I'm totally in favor of this PR, but the way it's written makes it extremely difficult to review!

Could you please search-and-replace finalAttrs with args and previousAttrs with args? That alone will cut the size of the diff in half.

Then please add a let args = { src = args.src or null; ... } to rebind (shadow) args. That will cut the diff down to only a dozen lines changed or so -- way easier to review.

Thanks,

[ghost]

Could we pick a more descriptive name here? Once chosen, it's hard to change this.

The purpose of this is to indicate which attributes should not become part of the builder's environment. So we should pick a name that makes that purpose obvious to the reader without having to open make-derivation.nix in order to figure it out. Something like removeFromBuilderEnv or filterFromDerivation or notEnvVars or something else entirely.

[amesgen]

Good point, will go with removeFromBuilderEnv for now 👍

[ghost]

Technically an overlay is a list of overrides. This is just an override.

[amesgen]

According to the nixpkgs manual:

Overlays are Nix functions which accept two arguments, conventionally called self and super, and return a set of packages.

rustOverlay doesn't fulfill the "set of packages" criterion, so using sth more vague seems sensible, will change 👍

[ghost]

In the future, for big refactors like this, it would make them easier to review if you "rebuilt" the args attrset up at the top of the file (taking values from finalAttrs instead of args of course). Then you could make tiny easy-to-approve changes to lines like this, simply inserting (argsRebuilt) after inherit.

[amesgen]

Yeah, you raise a good point here and in #194475 (review) above. I can rewrite it in the style you are suggesting here, as there are no existing review threads etc. that would be invalidated by this. WDYT?

[ghost]

Your call. The cleanup in your d16ba39 was enough that I was able to get through the diff without losing track of what was going on.

[amesgen]

Ok, then I will leave it is as is for now (unless someone else objects) -- I ran into difficult-to-debug "infinite recursion" errors when trying to refactor this, which I like to avoid debugging unless necessary 😄

[ghost]

+2 We should merge this. I'd like to use it in #204694 and elsewhere.

Ping @roberth (who introduced mkDerivation (finalAttrs:) and @Ericson2314 (the build-support/rust guru) to see if they have any objections.

This produced no post-eval changes on either OfBorg or my system (which has a lot of overlays) for x86+arm+powerpc full workstation, mips router-only. So I feel confident it won't break things. If it does you can blame me.

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/1607

[winterqt]

Poke @roberth ^^

[SuperSandro2000]

Can we move the assserts after the inputs? thats a bit cleaner than inlining them.

[amesgen]

Due to the evaluation model of finalAttrs, the assertions can not be kept in the place they were before. Citing from the PR description:

Asserts involving attributes from finalAttrs have to be "hidden" inside of values. Right now, I moved them to the respective attributes/variables; another option would be to put them into a dedicated asserts attribute, i.e. asserts = assert (all asserts here); "";.

[roberth]

Just small suggestions; already looks good.

[roberth]

👍 👍

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/1704

[jtojnar]

With __structuredAtts the arguments passed to mkDerivation will not end up in the environment so this will become redundant.

[roberth]

They would still become part of the JSON and part of the hash, causing rebuilds.
For instance, changes to cargoUpdateHook were previously masked by the FOD hash, but this reintroduces that value as an unnecessary input dependency that will cause a rebuild even if the FOD hash didn't have to change.

Similarly, you may want to have a "local" variable that only affects the passthru attributes, but isn't a passthru itself; perhaps some configuration for an automatically generated updateScript. That could only be expressed by filtering away at least one more attribute.

That said, perhaps the name could be improved by removing the Env part; what about removeFromBuilder?

It's not a pretty solution, but it's the best we can do when we don't have an alternative place for such attributes. We could have a single attribute like locals specifically for this purpose, but that doesn't help with attributes that already exist, and I don't like that it exposes an implementation detail to users. "Local" should be the default behavior of attributes at the root of the argument, but at that point it's not mkDerivation anymore.

[jtojnar]

Rather than passing the buildRustPackage to mkDerivation and then removing them with removeFromBuilderEnv. You could remove them here and then pass them to rustOverride.

diff --git a/pkgs/build-support/rust/build-rust-package/default.nix b/pkgs/build-support/rust/build-rust-package/default.nix
index b7ae02daae0..23ae8f6b6ac 100644
--- a/pkgs/build-support/rust/build-rust-package/default.nix
+++ b/pkgs/build-support/rust/build-rust-package/default.nix
@@ -19,13 +19,14 @@
 
 let rustOverride =
 
+rustAttrs:
 finalAttrs:
 
 let
   cargoPatches = finalAttrs.cargoPatches or [];
   buildType = finalAttrs.buildType or "release";
   checkType = finalAttrs.checkType or buildType;
-  cargoLock = finalAttrs.cargoLock or null;
+  cargoLock = rustAttrs.cargoLock or null;
   cargoVendorDir = finalAttrs.cargoVendorDir or null;
   buildNoDefaultFeatures = finalAttrs.buildNoDefaultFeatures or false;
   checkNoDefaultFeatures = finalAttrs.checkNoDefaultFeatures or buildNoDefaultFeatures;
@@ -33,7 +34,7 @@ let
   checkFeatures = finalAttrs.checkFeatures or buildFeatures;
   useNextest = finalAttrs.useNextest or false;
   auditable = finalAttrs.auditable or false; # TODO: change to true
-  depsExtraArgs = finalAttrs.depsExtraArgs or { };
+  depsExtraArgs = rustAttrs.depsExtraArgs or { };
 
   # Toggles whether a custom sysroot is created when the target is a .json file.
   __internal_dontAddSysroot = finalAttrs.__internal_dontAddSysroot or false;
@@ -57,7 +58,7 @@ let
       preUnpack = finalAttrs.preUnpack or null;
       unpackPhase = finalAttrs.unpackPhase or null;
       postUnpack = finalAttrs.postUnpack or null;
-      cargoUpdateHook = finalAttrs.cargoUpdateHook or "";
+      cargoUpdateHook = rustAttrs.cargoUpdateHook or "";
       # Name for the vendored dependencies tarball
       name = finalAttrs.cargoDepsName or finalAttrs.name or "${finalAttrs.pname}-${finalAttrs.version}";
       patches = cargoPatches;
@@ -85,7 +86,6 @@ let
 
 in
 
-
 previousAttrs:
 
 lib.optionalAttrs useSysroot {
@@ -95,7 +95,6 @@ lib.optionalAttrs useSysroot {
     information.
   '' ("--sysroot ${sysroot} " + (finalAttrs.RUSTFLAGS or ""));
 } // {
-  removeFromBuilderEnv = [ "depsExtraArgs" "cargoUpdateHook" "cargoLock" ];
 
   inherit cargoDeps;
 
@@ -159,4 +158,9 @@ lib.optionalAttrs useSysroot {
   } // (previousAttrs.meta or {});
 };
 
-in args: (stdenv.mkDerivation args).overrideAttrs rustOverride
+in
+args:
+let
+  derivationArgs = finalAttrs: builtins.removeAttrs (if builtins.isFunction finalAttrs then args finalAttrs else args) [ "depsExtraArgs" "cargoUpdateHook" "cargoLock" ];
+in
+(stdenv.mkDerivation derivationArgs).overrideAttrs (rustOverride args)

Edit: Getting infinite recursion 😞 Would be nice to have NixOS/nix#4154

[amesgen]

Yeah, accidentally running into infinite recursions also happened to me a few times 😄

[jtojnar]

Resolved the conflict in master...jtojnar:nixpkgs:buildRustPackage-finalAttrs

[SuperSandro2000]

Suggested change

	repo = finalAttrs.pname;
	repo = "difftastic";

this is a bit problematic because if someone appends something to pname like -unwrapped than the download link silently breaks because of FODs.

[roberth]

It's worse: it wouldn't even break for you but for someone else, because - having the output - you don't need to re-run the FOD.

D.R.Y: do repeat yourself ;)

[amesgen]

Side effect of #221093. This is not ideal; I guess one solution would be to accept the rebuilds, but maybe there is a different approach here.

[roberth]

Attributes with null values are completely removed from the .drv, so this doesn't look like a mass rebuild to me.

[amesgen]

Here is a concrete example:

With this PR in its current state (48e9de7) as well as before this PR (fe2ecaf):

 $ nix-instantiate -A cargo
/nix/store/1977acij72524vy0c8ij19ng7nzrf6yv-cargo-1.68.2.drv

After applying this diff

diff --git a/pkgs/build-support/rust/build-rust-package/default.nix b/pkgs/build-support/rust/build-rust-package/default.nix
index f8e1eef2e93..6ca8a730d59 100644
--- a/pkgs/build-support/rust/build-rust-package/default.nix
+++ b/pkgs/build-support/rust/build-rust-package/default.nix
@@ -58,7 +58,7 @@ let
       unpackPhase = finalAttrs.unpackPhase or null;
       # TODO using previousAttrs here as we otherwise trigger rebuilds for all
       # FOD fetcher users as finalAttrs.postUnpack is prefixed below.
-      postUnpack = previousAttrs.postUnpack or null;
+      postUnpack = finalAttrs.postUnpack or null;
       cargoUpdateHook = finalAttrs.cargoUpdateHook or "";
       # Name for the vendored dependencies tarball
       name = finalAttrs.cargoDepsName or finalAttrs.name or "${finalAttrs.pname}-${finalAttrs.version}";

we get

 $ nix-instantiate -A cargo
/nix/store/3msgx9jda9kc6hvfl7vgf9b8nsbm5g6y-cargo-1.68.2.drv

Diff:

 $ nix-diff /nix/store/1977acij72524vy0c8ij19ng7nzrf6yv-cargo-1.68.2.drv /nix/store/3msgx9jda9kc6hvfl7vgf9b8nsbm5g6y-cargo-1.68.2.drv
- /nix/store/1977acij72524vy0c8ij19ng7nzrf6yv-cargo-1.68.2.drv:{out}
+ /nix/store/3msgx9jda9kc6hvfl7vgf9b8nsbm5g6y-cargo-1.68.2.drv:{out}
• The input derivation named `cargo` differs
  - /nix/store/qcsj2rh9k0dgig555kb12vx6zrzsw2nr-cargo.drv:{out}
  + /nix/store/fmvz39xbarfgssly8j6lzjp5dcxpgwxb-cargo.drv:{out}
  • The input derivation named `cargo-auditable-0.6.1` differs
    - /nix/store/7gkiqcx8qigih6pzga18ps6w5dpv3n33-cargo-auditable-0.6.1.drv:{out}
    + /nix/store/lnxzsh3ij88gp0vzj0628p1dn4gd2irj-cargo-auditable-0.6.1.drv:{out}
    • The input derivation named `cargo-auditable-0.6.1-vendor.tar.gz` differs
      - /nix/store/n67n6kbyd0ch4v88wwra9fy57p6mr263-cargo-auditable-0.6.1-vendor.tar.gz.drv:{out}
      + /nix/store/wkvcmy569xq0lgwbfvismc7f1lwb8qhb-cargo-auditable-0.6.1-vendor.tar.gz.drv:{out}
      • The environments do not match:
          + postUnpack=eval "$cargoDepsHook"

export RUST_LOG=

    • Skipping environment comparison
  • Skipping environment comparison
• Skipping environment comparison

Does that demonstrate the problem more clearly?

[9999years]

1. Is there anything keeping this from getting merged? It looks like the last review was almost a year ago. It's not clear which of the comments are blocking and which aren't. This functionality is widely requested and we should take it over the line, either by merging this PR or buildRustPackage: add overrideRust attribute #288430.

2. I'm not entirely sure of the semantics with this approach. I noticed that the examples use overrideAttrs — does that replace/remove the existing overrideAttrs functionality?

[amesgen]

Is there anything keeping this from getting merged? It looks like the last review was almost a year ago. It's not clear which of the comments are blocking and which aren't.

I think there are no fundamental blockers, but the code here is rather tricky as it is super easy to accidentally write sth that causes infinite recursions, see #194475 (comment). I hope to rebase it in the next days, but everyone is also welcome to take over this PR if they want to drive this.

I stopped regularly rebasing this PR as I wanted to wait for the pkgs-modules Nixpkgs Architectore working group, but I just saw that it was shut down for now.

This functionality is widely requested and we should take it over the line, either by merging this PR or #288430.

From my PoV, #288430 could be merged before this PR as it is useful as long as some Rust packages are not yet using the finalAttrs style.

I'm not entirely sure of the semantics with this approach. I noticed that the examples use overrideAttrs — does that replace/remove the existing overrideAttrs functionality?

It is the regular overrideAttrs, this PR just enables to use the advantages that the finalAttrs style (#119942) brings to overrideAttrs, also see https://nixos.org/manual/nixpkgs/unstable/#sec-pkg-overrideAttrs.

[nikstur]

Having this feature would really make my life much easier. It would make this override that I use in bombon to add a passthru SBOM to a Rust derivation much simpler: https://github.com/nikstur/bombon/blob/main/nix/passthru-vendored.nix#L9

Also it would make it much easier to add a passthru clippy or rustfmt derivation directly from the same file.