all-packages.nix: move defaults to package files #55129
Conversation
These are already inherited in the parent derivation.
| @@ -1,4 +1,4 @@ | |||
| {stdenvNoCC, subversion, sshSupport ? false, openssh ? null, expect}: | |||
| {stdenvNoCC, subversion, sshSupport ? true, openssh ? null, expect}: | |||
There was a problem hiding this comment.
Shoud this openssh ? null become just openssh if the local default for ssh support is true?
There was a problem hiding this comment.
In theory: no preference, in practice: it usually it is not in most other expressions.
There was a problem hiding this comment.
The default will have the openssh argument, so getting null already needs an explicit override, no?
There was a problem hiding this comment.
Strictly speaking, the default it will get depends on callPackage implementation used, so the above should be okay, normally. In this instance, however, the output will depend on openssh anyway because the path gets burned into derivation irrespective of sshSupport. I'm not sure if fixing this should be in scope of this PR, though.
There was a problem hiding this comment.
I was only asking about default argument values, I agree with both your implied claims (ignoring the argument if the support if off is good, but fixing this is out of scope).
I think if it is called in Nixpkgs exactly once, it is acceptable to assume this is the only type of callPackage that will be applied.
| , x11Support ? false, xlibsWrapper ? null | ||
| , cupsSupport ? false, cups ? null | ||
| , cupsSupport ? config.ghostscript.cups or (!stdenv.isDarwin), cups ? null | ||
| , x11Support ? cupsSupport, xlibsWrapper ? null # with CUPS, X11 only adds very little |
There was a problem hiding this comment.
Somewhat borderline on that… (I do not oppose that, but do not actively suport and will wait longer for objections to this change before merging)
There was a problem hiding this comment.
But that is exactly what was written in all-packages.nix before this change.
There was a problem hiding this comment.
I agree with the logic, I am just not sure if this type of extracting defaults from contents of other arguments is something everyone has made peace with.
There was a problem hiding this comment.
Hm, I felt this was a common thing to do, grep -rE '.*Support \? .*Support' gives a lot of results.
There was a problem hiding this comment.
I tried to comment on the entire two-line combo — …Support ? …Support but together with …Support ? config.…
There was a problem hiding this comment.
Ah, I see. Well, a) it is just another variable binding, so not that much of a difference, IMHO b) I don't see any other options in this case as applying config.ghostscript or {} in all-packages.nix will do the wrong thing.
| licenseFile = config.gams.licenseFile or null; | ||
| optgamsFile = config.gams.optgamsFile or null; | ||
| }; | ||
| gams = callPackage ../tools/misc/gams (config.gams or {}); |
There was a problem hiding this comment.
temporary dependency version overrides (in our normal «update/pin old where needed/investigate» upproach) would become a bit more annoying with this style in all-packages.nix…
There was a problem hiding this comment.
Sure, but I did not invent this style, this is the style that is already used in all-packages.nix.
There was a problem hiding this comment.
… unfortunately. Although it seems that this abbreviation indeed doesn't lead to the expected problem in practice.
|
ping? |
|
Indeed. |
|
Thanks! |
What?
A bit of noop cleanups followed by a first patchset in a series that moves package option defaults (like
whateverSupport = false) fromall-packages.nixto the package expressions themselves.I take specific care not to rename or drop any
configoptions here (I do add some as a side-effect, though).Why?
Because definitions in
all-packages.nixand package expression frequently conflict. It also makes #50643 (which itself is motivated by a proper implementation of #12877) much easier to do.git logdwm: cleanup whitespace
dysnomia: cleanup whitespace
pulseaudioFull, libpulseaudio-vanilla: cleanup
These are already inherited in the parent derivation.
top-level: fix a typo
top-level: cleanup whitespace
lib: tiny cleanup
*: move defaults to the package file
A bunch of these.
nix-instantiateenvironmentnix-env -qaPdiffs/cc @matthewbauer @7c6f434c, I guess