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.

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/attrsets: add cartesianProductOfSets function #110787

Merged
merged 3 commits into from
Jan 29, 2021

Conversation

tfc
Copy link
Contributor

@tfc tfc commented Jan 25, 2021

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.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 25, 2021
lib/attrsets.nix Outdated Show resolved Hide resolved
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.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 28, 2021
@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.

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
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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants