Skip to content
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.

Sign up for GitHub

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

Jump to bottom

lib/attrsets: add cartesianProductOfSets function #110787

Merged
merged 3 commits into from Jan 29, 2021
Merged

lib/attrsets: add cartesianProductOfSets function #110787

merged 3 commits into from Jan 29, 2021

Conversation

tfc
Copy link
Contributor

@tfc tfc commented Jan 25, 2021 •
edited

Motivation for this change

For library maintainers and other projects it is useful to build a project with multiple library and toolchain combinations.
For example, in C++ libraries it might be worthwhile to take the derivation and override it with different build profile settings and toolchains.

This might look like this:

  buildConfs = lib.cartesianProductOfSets {
    cc = with pkgs; [ clang_9 clang_10 gcc7 gcc8 gcc9 gcc10 ];
    buildType = [ "Debug" "Release" ];
  };
  builds = let
    buildFunction = { cc, buildType }: lib.nameValuePair
      "${cc.name}-${lib.toLower buildType}"
      (
        callPackage ./build.nix {
          inherit buildType;
          stdenv = overrideCC stdenv cc.value;
        }
      );
  in
    builtins.listToAttrs (builtins.map buildFunction buildConfs);
  # this results in builds = { gcc9-release = ...; gcc9-debug = ...; clang_9-release = ...; ...etc... }

implemented the function cartesianProductOfSets along with nix unit tests

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

blitz
blitz reviewed Jan 25, 2021
View reviewed changes
lib/attrsets.nix Outdated Show resolved Hide resolved
infinisil
infinisil reviewed Jan 25, 2021
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there's also lists.crossList which does almost the same:

lists.crossLists (a: b: { inherit a b; }) [ [ 1 2 ] [ 10 20 ] ]
-> [ { a = 1; b = 10; } { a = 1; b = 20; } { a = 2; b = 10; } { a = 2; b = 20; } ]

Not sure if there's some unified way to implement these two

lib/attrsets.nix Outdated Show resolved Hide resolved
@blitz
Copy link
Contributor

blitz commented Jan 27, 2021

@tfc Do you think that this function could be implemented in terms of crossLists or vice versa? Or do they overlap so much that its not worth having both?

@tfc
Copy link
Contributor Author

tfc commented Jan 27, 2021

@blitz @infinisil i tried implementing cartesianProductOfSets in terms of crossLists and with the help of a colleague came up with:

  cartesianProductOfSets = set:
    let
      extendSetOrFunction = setOrFunction: attrName:
        if builtins.isFunction setOrFunction
        then a: extendSetOrFunction (setOrFunction a) attrName
        else a: setOrFunction // { ${attrName} = a; };
    in
    pkgs.lib.crossLists
      (pkgs.lib.foldl extendSetOrFunction { } (builtins.attrNames set))
      (builtins.attrValues set);

not sure how much better this is.

@infinisil
Copy link
Member

Tbh I think we can just deprecate crossLists. I think it's just an inferior version of this function here, and any usages of crossLists can be replaced with a more easily-readable version of this function.

@tfc tfc force-pushed the cartesian-product branch 3 times, most recently from 80f79e5 to 078667e Compare January 28, 2021 10:42
@tfc
Copy link
Contributor Author

tfc commented Jan 28, 2021

@infinisil i deprecated crossLists.

blitz
blitz approved these changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@blitz blitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came out very nicely!

@blitz blitz requested a review from ehmry January 28, 2021 13:24
ehmry
ehmry approved these changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@ehmry ehmry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

@infinisil
Copy link
Member

Nice! Though the formatting makes this hard to review. Since we don't yet have a policy for auto-formatters I think we should avoid them for the time being.

@tfc
Copy link
Contributor Author

tfc commented Jan 28, 2021

@infinisil shall i roll back the nixpkgs-fmt application to the code?

@infinisil
Copy link
Member

@tfc Yes please. I really hope we can adopt a formatter for all of nixpkgs eventually though :)

@tfc
Copy link
Contributor Author

tfc commented Jan 28, 2021

@infinisil done

jonringer
jonringer approved these changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff LGTM

@infinisil infinisil merged commit aa48e20 into NixOS:master Jan 29, 2021
@tfc tfc deleted the cartesian-product branch January 30, 2021 16:12
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/lib-crosslists-is-deprecated-use-lib-cartesianproductofsets-instead/41824/2

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/lib-crosslists-is-deprecated-use-lib-cartesianproductofsets-instead/41824/3

@gvolpe gvolpe mentioned this pull request Mar 24, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blitz blitz blitz approved these changes

@ehmry ehmry ehmry approved these changes

@jonringer jonringer jonringer approved these changes

@infinisil infinisil infinisil approved these changes

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@flokli flokli Awaiting requested review from flokli

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

@Profpatsch Profpatsch Awaiting requested review from Profpatsch

Assignees
No one assigned
Labels
6.topic: nixos 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tfc @blitz @infinisil @nixos-discourse @ehmry @jonringer