-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
lib: etc #113661
base: master
Are you sure you want to change the base?
lib: etc #113661
Conversation
ac2d716
to
7c350f4
Compare
- truncate 4 "1.2.3-stuff" | ||
=> "1.2.3.stuff" |
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.
Wouldn't it be?
- truncate 4 "1.2.3-stuff" | |
=> "1.2.3.stuff" | |
- truncate 3 "1.2.3-stuff" | |
=> "1.2.3-stuff" |
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.
Well, the pitfall of this truncation function follows from the implementation of builtins.splitVersion
, which has the following behaviour
builtins.splitVersion "1.2.3-stuff" == [ "1" "2" "3" "stuff" ]
In the same way the current (in nixpkgs master branch) majorMinor
implementation satisfies the following:
lib.versions.majorMinor "1.2-stuff" == "1.2"
lib.versions.majorMinor "1-stuff" == "1.stuff"
I merely abstracted away the body of the function to generalize it to majorMinorPatch
.
I agree this is not intuitive, but that's the best I can do that does not break compatibility and extends the current library quite uniformly.
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 see, maybe add a note on splitVersion
in the documentation.
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.
done
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.
Maybe you should also add a note in the documentation of truncate
that it doesn't preserve the delimiter type explicitly.
isGe = opTruncate versionAtLeast; | ||
isGt = opTruncate (flip versionOlder); | ||
isLe = opTruncate (flip versionAtLeast); | ||
isLt = opTruncate versionOlder; | ||
isEq = opTruncate preds.equal; | ||
range = low: high: preds.inter (versions.isGe low) (versions.isLe high); |
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 find the naming here pretty confusing and it is non obvious to a reader in another file that the version number gets truncated before the predicate is applied.
Also what is the exact motivaton for this versionAtLeast
etc. can already deal with versions of different lengths.
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 major difference is that versionAtLeast
implements an order relation on version numbers (which is total assuming the only separator which is used is .
), while the predicates I implement here are not order relations, they are not antisymmetric (in the sense that both isLe "1.1" "1.1.2"
and isLe "1.1.2" "1.1"
are true) and (isLe x) y
should be read as «y
is a version which is lower or equal to x
». Contrarily to versionAtLeast
, its analogous isLe
uses its first argument to determine the length to truncate so that isLe "1.1" "1.1.2"
is true while versionAtLeast "1.1" "1.1.2"
is false...
The main motivation is to be able to combine and read them nicely with high order function such as
filter
, e.g. the result offilter (versionAtLeast "1.0") [ "1.3.1" "1.0.0" "1.0" "0.9.0" "1.0.2" ]
, which is[ "1.0" "0.9.0" ]
is very unnatural to me, both in readingversionAtLeast
the wrong way around and the fact that I would like1.0.2
to be included in the output: indeed "1.0.2" is part of the family of "1.0" versions! In comparisonfilter (isLe "1.0") [ "1.3.1" "1.0.0" "1.0" "0.9.0" "1.0.2" ]
reads more naturally (we keep versions that are lesser or equal to "1.0") and the outcome[ "1.0.0" "1.0" "0.9.0" "1.0.2" ]
matches my expectations: if I wanted to filter more precisely I would give the patch version of the first argument ofisLe
instead, and if I wanted to exclude all "1.0", then I would useisLt "1.0"
.switch
(defined in this PR), for examplenixpkgs/pkgs/development/coq-modules/mathcomp/default.nix
Lines 21 to 22 in 3daa06c
defaultVersion = with versions; switch coq.coq-version [ { case = isGe "8.13"; out = "1.12.0"; } # lower version of coq to 8.10 when all mathcomp packages are ported
EDIT: I corrected a few typos that made it difficult to understand the first paragraph... should be better now.
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 agree this should be explained in a better way inside the comment in the code. I will try my best to be clearer.
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 pushed a new commit with better explanations (basically what is up here modulo transformation from github PR comment to inline code comment).
9d80b4b
to
49b69f5
Compare
- in `lib.versions`: adding functions `preds.truncate`, `preds.majorMinorPatch`, `preds.isGe`,`preds.isLe`, `preds.isGt`, `preds.isLt`, `preds.isEq`, `preds.range`, - in`lib.preds`: adding functions `preds.true`, `preds.false`, `preds.inter`, `preds.union`, `preds.compl`, `preds.diff`, `preds.impl`, and `preds.equal`, - in `lib.switchs`: adding functions `switch-if` and `switch`, - in `lib.lists`: adding function `splitList`- in `lib.versions`: adding functions `truncate`, `majorMinorPatch`, `isGe`, `isLe`, `isGt`, `isLt`, `isEq`, `range`, - in`lib.preds`: adding functions `true`, `false`, `inter`, `union`, `compl`, `diff`, `impl`, and `equal`, - in `lib.switchs`: adding functions `switch-if` and `switch`, - in `lib.lists`: adding function `splitList`, and used these in Coq related packages...
BTW @sternenseemann I would be glad if you could re-review after I took your remarks into account. I just rebased. |
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 just learned about this PR. Great to be able to read an improved doc for these functions!
|
||
if the variables p are not functions, | ||
they are converted to a `equal p` | ||
if `out` is missing then `default-out` is taken |
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.
Is there a use case for a complex-clause
without an out
?
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 think I have one somewhere, but anyway, this makes the code shorter!
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.
Does it? I thought removing this would make this code:
switch-if (map (cl: { cond = combine cl var;
out = cl.out or default; }) clauses) default;
become:
switch-if (map (cl: cl // { cond = combine cl var; }) clauses) default;
But if you actually use this, never mind.
|
||
Example: | ||
```nix | ||
let test = v1: v2: with versions; switch [ v1 v2 ] [ |
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'm not sure what is the purpose of with versions;
in this example. Did you originally intend to have a more realistic example, changed your mind, and forgot to remove with versions;
?
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.
Ahah yes this is deadcode. Well spotted!
|
||
/* Break a version string into its component parts. | ||
|
||
Example: | ||
splitVersion "1.2.3" | ||
=> ["1" "2" "3"] | ||
|
||
Remark: the builtins version of `splitVersion` -- which is |
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'm wondering if this isn't a Frenchism. I would use Note:
instead.
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.
What would be the problem with "remark", except that it is an understatement. "Disclaimer" would be more appropriate IMO
...
Co-authored-by: Théo Zimmermann <[email protected]>
=> `[ "zero" "one" "I do not count beyond one" ]` | ||
|
||
*/ | ||
switch-if = c: d: (findFirst (getAttr "cond") {} c).out or d; |
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.
Please use camelCase (switchIf
).
I'm not in favor of |
|
Are you not in favor of both |
When fully applied, indeed, but when partially applied (e.g. when combined with More generally, maybe I misunderstood the purpose of |
I would be delighted to see a switch construct in nix, but I don't feel confident enough it will ever happen to disregard the addition of I do think there is a real need for a switch construct: If you have a lot of nixpkgs/pkgs/build-support/bintools-wrapper/default.nix Lines 70 to 87 in 33968c4
If we'd require {
dynamicLinker =
/**/
if sharedLibraryLoader == null then null
else if targetPlatform.libc == "musl" then "${sharedLibraryLoader}/lib/ld-musl-*"
else if (targetPlatform.libc == "bionic" && targetPlatform.is32bit) then "/system/bin/linker"
else if (targetPlatform.libc == "bionic" && targetPlatform.is64bit) then "/system/bin/linker64"
else if targetPlatform.libc == "nblibc" then "${sharedLibraryLoader}/libexec/ld.elf_so"
else if targetPlatform.system == "i686-linux" then "${sharedLibraryLoader}/lib/ld-linux.so.2"
else if targetPlatform.system == "x86_64-linux" then "${sharedLibraryLoader}/lib/ld-linux-x86-64.so.2"
else if targetPlatform.system == "powerpc64le-linux" then "${sharedLibraryLoader}/lib/ld64.so.2"
# ARM with a wildcard, which can be "" or "-armhf".
else if (with targetPlatform; isAarch32 && isLinux) then "${sharedLibraryLoader}/lib/ld-linux*.so.3"
else if targetPlatform.system == "aarch64-linux" then "${sharedLibraryLoader}/lib/ld-linux-aarch64.so.1"
else if targetPlatform.system == "powerpc-linux" then "${sharedLibraryLoader}/lib/ld.so.1"
else if targetPlatform.isMips then "${sharedLibraryLoader}/lib/ld.so.1"
else if targetPlatform.isDarwin then "/usr/lib/dyld"
else if targetPlatform.isFreeBSD then "/libexec/ld-elf.so.1"
else if lib.hasSuffix "pc-gnu" targetPlatform.config then "ld.so.1"
else null;
}
This would be |
I think the main point is that adding functions to lib means endorsing their use as part of nixpkgs. We need to take care to some extent that we don't unnecessarily increase the complexity of nix code in nixpkgs, for example not all contributors have a background in functional programming and already find some concepts we employ regularly hard to grasp. I think especially with an emphasize on function composition in |
To me code like
is easy to understand even for somebody who isn't very familiar with Nix or nixpkgs.lib. OTOH, I have trouble figuring out code like
E.g. it's not very obvious what |
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.
Can you try adding the new library categories to doc/doc-support/lib-function-{docs.nix,locations.nix}
and building the manual. This should also help spotting any places where nixdoc
doesn't like your doc comments.
/* Takes a predicate and a list as input and | ||
returns a list of lists, separated by elements that match the predicate. | ||
|
||
This is analoguous to `builtins.split separator string`, |
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.
Wonder if we should use a stronger name like splitIf
or splitListIf
to disambiguate from builtins.split
?
- `res` has length `2 * k + 1`, | ||
where `k` is the number of elements of `l` that | ||
match `pred`. (ie `splitList pred l`) | ||
- `elemAt res (2 * n)` is always a list, |
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.
Maybe add “where n
is a number smaller than builtins.div (builtins.length l) 2
. Same for the next property.
@@ -0,0 +1,17 @@ | |||
{ lib }: | |||
with lib; { |
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.
Document these individually, so nixdoc
can pick them up properly.
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 think I will rename preds.true
to preds.any
and preds.false
to preds.none
to make the problematic example of @edolstra ["8.13" preds.true]
read ["8.13" preds.any]
instead, which looks more readable IMO.
|
||
*/ | ||
splitList = pred: l: | ||
let loop = (vv: v: l: if l == [] then vv ++ [v] |
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 parentheses should be unnecessary here.
``` | ||
=> `[ "small, 8" "sum over 42" "zero, one" "weird" "weird" ]` | ||
*/ | ||
switch = var: clauses: default: with preds; let |
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 think we should only have switch
, but the magic should be explicit by checking which of those attributes the case attribute set has:
cond
: Is a predicategets converted to
all (equal true) (zipListsWith equal cl.conds var)`conds
:case
: gets converted toequal x
cases
: gets converted toall (equal true) (zipListsWith equal cl.cases var)
Some additional ideas:
- There should be a version of
switch
withvar
as its last argument, so it can be curried easily. - Having a type of condition for boolean expressions would also be nice (maybe named
cond
and the current renamed topred
?), so expressions like!lib.inNixShell
could be incooperated nicely. oneof
: which would be roughlybuiltins.any (equal true) (builtins.map (equal x) cl.oneof)
could be nice
Edit: Fixed my cases
example, added conds
.
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.
@sternenseemann Don't you like the fact that when some x
is not a function it gets converted automatically to equal x
? This looks harmless to me as it would not make sense to compare two functions, and it allows to combine both styles within cases
(as in {cases = [somePred x]; ...}
where somePred
is really a predicate and x
is not), which I think is a common usecase. Moreover it also frees cond
for boolean conditions in order to subsume switchIf
. I like your oneof
suggestion and would also add allof
for the sake of dualization.
Indeed switch
is not flipable conveniently via flip
... maybe we could have switchFun = cs: dft: x: switch x cs dflt
?
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.
Yeah, the “magic” is pretty cool, but I'm also a bit cautious in this regard. There is always the danger of for example a function that is partially applied by accident triggering the magic although you didn't want to — resulting in a weird and hard to debug eval error.
Not sure what the best call is here.
- truncate 4 "1.2.3-stuff" | ||
=> "1.2.3.stuff" |
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.
Maybe you should also add a note in the documentation of truncate
that it doesn't preserve the delimiter type explicitly.
One thing I always appreciated, coming into the |
For comparison, the full snippet with {
defaultVersion = with versions;
/**/ if coq.coq-version == "8.13" then "1.5.0"
else if coq.coq-version == "8.12" then "1.4.0"
else if coq.coq-version == "8.11" then "1.3.2"
else if coq.coq-version == "8.10" then "1.2.1"
else if coq.coq-version == "8.9" then "1.1.0"
else if coq.coq-version == "8.8" then "20190311"
else if coq.coq-version == "8.7" && isLe "1.8" ssreflect.version then "1.0.0"
else if coq.coq-version == "8.6" then "20171102"
else if coq.coq-version == "8.5" then "20170512"
else null;
} I think arguing over which of those is better won't really lead to any outcome. I do think that the editability of |
I do not intend to remove the use of |
There was NixOS/rfcs#82, but it seems to have stalled since. In general I see nothing wrong in principle with adding the functions added here. |
As another maintainer of |
I have seen this idiom for switch several time:
it is both shorter and way easier to read than |
I used it in the begining, it works well for this particular example, but in the case of ranges of versions of two libs then it becomes unreadable. And the question of maintainability matters, if you need to change completely the pattern at some point it becomes even harder for casual users... |
I marked this as stale due to inactivity. → More info |
Motivation for this change
In Coq related packages, I defined several generic functions that I
believe are of general interest. I am open to many variations or
renaming of those:
in
lib.versions
: adding functionstruncate
,majorMinorPatch
,isGe
,isLe
,isGt
,isLt
,isEq
,range
,in
lib.preds
: adding functionstrue
,false
,inter
,union
,compl
,diff
,impl
, andequal
,in
lib.switchs
: adding functionsswitch-if
andswitch
,in
lib.lists
: adding functionsplitList
.Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)