buildRustPackage: support fixed-point arguments via lib.extendMkDerivation by ShamrockLee · Pull Request #382550 · NixOS/nixpkgs

ShamrockLee · 2025-02-16T12:30:43Z

This PR enables buildRustPackage to take finalAttrs: { ... } with the help of lib.extendMkDerivation introduced in #234651.

The large diff block is due to indentation changes, while the effective diff is minimal. Commit-by-commit review would be much easier.

I cherry-picked the uiua package transition from #354999 to validate the implementation. Other transition examples in #194475 and #354999 seems no longer relevant.

This PR doesn't address the existing overriding issues of the buildRustPackage arguments (such as cargoHash). They will be addressed in subsequent PRs with the help of finalAttrs, just like #225051 and #379602.

Things done

Add a 👍 reaction to pull requests you find important.

ShamrockLee · 2025-02-16T16:18:57Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 382550

9999years

LGTM!

amesgen

LGTM, looking forward to

This PR doesn't address the existing overriding issues of the buildRustPackage arguments (such as cargoHash). They will be addressed in subsequent PRs with the help of finalAttrs

I guess such a PR will be rather similar to what is done in #194475/#354999, maybe they could even be rebased on top of this PR (but it might be easier to just do it from scratch).

Move assertions down to the corresponding attribute value. Avoid adding assertions to the whole argument set.

Support fixed-point arguments with lib.extendMkDerivation Postpone formatting for more concise diff.

ShamrockLee · 2025-02-20T16:47:50Z

Rebased on top of master to resolve merge conflicts.

…poserProject2` Inspired from NixOS#382550 Context: NixOS#234651

…erVendor` Inspired from NixOS#382550 Context: NixOS#234651

…poserProject2` Inspired from #382550 Context: #234651

…erVendor` Inspired from #382550 Context: #234651

ShamrockLee · 2025-03-01T19:28:11Z

Do you think that this could/should be backported?

The changes are backward-compatible, but it requires backporting lib.extendMkDerivation. We might need compelling reasons to convince the reviewers, such as how it eases backporting of other packages (which is why the fetchFromGitHub's tag argument gets backported in #364439 and #373256).

GaetanLepage · 2025-03-01T21:13:20Z

Do you think that this could/should be backported?

The changes are backward-compatible, but it requires backporting lib.extendMkDerivation. We might need compelling reasons to convince the reviewers, such as how it eases backporting of other packages (which is why the fetchFromGitHub's tag argument gets backported in #364439 and #373256).

My question was motivated by a specific backport. But I get that backporting this would require a significant effort.
It is fine not doing it.

ShamrockLee · 2025-03-01T21:32:21Z

My question was motivated by a specific backport.

That sounds like a good evidence supporting the backport.

But I get that backporting this would require a significant effort.
It is fine not doing it.

The PRs needed for this to get backported include:

And the reformatting commits should be re-do manually by running the corresponding versions of nixfmt instead of direct cherry-picking to avoid bringing unwanted changes to the release branch.

IMO, the technical part is not very challenging. Maybe we could start from #234651 and see how the library people would think about it.

nixos-discourse · 2025-03-15T21:57:10Z

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

https://discourse.nixos.org/t/git-blame-ignore-revs-and-build-helper-format-changes/61726/1

…poserProject2` Inspired from #382550 Context: #234651 (cherry picked from commit 862a3e9)

…erVendor` Inspired from #382550 Context: #234651 (cherry picked from commit cd850d5)

ShamrockLee · 2025-03-19T02:35:46Z

Now that lib.extendDerivation has been backported. Let's backport buildRustPackage.

nixpkgs-ci · 2025-03-19T02:37:42Z

Backport failed for release-24.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.11
git worktree add -d .worktree/backport-382550-to-release-24.11 origin/release-24.11
cd .worktree/backport-382550-to-release-24.11
git switch --create backport-382550-to-release-24.11
git cherry-pick -x 016ba92a432222dbbaa249720373d1c4386d4523 7609cbad7cfbee711150ce754c51f6a2ca5fce28 721751bd2326dad2902a81c80a21f621e79e5fd0 31e261a67af18d09bba1ea6b76e997ab25356aac

JohnRTitor · 2025-03-19T03:47:05Z

@ShamrockLee I must entreat you to undertake the manual backporting of this most esteemed pull request.

nixpkgs-ci · 2025-03-19T22:43:19Z

Backport failed for release-24.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.11
git worktree add -d .worktree/backport-382550-to-release-24.11 origin/release-24.11
cd .worktree/backport-382550-to-release-24.11
git switch --create backport-382550-to-release-24.11
git cherry-pick -x 016ba92a432222dbbaa249720373d1c4386d4523 7609cbad7cfbee711150ce754c51f6a2ca5fce28 721751bd2326dad2902a81c80a21f621e79e5fd0 31e261a67af18d09bba1ea6b76e997ab25356aac

With the recent deprecation of `rustPlatform.fetchCargoTarball` + migration to using `fetchCargoVendor` by default in `buildRustPackage` (NixOS/nixpkgs#394012), the `cargo-bundle` override strategy used here, as prescribed by the [nixos asia wiki](https://nixos.asia/en/buildRustPackage) no longer works: https://github.com/zed-industries/zed/blob/c6e2d20a02b523172aea8a22fa2ec6c8975b52e4/nix/build.nix#L100-L116 [`fetchCargoTarball` produced a single derivation][tarball-drv] but `fetchCargoVendor` [produces two][vendor-drvs]: - `${name}-vendor-staging` (inner; FoD) - `${name}-vendor` (outer) [tarball-drv]: https://github.com/NixOS/nixpkgs/blob/36fd87baa9083f34f7f5027900b62ee6d09b1f2f/pkgs/build-support/rust/fetch-cargo-tarball/default.nix#L79 [vendor-drvs]: https://github.com/NixOS/nixpkgs/blob/10214747f5e6e7cb5b9bdf9e018a3c7b3032f5af/pkgs/build-support/rust/fetch-cargo-vendor.nix#L52-L103 `overrideAttrs` here is setting `outputHash` on the latter (which isn't a fixed-output-derivation and does not have `outputHashMode` set which implies `outputHashMode = "flat"`) instead of the inner; this results in errors like this: ```console ❯ nix develop error: output path '/nix/store/cb57w05zvsqxshqjl789kmsy9pbqjn06-cargo-bundle-0.6.1-zed-vendor.tar.gz' should be a non-executable regular file since recursive hashing is not enabled (outputHashMode=flat) error: 1 dependencies of derivation '/nix/store/k3azmxljgjn26hqyhg9m1y3lhx32y939-cargo-bundle-0.6.1-zed.drv' failed to build error: 1 dependencies of derivation '/nix/store/8ag4v0m90m4kcaq1ypp7f85pp8s6fxgc-nix-shell-env.drv' failed to build ``` > [!NOTE] > you will need to remove > `/nix/store/cb57w05zvsqxshqjl789kmsy9pbqjn06-cargo-bundle-0.6.1-zed-vendor.tar.gz` > from your nix store in order to be able to reproduce this We want to be setting `outputHash` on the [first derivation][first-drv] instead. This change has us just do the call to `fetchCargoTarball` manually instead of using overrides. [first-drv]: https://github.com/NixOS/nixpkgs/blob/10214747f5e6e7cb5b9bdf9e018a3c7b3032f5af/pkgs/build-support/rust/fetch-cargo-vendor.nix#L85 --- I suspect CI/other machines didn't catch this due to a store path matching the name + `outputHash` already being present but I'm not entirely sure how this happened... `sha256-Q49FnXNHWhvbH1LtMUpXFcvGKu9VHwqOXXd+MjswO64=` is actually a `fetchCargoTarball` hash, not a `fetchCargoVendor` hash (and upstream `cargo-about`'s `cargoDeps` [has been using `cargoVendor`][ups] since before the nixpkgs bump in 50ad71a) [ups]: https://github.com/NixOS/nixpkgs/blob/1d09c579c12869453d98dd35f6ff9d28dc32e869/pkgs/by-name/ca/cargo-about/package.nix#L22 Note: this commit leaves the hash as is + uses `fetchCargoTarball` to demonstrate that we're not making any modifications to the cargo-about dependencies. The next commit will switch to using `fetchCargoVendor`. --- > [!NOTE] > eventually we'll be able to just have > `.overrideAttrs (_: { cargoHash = "..."; })` > work as expected [^2] [^2]: [now that `buildRustPackage`](NixOS/nixpkgs#382550) uses [`lib.extendMkDerivation`](https://github.com/nixos/nixpkgs/blob/bbdf8601bcf2a7e733d5ef2552109a5d8d5a44ce/doc/build-helpers/fixed-point-arguments.chapter.md) (NixOS/nixpkgs/#234651) the groundwork is in place; a follow PR [needs to use `cargoHash` and friends from `finalAttrs`](https://github.com/NixOS/nixpkgs/blob/10214747f5e6e7cb5b9bdf9e018a3c7b3032f5af/pkgs/build-support/rust/build-rust-package/default.nix#L104)

With the recent deprecation of `rustPlatform.fetchCargoTarball` + migration to using `fetchCargoVendor` by default in `buildRustPackage` (NixOS/nixpkgs#394012), the `cargo-bundle` override strategy used here, as prescribed by the [nixos asia wiki](https://nixos.asia/en/buildRustPackage) no longer works: https://github.com/zed-industries/zed/blob/c6e2d20a02b523172aea8a22fa2ec6c8975b52e4/nix/build.nix#L100-L116 [`fetchCargoTarball` produced a single derivation][tarball-drv] but `fetchCargoVendor` [produces two][vendor-drvs]: - `${name}-vendor-staging` (inner; FoD) - `${name}-vendor` (outer) [tarball-drv]: https://github.com/NixOS/nixpkgs/blob/36fd87baa9083f34f7f5027900b62ee6d09b1f2f/pkgs/build-support/rust/fetch-cargo-tarball/default.nix#L79 [vendor-drvs]: https://github.com/NixOS/nixpkgs/blob/10214747f5e6e7cb5b9bdf9e018a3c7b3032f5af/pkgs/build-support/rust/fetch-cargo-vendor.nix#L52-L103 `overrideAttrs` here is setting `outputHash` on the latter (which isn't a fixed-output-derivation and does not have `outputHashMode` set which implies `outputHashMode = "flat"`) instead of the inner; this results in errors like this: ```console ❯ nix develop error: output path '/nix/store/cb57w05zvsqxshqjl789kmsy9pbqjn06-cargo-bundle-0.6.1-zed-vendor.tar.gz' should be a non-executable regular file since recursive hashing is not enabled (outputHashMode=flat) error: 1 dependencies of derivation '/nix/store/k3azmxljgjn26hqyhg9m1y3lhx32y939-cargo-bundle-0.6.1-zed.drv' failed to build error: 1 dependencies of derivation '/nix/store/8ag4v0m90m4kcaq1ypp7f85pp8s6fxgc-nix-shell-env.drv' failed to build ``` > [!NOTE] > you will need to remove `/nix/store/cb57w05zvsqxshqjl789kmsy9pbqjn06-cargo-bundle-0.6.1-zed-vendor.tar.gz` > from your nix store in order to be able to reproduce this We want to be setting `outputHash` on the [first derivation][first-drv] instead. This change has us just do the call to `fetchCargoTarball` manually instead of using overrides. [first-drv]: https://github.com/NixOS/nixpkgs/blob/10214747f5e6e7cb5b9bdf9e018a3c7b3032f5af/pkgs/build-support/rust/fetch-cargo-vendor.nix#L85 --- I suspect CI/other machines didn't catch this due to a store path matching the name + `outputHash` already being present but I'm not entirely sure how this happened... `sha256-Q49FnXNHWhvbH1LtMUpXFcvGKu9VHwqOXXd+MjswO64=` is actually a `fetchCargoTarball` hash, not a `fetchCargoVendor` hash (and upstream `cargo-about`'s `cargoDeps` [has been using `cargoVendor`][ups] since before the nixpkgs bump in 50ad71a) [ups]: https://github.com/NixOS/nixpkgs/blob/1d09c579c12869453d98dd35f6ff9d28dc32e869/pkgs/by-name/ca/cargo-about/package.nix#L22 --- > [!NOTE] > eventually we'll be able to just have `.overrideAttrs (_: { cargoHash = "..."; })` work as expected [^2] --- Release Notes: - N/A [^2]: [now that `buildRustPackage`](NixOS/nixpkgs#382550) uses [`lib.extendMkDerivation`](https://github.com/nixos/nixpkgs/blob/bbdf8601bcf2a7e733d5ef2552109a5d8d5a44ce/doc/build-helpers/fixed-point-arguments.chapter.md) (NixOS/nixpkgs/#234651) the groundwork is in place; a follow PR [needs to use `cargoHash` and friends from `finalAttrs`](https://github.com/NixOS/nixpkgs/blob/10214747f5e6e7cb5b9bdf9e018a3c7b3032f5af/pkgs/build-support/rust/build-rust-package/default.nix#L104)

Now that 7609cba (buildRustPackage: restructure with lib.extendMkDerivation, 2025-02-16) has been merged (in NixOSgh-382550), migrate to `finalAttrs` pattern. Related-to: https://nixos.org/manual/nixpkgs/unstable/#mkderivation-recursive-attributes

ShamrockLee marked this pull request as ready for review February 16, 2025 15:48

nix-owners bot requested review from figsoda, winterqt and zowoq February 16, 2025 15:49

ShamrockLee requested a review from TomaSajt February 16, 2025 15:51

9999years mentioned this pull request Feb 18, 2025

finalAttrs everywhere / generic makeOverridable #373867

Closed

9999years approved these changes Feb 18, 2025

View reviewed changes

amesgen approved these changes Feb 18, 2025

View reviewed changes

amesgen added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Feb 18, 2025

ShamrockLee and others added 4 commits February 21, 2025 00:42

buildRustPackage: avoid global assertions

016ba92

Move assertions down to the corresponding attribute value. Avoid adding assertions to the whole argument set.

buildRustPackage: restructure with lib.extendMkDerivation

7609cba

Support fixed-point arguments with lib.extendMkDerivation Postpone formatting for more concise diff.

buildRustPackage: reformat after refactoring

721751b

uiua: use finalAttrs pattern

31e261a

ShamrockLee force-pushed the build-rust-package-finalattrs branch from c5a2020 to 31e261a Compare February 20, 2025 16:47

wegank removed the 12.approvals: 2 This PR was reviewed and approved by two persons. label Feb 21, 2025

lf- approved these changes Feb 22, 2025

View reviewed changes

lf- merged commit c2fa08b into NixOS:master Feb 22, 2025
25 of 27 checks passed

sodiboo mentioned this pull request Feb 25, 2025

[24.11] niri: 25.01 -> 25.02 #384921

Merged

13 tasks

This was referenced Feb 26, 2025

rustPlatform.buildRustPackage: support finalAttrs style #194475

Closed

yazi-unwrapped: fix update script; use finalAttrs #385409

Merged

drupol mentioned this pull request Feb 28, 2025

build-support/php: support lib.extendMkDerivation #385830

Merged

13 tasks

drupol added a commit to drupol/nixpkgs that referenced this pull request Feb 28, 2025

build-support/php: support lib.extendMkDerivation for `php.buildCom…

d9d0470

…poserProject2` Inspired from NixOS#382550 Context: NixOS#234651

drupol added a commit to drupol/nixpkgs that referenced this pull request Feb 28, 2025

build-support/php: support lib.extendMkDerivation for `php.mkCompos…

4cdddfa

…erVendor` Inspired from NixOS#382550 Context: NixOS#234651

JohnRTitor pushed a commit that referenced this pull request Mar 1, 2025

build-support/php: support lib.extendMkDerivation for `php.buildCom…

862a3e9

…poserProject2` Inspired from #382550 Context: #234651

JohnRTitor pushed a commit that referenced this pull request Mar 1, 2025

build-support/php: support lib.extendMkDerivation for `php.mkCompos…

cd850d5

…erVendor` Inspired from #382550 Context: #234651

nyabinary mentioned this pull request Mar 3, 2025

xdg-desktop-portal-cosmic: 1.0.0-alpha.5.1 -> 1.0.0-alpha.6 #386198

Merged

13 tasks

JohnRTitor mentioned this pull request Mar 15, 2025

buildNpmPackage: support finalAttrs through lib.extendMkDerivation #390099

Merged

This was referenced Mar 15, 2025

buildVscodeExtension: support finalAttrs through lib.extendMkDerivation #390180

Merged

build-support/go: support finalAttrs through lib.extendMkDerivation #390220

Merged

nixpkgs-ci bot pushed a commit that referenced this pull request Mar 18, 2025

build-support/php: support lib.extendMkDerivation for `php.buildCom…

da85067

…poserProject2` Inspired from #382550 Context: #234651 (cherry picked from commit 862a3e9)

nixpkgs-ci bot pushed a commit that referenced this pull request Mar 18, 2025

build-support/php: support lib.extendMkDerivation for `php.mkCompos…

b2ed58e

…erVendor` Inspired from #382550 Context: #234651 (cherry picked from commit cd850d5)

ShamrockLee added the backport release-24.11 label Mar 19, 2025

ShamrockLee removed the backport release-24.11 label Mar 19, 2025

This was referenced Mar 19, 2025

build-support/rust/sysroot: remove #378006

Merged

[Backport release-24.11] build-support/rust/sysroot: remove #391339

Merged

ShamrockLee added the backport release-24.11 label Mar 19, 2025

ShamrockLee mentioned this pull request Mar 19, 2025

[Backport release-24.11] buildRustPackage: support fixed-point arguments via lib.extendMkDerivation #391398

Merged

13 tasks

axelkar mentioned this pull request Jun 9, 2025

Impossible to override arguments of rustPlatform.fetchCargoVendor #415397

Open

acuteaangle mentioned this pull request Jun 11, 2025

pineflash: Use finalAttrs pattern #415752

Merged

13 tasks

oneingan mentioned this pull request Aug 21, 2025

rust support finalAttrs upstream nikstur/bombon#171

Open

stepbrobd mentioned this pull request Sep 25, 2025

ocamlPackages.buildDunePackage: support fixed point args with lib.extendMkDerivation #446007

Merged

13 tasks

Uh oh!

Conversation

ShamrockLee commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

ShamrockLee commented Feb 16, 2025

nixpkgs-review result

Uh oh!

9999years left a comment

Choose a reason for hiding this comment

Uh oh!

amesgen left a comment

Choose a reason for hiding this comment

Uh oh!

ShamrockLee commented Feb 20, 2025

Uh oh!

Uh oh!

ShamrockLee commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GaetanLepage commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShamrockLee commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nixos-discourse commented Mar 15, 2025

Uh oh!

ShamrockLee commented Mar 19, 2025

Uh oh!

nixpkgs-ci bot commented Mar 19, 2025

Uh oh!

JohnRTitor commented Mar 19, 2025

Uh oh!

nixpkgs-ci bot commented Mar 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ShamrockLee commented Feb 16, 2025 •

edited

Loading

`nixpkgs-review` result

ShamrockLee commented Mar 1, 2025 •

edited

Loading

GaetanLepage commented Mar 1, 2025 •

edited

Loading

ShamrockLee commented Mar 1, 2025 •

edited

Loading