flake: introduce special + syntax to combine sets at invocation time#2429
flake: introduce special + syntax to combine sets at invocation time#2429charludo wants to merge 2 commits into
+ syntax to combine sets at invocation time#2429Conversation
sespiros
left a comment
sespiros
left a comment
There was a problem hiding this comment.
Thanks, this indeed looks technically neat and I am happy to merge since this is dev-only and also easily reversible, but I do have some concerns (that I tried to express in #2405 (comment)) on how useful this added complexity will be. Could you tell me some concrete use cases you have in mind before we start building on this? Other than this high level comment/question, just some inline comments.
To share my thoughts a bit, in the concrete use cases I had so far (badaml tests), I am not sure if this composability would bring that much benefit other than simply taking the debug overrides out of the badaml sets. Most of the other overrides couldn't be split out in separate composable sets that would be useful to any other combination of sets. Could I split out qemu-wrapped too as a reusable set for instance? I could create a qemu-acpi set (that overrides qemu-wrapped with the withACPITable flag) and a qemu-serial set (for the withSerialLog flag). But then, does running ... .#badaml-vuln+debug+qemu-serial+qemu-acpi.foo vs ... .#badaml-vuln.foo make my life any easier than simply having a set which is monolithic and per use-case? That actually wouldn't even be possible due to the limit of 3 which I believe is there so that the possibilities don't explode. Moreover there are cross-dependencies, i.e supplying withACPITable to qemu without specifying withACPIDebug to kernel-uvm is not useful so now I need to be aware of that vs having a per-use case set in that folder. That's why in #2405 I opted for this simple approach of prepending sets with _ to indicate a part of a set. otoh we could have both (parts of sets and composable sets and only allow composing sets).
Other concrete upcoming use cases I see is the oak set and the runtime-rs set but will we want to compose these/is it meaningful to be able to compose all those different combinations? If it is, it seems much more intentional to me to see a set in that folder named runtime-rs_oak that indicates that we do care about that particular combination.
Then if it's only some flavor of debug that we want to attach in these per-use-case sets, I think that could be done in some simpler and more straightforward way.
| ]; | ||
|
|
||
| sets = setsFromDirectory ./overlays/sets; | ||
| setsDir = ./overlays/sets; |
There was a problem hiding this comment.
AI comment suggestion, feel free to keep or drop.
| setsDir = ./overlays/sets; | |
| # Sets are built from the overlays in ./overlays/sets. Beyond the single-file | |
| # sets, any combination of up to maxCombineDepth overlays is exposed under a | |
| # name joining the set names with "+". Each combination is evaluated once and | |
| # stored under its sorted (canonical) name; every permutation of the names is | |
| # registered as an alias of that same evaluation, so the order the user types | |
| # does not matter and does not cost an extra eval. | |
| setsDir = ./overlays/sets; |
| setsDir = ./overlays/sets; | ||
| setNames = map (nixpkgs.lib.removeSuffix ".nix") (builtins.attrNames (builtins.readDir setsDir)); | ||
|
|
||
| nonEmptySubsets = |
There was a problem hiding this comment.
Add a comment on what this function does in a similar spirit to the one setsFromDirectory had. Same for the rest of the functions that build towards the final attribute set (permutations, canonicalName, canonicalSets, sets).
|
|
||
| sets = setsFromDirectory ./overlays/sets; | ||
| setsDir = ./overlays/sets; | ||
| setNames = map (nixpkgs.lib.removeSuffix ".nix") (builtins.attrNames (builtins.readDir setsDir)); |
There was a problem hiding this comment.
See my high level comment too but what do you think about excluding here sets starting with "_" to allow for parts of sets?
| canonicalName = | ||
| subset: nixpkgs.lib.concatStringsSep "+" (nixpkgs.lib.sort builtins.lessThan subset); | ||
|
|
||
| subsets = builtins.filter (s: builtins.length s <= 3) (nonEmptySubsets setNames); |
There was a problem hiding this comment.
Extract literal 3 as maxCombineDepth or equivalent.
| canonicalSets = builtins.listToAttrs ( | ||
| map (s: { | ||
| name = canonicalName s; | ||
| value = mkSet ((defaultOverlays (canonicalName s)) ++ map (n: import (setsDir + "/${n}.nix")) s); |
There was a problem hiding this comment.
This is nitpicking/not sure if intentional but the overlays here are applied in the order of s but s comes from nonEmptySets which is in reverse alphabetical order. So applying a+b applies the overlays in reverse order.
2 participants
This is a suggestion on how to combine arbitrary sets. Any permutation of set names joined by
+accesses the package set with all specified overlays applied. To keep eval time down and to prevent accidental overrides, set names are internally sorted after splitting on+.