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.

Sign up for GitHub

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

Jump to bottom

buildRustPackage: support fixed-point arguments via lib.extendMkDerivation #382550

Merged
merged 4 commits into from
Feb 22, 2025
Merged

buildRustPackage: support fixed-point arguments via lib.extendMkDerivation #382550

merged 4 commits into from
Feb 22, 2025
+241 −243

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Feb 16, 2025 •
edited
Loading

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

  • Built on platform(s) (Ne rebuilds or clean-ups.)
    • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@github-actions github-actions bot added 6.topic: rust 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Feb 16, 2025
@ShamrockLee ShamrockLee marked this pull request as ready for review February 16, 2025 15:48
@nix-owners nix-owners bot requested review from winterqt, figsoda and zowoq February 16, 2025 15:49
@ShamrockLee ShamrockLee requested a review from TomaSajt February 16, 2025 15:51
@ShamrockLee
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 382550

@9999years 9999years mentioned this pull request Feb 18, 2025
Closed
9999years
9999years approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@9999years 9999years left a comment

Choose a reason for hiding this comment

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

LGTM!

amesgen
amesgen approved these changes Feb 18, 2025
View reviewed changes
Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

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

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).

@amesgen amesgen added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Feb 18, 2025
ShamrockLee and others added 4 commits February 21, 2025 00:42
@ShamrockLee
buildRustPackage: avoid global assertions
016ba92 
Move assertions down to the corresponding attribute value.

Avoid adding assertions to the whole argument set.
@ShamrockLee
buildRustPackage: restructure with lib.extendMkDerivation
7609cba 
Support fixed-point arguments with lib.extendMkDerivation

Postpone formatting for more concise diff.
@ShamrockLee
buildRustPackage: reformat after refactoring
721751b
@TomaSajt @ShamrockLee
uiua: use finalAttrs pattern
31e261a
@ShamrockLee ShamrockLee force-pushed the build-rust-package-finalattrs branch from c5a2020 to 31e261a Compare February 20, 2025 16:47
@ShamrockLee
Copy link
Contributor Author

Rebased on top of master to resolve merge conflicts.

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Feb 21, 2025
@lf- lf- merged commit c2fa08b into NixOS:master Feb 22, 2025
25 of 27 checks passed
@sodiboo sodiboo mentioned this pull request Feb 25, 2025
Merged
13 tasks
This was referenced Feb 26, 2025
Open
Merged
drupol added a commit to drupol/nixpkgs that referenced this pull request Feb 28, 2025
@drupol
build-support/php: support lib.extendMkDerivation
678e601 
Inspired from NixOS#382550
Context: NixOS#234651
@drupol drupol mentioned this pull request Feb 28, 2025
Merged
13 tasks
drupol added a commit to drupol/nixpkgs that referenced this pull request Feb 28, 2025
@drupol
build-support/php: support lib.extendMkDerivation
a2e2ee9 
Inspired from NixOS#382550
Context: NixOS#234651
drupol added a commit to drupol/nixpkgs that referenced this pull request Feb 28, 2025
@drupol
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
@drupol
build-support/php: support lib.extendMkDerivation for `php.mkCompos…
4cdddfa 
…erVendor`

Inspired from NixOS#382550

Context: NixOS#234651
@GaetanLepage
Copy link
Contributor

GaetanLepage commented Feb 28, 2025 •
edited
Loading

Do you think that this could/should be backported?

(EDIT: Thank you for this great work!)

JohnRTitor pushed a commit that referenced this pull request Mar 1, 2025
@drupol @JohnRTitor
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
@drupol @JohnRTitor
build-support/php: support lib.extendMkDerivation for `php.mkCompos…
cd850d5 
…erVendor`

Inspired from #382550

Context: #234651
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Mar 1, 2025 •
edited
Loading

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
Copy link
Contributor

GaetanLepage commented Mar 1, 2025 •
edited
Loading

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
Copy link
Contributor Author

ShamrockLee commented Mar 1, 2025 •
edited
Loading

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.

@nyabinary nyabinary mentioned this pull request Mar 3, 2025
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lf- lf- lf- approved these changes

@9999years 9999years 9999years approved these changes

@amesgen amesgen amesgen approved these changes

@winterqt winterqt Awaiting requested review from winterqt

@figsoda figsoda Awaiting requested review from figsoda

@zowoq zowoq Awaiting requested review from zowoq

@TomaSajt TomaSajt Awaiting requested review from TomaSajt

Assignees
No one assigned
Labels
6.topic: rust 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ShamrockLee @GaetanLepage @lf- @9999years @amesgen @wegank @TomaSajt