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

buildRustPackage: refactor to support finalAttrs pattern #354999

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

TomaSajt
Copy link
Contributor

@TomaSajt TomaSajt commented Nov 10, 2024

I'll rebase onto staging once this PR is reviewed.
Note: the first commit is a cherry-pick from staging

Supersedes #194475
Supersedes #288430
Supersedes #340798

Similar PR that also does this for buildGoPackage: #353526

TODO: check eval performance impact

This PR refactors the buildRustPackage wrapper around mkDerivation to support the finalAttrs pattern better.

A non-goal was having 0 rebuilds.
The rebuilds are because of more attrs actually being passed to mkDerivation, which improves overridability.


Other than just being able to do something like

rustPlatform.buildRustPackage (finalAttrs: {
  a = finalAttrs.cargoHash;
  b = finalAttrs.cargoDeps;
  c = finalAttrs.doCheck
})

you'll also be able to override derivations much easier:

drv.overrideAttrs {
  ...
  cargoHash = "...";
}

cargoLock is something that can only be inside passthru, since it can't be converted to a string. This means that overriding needs to be done through passthru. This is a bit painful, but at least it's possible.

drv.overrideAttrs (prevAttrs: {
  passthru = prevAttrs.passthru or {} // {
    cargoLock = { lockfile = ./Cargo.lock; }
  };
})

This means that to override a package that uses cargoLock to use cargoHash, you will need to set passthru.cargoLock to null:

drv.overrideAttrs (prevAttrs: {
  cargoHash = "...";
  passthru = prevAttrs.passthru or {} // {
    cargoLock = null
  };
})

You may also just override cargoDeps directly, like you would have done before this PR

drv.overrideAttrs {
  cargoDeps = ...;
}

(cargoDeps can also be set directly from buildCargoPackage if, for some reason, you needed that)


Note:

Having @args and a ? "default" together can be error-prone and may lead to unexpected behaviour.
Because of this, I opted to set default values by just setting them to the default and then expecting them to be overridden with //

{ a = "default"; } // lib.removeAttrs args namesToRemove


I changed the wrapper to use { ~~~ } // args' instead of args' // { ~~~ }. This will make args' shadow the values being set without us having to do a = args.a or "default" everywhere. Though, when we don't want this (e.g. lists, where we'd rather combine with the original), we still do or "default"

Above I used args' to denote lib.removeAttrs args namesToRemove where namesToRemove is a list of values that should not be blindly shadowing anything. Usually, these have some extra logic associated with them.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Sorry, something went wrong.

@TomaSajt TomaSajt force-pushed the build-rust-package-fixed-point branch from 12b63e7 to bba2752 Compare November 10, 2024 14:23
@TomaSajt TomaSajt changed the base branch from master to staging November 10, 2024 14:26
@TomaSajt TomaSajt force-pushed the build-rust-package-fixed-point branch 3 times, most recently from a9bea81 to 639e245 Compare November 10, 2024 15:15
@TomaSajt TomaSajt marked this pull request as ready for review November 10, 2024 19:42
@nix-owners nix-owners bot requested review from figsoda, winterqt and zowoq November 10, 2024 19:43
@TomaSajt TomaSajt force-pushed the build-rust-package-fixed-point branch 3 times, most recently from cfe8797 to 7552eb6 Compare November 10, 2024 23:15
@TomaSajt TomaSajt force-pushed the build-rust-package-fixed-point branch 5 times, most recently from 4b14c19 to dd7fb2e Compare November 15, 2024 01:12
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 15, 2024
@TomaSajt TomaSajt force-pushed the build-rust-package-fixed-point branch 2 times, most recently from 16d8715 to 3b3a29b Compare November 16, 2024 11:47
@github-actions github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Nov 16, 2024
@github-actions github-actions bot removed 8.has: changelog 6.topic: policy discussion 6.topic: k3s Kubernates distribution (https://k3s.io/) 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Nov 16, 2024
@TomaSajt TomaSajt force-pushed the build-rust-package-fixed-point branch 2 times, most recently from aaf0a62 to e8cb71b Compare November 16, 2024 12:09
@TomaSajt TomaSajt marked this pull request as draft November 16, 2024 12:16
@TomaSajt TomaSajt changed the base branch from staging to master November 16, 2024 12:16
@TomaSajt TomaSajt force-pushed the build-rust-package-fixed-point branch 2 times, most recently from 4e98efb to 27d0957 Compare November 16, 2024 12:51
@JohnRTitor JohnRTitor requested a review from Mic92 November 16, 2024 18:18
Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

For easier review, could you format the file with nixfmt-rfc-style first and then make the appropriate changes for finalAttrs?

Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

And do make the changes atomically in seperate commits.

Like:

  1. Support finalAttrs pattern
  2. Improve overridebility

@TomaSajt
Copy link
Contributor Author

For easier review, could you format the file with nixfmt-rfc-style first and then make the appropriate changes for finalAttrs?

It was originally like that, however I had to rebase and merged them together temporarily. Will separate them again.

And do make the changes atomically in seperate commits.

Like:

1. Support finalAttrs pattern

2. Improve overridebility

The reverse order would be better for me better: the support finalAttrs pattern was just 2 lines of change after having done the refactoring for overridability.

@JohnRTitor
Copy link
Contributor

The reverse order would be better for me better: the support finalAttrs pattern was just 2 lines of change after having done the refactoring for overridability.

Ah. I wonder if it would be better to split the override fix commit into a seperate PR. finalAttrs pattern with 0 rebuilds can target master while the overridability commit can't. Guess that won't be happening.

@TomaSajt TomaSajt force-pushed the build-rust-package-fixed-point branch 4 times, most recently from 55e78c0 to 3a08c15 Compare November 16, 2024 19:35
@TomaSajt TomaSajt removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 16, 2024

Verified

This commit was signed with the committer’s verified signature.
TomaSajt Toma

Verified

This commit was signed with the committer’s verified signature.
TomaSajt Toma

Verified

This commit was signed with the committer’s verified signature.
TomaSajt Toma

Verified

This commit was signed with the committer’s verified signature.
TomaSajt Toma

Verified

This commit was signed with the committer’s verified signature.
TomaSajt Toma

Verified

This commit was signed with the committer’s verified signature.
TomaSajt Toma
@TomaSajt TomaSajt force-pushed the build-rust-package-fixed-point branch from 3a08c15 to 975a1c1 Compare November 16, 2024 19:37
@JohnRTitor JohnRTitor added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 17, 2024
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

2 participants