Skip to content

various: remove by-name overrides#474456

Closed
qweered wants to merge 2 commits intoNixOS:masterfrom
qweered:remove-by-name-overrides
Closed

various: remove by-name overrides#474456
qweered wants to merge 2 commits intoNixOS:masterfrom
qweered:remove-by-name-overrides

Conversation

@qweered
Copy link
Contributor

@qweered qweered commented Dec 26, 2025

Continuation of #469771, resurrection of #453948

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@qweered qweered force-pushed the remove-by-name-overrides branch from 15c741c to bddf086 Compare December 26, 2025 23:58
@SigmaSquadron
Copy link
Contributor

Haven't reviewed all changes, but some preliminary notes:

  • I feel like we really should avoid the { ... }@args pattern. It's complex, difficult to understand for new maintainers, and there's really no good reason to have it when we can just have luajit in the list of function inputs.
  • Pinning dependencies like stormlib to older versions will likely have adverse security consequences.

@qweered qweered force-pushed the remove-by-name-overrides branch from bddf086 to 7850caf Compare December 27, 2025 00:05
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 6.topic: python Python is a high-level, general-purpose programming language. labels Dec 27, 2025
@qweered qweered force-pushed the remove-by-name-overrides branch from 7850caf to 420441c Compare December 27, 2025 00:29
@qweered
Copy link
Contributor Author

qweered commented Dec 27, 2025

I removed @args pattern. About pinning dependencies we really don't have better way to handle them, i know that focus of @NixOS/nixpkgs-core is to remove and then disallow by-name overrides

@qweered qweered force-pushed the remove-by-name-overrides branch from 420441c to bbf2a56 Compare December 27, 2025 01:47
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 27, 2025
@qweered qweered force-pushed the remove-by-name-overrides branch from bbf2a56 to 948551b Compare December 28, 2025 22:41
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 28, 2025
@qweered qweered force-pushed the remove-by-name-overrides branch from 948551b to 8582fcd Compare December 28, 2025 23:07
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 29, 2025
@qweered qweered force-pushed the remove-by-name-overrides branch from 8582fcd to 123f24f Compare December 31, 2025 10:22
@qweered qweered requested a review from mdaniels5757 December 31, 2025 10:23
@nixpkgs-ci nixpkgs-ci bot added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Dec 31, 2025
@qweered qweered force-pushed the remove-by-name-overrides branch 2 times, most recently from dc4819b to 429edd6 Compare January 3, 2026 03:26
@qweered qweered requested a review from Aleksanaa January 3, 2026 03:27
@nixpkgs-ci nixpkgs-ci bot added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jan 3, 2026
@qweered qweered force-pushed the remove-by-name-overrides branch from 429edd6 to 4154bad Compare January 10, 2026 15:02
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 10, 2026
Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up!

I went through the whole diff except glib as I'm out of time now, mostly looks good. Big thanks for splitting up the commits per package, those were easier to review than the various: commits. There were a few unrelated changes like rec -> finalAttrs, with libs.platforms removal or removing outdated overrides as suggested previously which is fine, but I think you could keep the scope a little narrower next time.

@emilazy
Copy link
Member

emilazy commented Jan 11, 2026

AFAICT, this PR does not address the fundamental issues caused by current by-name limitations around sometimes producing unusable override interfaces (e.g. in the case of conditional stdenvs) that led to @wolfgangwalther’s original PR being abandoned, right?

  • I feel like we really should avoid the { ... }@args pattern. It's complex, difficult to understand for new maintainers, and there's really no good reason to have it when we can just have luajit in the list of function inputs.

Avoiding it produces the worse outcome where you have a load‐bearing foo' but can still refer to the non‐overridden foo later in the package, as well as resulting in more code churn.

@qweered
Copy link
Contributor Author

qweered commented Jan 11, 2026

for { ... }@args or foo' problem we have rfc NixOS/rfcs#192 and implementation #464396

@mdaniels5757 mdaniels5757 force-pushed the remove-by-name-overrides branch from fbb3bf1 to b3ce16e Compare January 28, 2026 00:32
@mdaniels5757
Copy link
Member

Rebased, which dropped the following commits: bc61ec4, 439778e, 5a635e4, a122428, 8561b04, 2e6967b

@mdaniels5757
Copy link
Member

@qweered I guess the only things that haven't been split are c292e79 and b3ce16e.

I think c292e79 should probably be split up to be one PR for package. That way, each packages' maintainers can decide how they'd like to remove their all-packages.nix override, and this doesn't block the other packages. Would you mind splitting this one up and making those PRs? (In case you didn't already know about it: git add -p is your friend :) )

b3ce16e should be its own PR too, I think. That one is easy, but I don't want to make the PR without reading the diff, and I don't want to read all that... :)

@gepbird
Copy link
Contributor

gepbird commented Jan 28, 2026

b3ce16e should be its own PR too, I think. That one is easy, but I don't want to make the PR without reading the diff, and I don't want to read all that... :)

That diff isn't that scary after you hide whitespace (I learned it from @qweered in #474456 (comment)).

wolfgangwalther and others added 2 commits January 31, 2026 19:34
This has the additional benefit, that the tests will run with
`finalPackage`, i.e. taking overrides into account.
@qweered
Copy link
Contributor Author

qweered commented Jan 31, 2026

every commit from this brach has been splitted into separate pr

@qweered qweered closed this Jan 31, 2026
@qweered qweered deleted the remove-by-name-overrides branch January 31, 2026 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Development

Successfully merging this pull request may close these issues.

7 participants