rustPlatform.buildRustPackage: support finalAttrs style#194475
rustPlatform.buildRustPackage: support finalAttrs style#194475amesgen wants to merge 3 commits intoNixOS:masterfrom
finalAttrs style#194475Conversation
github-actions
bot
added
6.topic: rust
amesgen
force-pushed
the
buildRustPackage-finalAttrs
branch
from
aa4aa88 to
95a98c9
Compare
ofborg
bot
added
10.rebuild-darwin: 0
amesgen
force-pushed
the
buildRustPackage-finalAttrs
branch
from
95a98c9 to
a8e675e
Compare
|
I came to this from #107070 and I must say that the fact that I can actually see and fix the |
amesgen
force-pushed
the
buildRustPackage-finalAttrs
branch
from
a8e675e to
9e66b79
Compare
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point, will go with removeFromBuilderEnv for now 👍
There was a problem hiding this comment.
Technically an overlay is a list of overrides. This is just an override.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
amesgen
force-pushed
the
buildRustPackage-finalAttrs
branch
from
9e66b79 to
d16ba39
Compare
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
+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.
amesgen
force-pushed
the
buildRustPackage-finalAttrs
branch
from
d16ba39 to
d28df4f
Compare
ofborg
bot
added
10.rebuild-darwin: 1-10
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
Poke @roberth ^^ |
There was a problem hiding this comment.
Can we move the assserts after the inputs? thats a bit cleaner than inlining them.
There was a problem hiding this comment.
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
finalAttrshave 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 dedicatedassertsattribute, i.e.asserts = assert (all asserts here); "";.
ofborg
bot
added
10.rebuild-darwin: 0
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
Just small suggestions; already looks good.
amesgen
force-pushed
the
buildRustPackage-finalAttrs
branch
from
d0f22c7 to
172ee4d
Compare
amesgen
force-pushed
the
buildRustPackage-finalAttrs
branch
from
172ee4d to
fc094d7
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
amesgen
force-pushed
the
buildRustPackage-finalAttrs
branch
from
67d3965 to
48e9de7
Compare
ofborg
bot
removed
the
2.status: merge conflict
| # 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Attributes with null values are completely removed from the .drv, so this doesn't look like a mass rebuild to me.
There was a problem hiding this comment.
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.drvAfter 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.drvDiff:
$ 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 comparisonDoes that demonstrate the problem more clearly?
|
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.
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
It is the regular |
wegank
added
the
2.status: stale
wegank
added
the
2.status: merge conflict
stale
bot
removed
the
2.status: stale
|
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 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
wegank
added
the
2.status: stale
|
We should probably get to this, considering the |
stale
bot
removed
the
2.status: stale
wegank
added
the
2.status: stale
|
Status update: #234651 introduced
|
stale
bot
removed
the
2.status: stale
12 participants
Description of changes
This PR explores supporting the new
finalAttrsderivation style forbuildRustPackage, 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 toapplyin the module system), but that might be too powerful/confusing. Also see rustPlatform.buildRustPackage: supportfinalAttrsstyle #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
difftasticto the new style, such that changing its version is now much DRYer:Also note that
passthru.tests.versionthen automatically points to the new package.The "overlay" in
buildRustPackagehas a few curiosities, i.e. we have to be careful to avoidinfinite recursionerrors:{ a ? "", ...}@finalAttrssyntax. Even{...}@finalAttrscausesinfinite recursion.finalAttrshave 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 dedicatedassertsattribute, 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
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes