-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
all-packages.nix: move defaults to package files #55129
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
Changes from all commits
1ad836b
d35c199
735d287
849b10a
107f0fc
51687d9
6832a42
6cb5666
5da88a1
d064592
35a09f9
7706253
679bf2a
25709df
8cbe72a
ec122cc
bdd69a9
a4f2097
4bc9404
546c8ff
6e1e0bb
c1ee0a4
4ec5cbc
866944f
a4cebde
b5e511d
1b784e5
2cf1407
50153af
58a2757
caff8b7
ce75344
f56be70
56ee5bc
4f066da
ec2452d
f37effb
88ca6dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,13 @@ | ||
| { stdenv, lib, fetchurl, pkgconfig, zlib, expat, openssl, autoconf | ||
| { config, stdenv, lib, fetchurl, pkgconfig, zlib, expat, openssl, autoconf | ||
| , libjpeg, libpng, libtiff, freetype, fontconfig, libpaper, jbig2dec | ||
| , libiconv, ijs | ||
| , 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But that is exactly what was written in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I felt this was a common thing to do,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to comment on the entire two-line combo —
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| }: | ||
|
|
||
| assert x11Support -> xlibsWrapper != null; | ||
| assert cupsSupport -> cups != null; | ||
|
|
||
| let | ||
| version = "9.${ver_min}"; | ||
| ver_min = "26"; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoud this
openssh ? nullbecome justopensshif the local default for ssh support istrue?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory: no preference, in practice: it usually it is not in most other expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default will have the
opensshargument, so gettingnullalready needs an explicit override, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking, the default it will get depends on
callPackageimplementation used, so the above should be okay, normally. In this instance, however, the output will depend onopensshanyway because the path gets burned into derivation irrespective ofsshSupport. I'm not sure if fixing this should be in scope of this PR, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
callPackagethat will be applied.