top-level/all-packages: remove overwriting by-name callPackage#453948
top-level/all-packages: remove overwriting by-name callPackage#453948wolfgangwalther wants to merge 15 commits intoNixOS:masterfrom
Conversation
emilazy
left a comment
There was a problem hiding this comment.
I would like to see the changes in “by-name/various: move default values to override interface”, “by-name/various: use scopes as arguments”, “aegisub: remove overrides from all-package.nix”, and “by-name/various: move overrides to package.nix” consistently move the overrides into a let … in … in package.nix rather than adjusting all the references in the files.
This will not always produce what I think is a superior end result, but in some cases it will, and it minimizes churn – both review and git blame – while making it easier to review for correctness. Since the packages were written this way to begin with, we should by default assume that the identifiers they chose are appropriate – and if we think we should remove that layer of indirection, that should be a separate refactoring change, distinct from this mechanical lifting.
| conf ? { }, | ||
| config, | ||
| conf ? config.element-web.conf or { }, |
There was a problem hiding this comment.
I don’t think we should be using config for this. (Not a blocker, because it was like that beforehand – but I’d like to drop this, as you did with config.aegisub. OTOH, conf is really not a great name for this anyway, so eh…)
There was a problem hiding this comment.
The problem here is that conf is currently used from within nixpkgs, IIRC. So I can't drop it as easily. But I agree with you 100%.
There was a problem hiding this comment.
Scrubbing most ad-hoc config. from pkgs is on my giant notebook of "changes-to-make".
cbdfc0c to
136c599
Compare
Will look into these things tomorrow, but I addressed all other feedback, so far. |
136c599 to
17d2489
Compare
Unfortunately, the addition of that let block is not easily automated with the ast-grep based scripts, I have. I now addressed this feedback - in a compromise that is fair, I think. The majority of packages still uses the simple replacement, because they are really simple cases. Everything that I felt was either more complex or where intent to do things differently in the expression was visible, uses the There is one review comment open, but I just left that for visibility. I don't intend to do something about it in here. From my perspective, this is ready. |
Overriding the argument with the same default value as in package.nix is pointless.
The contributing guidelines have been changed to encourage breaking the override interface to benefit from explicit conflict resolution on upgrades.
Improves the override interface, because `withPython` does not conflict with the old `python` package.
The config option was introduced in 9361acf, this should just be done via overlays today.
This has the additional benefit, that the tests will run with `finalPackage`, i.e. taking overrides into account.
fmt_11 is now the default.
17d2489 to
866eee1
Compare
|
just rebased to resolve merge conflicts |
MattSturgeon
left a comment
There was a problem hiding this comment.
Diff LGTM, minus merge conflict
|
Current status: @emilazy is hacking on the ast-grep script I used for the initial changes to see whether we can make this more automated and less manual - and then scale this to tons of other packages that are not in by-name yet, but need the same treatment eventually. |
|
|
||
| let | ||
| # atf is a dependency of libiconv. Avoid an infinite recursion with `pkgsStatic` by using a bootstrap stdenv. | ||
| stdenv = if args.stdenv.hostPlatform.isDarwin then darwin.bootstrapStdenv else args.stdenv; |
There was a problem hiding this comment.
This means that it’s impossible to override the stdenv on Darwin (without knowing implementation details of the derivation). Is there a way to do this that preserves the ability to override it? That’s why I originally did it in all-packages.nix.
There was a problem hiding this comment.
I think @emilazy's comment in #453948 (comment) applies to that?
There was a problem hiding this comment.
I tried moving this into darwin's stdenv bootstrap, but failed. I'm sure you'd be capable of doing that, though :)
There was a problem hiding this comment.
It can’t be in the bootstrap because then pkgsStatic will have an infinite recursion. To make things behave the same between native and cross, the “overrides” have to be in the package definitions not in the bootstrap overlay.
There was a problem hiding this comment.
I see, that would explain me failing to do it.
We could introduce a withStdenv ? if ... argument to allow overriding.
There was a problem hiding this comment.
Putting as draft for the moment. I am currently looking into a way to make this case better to solve here - it's really unsatisfactory, that we can't override stdenv for these packages afterwards anymore.
|
Not going to pursue this anymore myself (#469692). |
In #444420, we changed the approach regarding the override interface for packages in by-name. This allows us to move a lot more packages to by-name - and also to get rid of
callPackageinall-packages.nixfor by-name packages.This PR removes all cases where a by-name package was overwritten, aka with the same name. There are still plenty of
callPackageinto by-name folders inall-packages.nix, but these introduce new attributes.This first step will allow us to disallow overwriting these by-name attributes and then simplify nixpkgs-vet accordingly.
Things done
Add a 👍 reaction to pull requests you find important.