init: lib.{collect',flattenAttrs} #221608
Conversation
tie
force-pushed
the
collect-path
branch
2 times, most recently
from
3824ed4 to
1e24d95
Compare
ofborg
bot
added
10.rebuild-darwin: 0
|
I think I can improve the implementation in a few places, so marking this as a draft for now. |
This change adds lib.collect' function, an advanced version of lib.collect, that exposes both attribute path and value for the predicate.
This change adds lib.flattenAttrs function that flattens an attribute
set. Along with lib.collect', it is mostly inteded for use with Nix
flakes where packages must be a flat attribute set of derivations. That
is, this allows flakes to define packages (or other outputs) as a tree
of derivations (or other types) and flatten them before exposing.
In particular, this is useful for flakes that want to expose
cross-compiled variants of packages so the user can run, for example,
`nix build path:.#go-linux-amd64-v3` independent of the current system
(as in `packages.${system}`). In this case, Go is used as an example
since the build system has an excellent support for cross-compilation
out-of-the-box (assuming Cgo is disabled).
|
Switched from using |
There was a problem hiding this comment.
Have only really reviewed flattenAttrs so far.
collect' seems to be a tree traversal. Some of the same suggestions may apply.
Would it make sense to specify the order (pre / in / post, https://en.wikipedia.org/wiki/Tree_traversal) in the documentation?
| if pred attrs then attrs | ||
| else | ||
| listToAttrs (map (x: nameValuePair (f x.path) x.value) (collect' | ||
| (_: v: pred v || !isAttrs v) |
There was a problem hiding this comment.
What if the tree only contains the empty path, and the root isn't an attrset?
I think you could come up with this in the tests by thinking more about the possible inputs, besides the desired results.
| /* Flatten an attribute set `attrs` with nested attributes where leaf nodes | ||
| verify a given predicate named `pred`. That is, a flattened set will have | ||
| all leaf values at top level. Attribute paths are transformed to names | ||
| using the function `f`. |
There was a problem hiding this comment.
| /* Flatten an attribute set `attrs` with nested attributes where leaf nodes | |
| verify a given predicate named `pred`. That is, a flattened set will have | |
| all leaf values at top level. Attribute paths are transformed to names | |
| using the function `f`. | |
| /* Consider the final argument to be the root of a tree. | |
| Every node that satisfies `isLeaf` or is not an attribute set is considered a leaf. | |
| Derive an attribute name from each path using `f`, and return them as a single attribute set. |
More declarative, less control flow.
| Using this function on attribute sets that refer to themselves will cause | ||
| infinite recursion if the predicate allows that. |
There was a problem hiding this comment.
| Using this function on attribute sets that refer to themselves will cause | |
| infinite recursion if the predicate allows that. | |
| If your input is cyclic or diverging, make sure that `isLeaf` only returns `true` for a finite part of your input. | |
| The returned flat attribute set does not exhibit the laziness of the original input. | |
| Specifically, `flattenAttrs` can not return before evaluating all leaves (except when `f` behaves as `x: true`, in which case `flattenAttrs f` is a convoluted attribute renaming function). |
Diverging inputs also need a good isLeaf.
Might want to simplify my phrasing though; not sure.
This should be a depth-first pre-order traversal, just like the Should I also add docs for |
|
That'd be great! |
|
I found good use for this for my Would be great to have this in nixpkgs - then I might add that to the Wiki 🤔 |
|
@tennox, neat! I plan to revisit this PR on a vacation next week (June 12th). The implementation should be pretty solid, so I think what’s left are tests and docs for edge cases (see previous review comments). |
|
I almost forgot about my PR
I've updated it to document the strictness issues, at least for derivations, where that's not insignificant. In general I wouldn't recommend creating trees if you need to flatten them anyway. |
wegank
added
2.status: stale
stale
bot
removed
the
2.status: stale
| @@ -564,6 +564,115 @@ runTests { | |||
| }; | |||
| }; | |||
|
|
|||
| # code from the example | |||
| testCollect'Example = let | |||
There was a problem hiding this comment.
Maybe
| testCollect'Example = let | |
| testCollectPrimeExample = let |
Since your test seems to be the first one that tests a function ending in "prime", following test contributions might the naming convention.
These are a little awkward to type:
mapAttrs'Example
overrideScope'Example
There was a problem hiding this comment.
I think this is a PrimeExample of an unnecessary rule that may lead to more discomfort rather than less.
To be fair, I wouldn't actually call it a prime example, but I couldn't resist the pun, and it actually makes a point!
However, test names are a bit inconsequential.
| @@ -444,6 +444,105 @@ rec { | |||
| else | |||
| []; | |||
|
|
|||
| /* Recursively collect values produced by function `f` that verify a given | |||
There was a problem hiding this comment.
Please change the format to commonmark. This PR is open for so long... conventions beeing changed in master branch already.
https://github.com/NixOS/nixpkgs/blob/master/lib/attrsets.nix#L65
| /* Recursively collect values produced by function `f` that verify a given | |
| /** Recursively collect values produced by function `f` that verify a given |
| when the predicate is verified. | ||
|
|
||
| If `pred` verifies the given `attrs` set, the result is a list with a | ||
| single element `f [ ] attrs`. |
There was a problem hiding this comment.
Order of recursion? Looks like it traverse the leafs first then going up? Sometimes this can be important to know.
| { path = [ ]; value = attrs; } | ||
| ]; | ||
| }; | ||
|
|
There was a problem hiding this comment.
What would you expect:
collect' (_: v: !isAttrs v) (path: value: { inherit path value; }) { a = 1; b = {}; }
There was a problem hiding this comment.
These are mostly inteded for use with Nix flakes
The motivating use case is a sneaky performance/robustness hazard, by evaluating all packages instead of just the one that's requested, so we probably shouldn't do this, and instead advise users on how to achieve the same result efficiently, for example by using a nesting of concatMapAttrs to create their packages attrsets.
I am very sorry about this, but this is actually a hard problem to solve because this is a fundamental property of "attribute trees" that people want to use, and we can hardly blame them.
|
Note that I'm not opposed to the addition of these functions, but we should make it very very clear that they should be used either
|
wegank
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
|
@tie can we split this up. I'd like to have separate discussions about |
|
A depth first algorithm can stop its traversal at any depth it deems sufficient. I don't think it implies that the whole tree or graph is always traversed in its entirety. |
Description of changes
This change adds an advanced version of lib.collect that exposes the attribute path, and a function to flatten an attribute set. These are mostly inteded for use with Nix flakes where packages are a flat set of derivations. That is, this allows flakes to define packages (or other outputs) as a tree of derivations (or other types) and flatten them before exposing.
In particular, this is useful for flakes that want to expose cross-compiled variants of packages so the user can run, for example,
nix build path:.#go-linux-amd64-v3independent of the current system (as inpackages.${system}). In this case, Go is used as an example since the build system has an excellent support for cross-compilation out-of-the-box (assuming Cgo is disabled).Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)