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

merge some of flake-utils into nixpkgs #123117

Closed
wants to merge 3 commits into from

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented May 15, 2021

Motivation for this change

90% of the flakes out there are using flake-utils' eachDefaultSystem.

Since all the flakes that use flake-utils also use nixpkgs as a dependency,
move that functionality here.

Before:

{
  description = "Flake utils demo";

  inputs.flake-utils.url = "github:numtide/flake-utils";

  outputs = { self, nixpkgs, flake-utils }:
    flake-utils.lib.eachDefaultSystem (system:
      let pkgs = nixpkgs.legacyPackages.${system}; in
      rec {
        packages = {
          hello = pkgs.hello;
        };
        defaultPackage = packages.hello;
      }
    );
}

After:

{
  description = "Flake utils demo";

  outputs = { self, nixpkgs }:
    nixpkgs.lib.mapAttrsJump (nixpkgs.lib.systems.tier1 ++ lib.systems.tier2) (system:
      let pkgs = nixpkgs.legacyPackages.${system}; in
      rec {
        packages = {
          hello = pkgs.hello;
        };
        defaultPackage = packages.hello;
      }
    );
}
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.

zimbatm added 2 commits May 15, 2021 16:13
Make the list of systems that we support as tier 1 and tier 2 available
to the user.
@zimbatm
Copy link
Member Author

zimbatm commented May 15, 2021

Type of feedback I am looking for:

  • mapAttrsJump: I invented this name because I didn't find prior code literature. If you find other examples out there I am interested.
  • lib.systems.tier{1,2}: I'm not super familiar with the more complicated constructs. Are the strings acceptable here?

@zimbatm
Copy link
Member Author

zimbatm commented May 15, 2021

Potentially, this PR could also gain a lib.systems.eachDefault for the common case. And if you have a full list of all the tiers, happy to add those as well.

@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 May 15, 2021
*
* Example:
* mapAttrsJump ["x86_64-linux"] (system: { hello = 42; })
* => { x86_64-linux = { hello = 42; }; }
Copy link
Member

Choose a reason for hiding this comment

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

This example looks like the regular genAttrs. Probably should be something like { hello = { x86_64-linux = 42; }; }.

lib/attrsets.nix Outdated Show resolved Hide resolved
Co-authored-by: Jan Tojnar <jtojnar@gmail.com>
@figsoda figsoda mentioned this pull request Oct 4, 2021
12 tasks
@stale
Copy link

stale bot commented Jan 9, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 9, 2022
@lilyball
Copy link
Member

lilyball commented Jan 18, 2022

I would really like to have some of this in nixpkgs, but I don't think this is the right approach. mapAttrsJump is mildly confusing the way it's written, and it's rather specialized, and it requires picking the systems to support which is just likely to cause a lot of flakes to pick the wrong set (such as picking just tier1) when they don't actually have any reason to restrict it. I'd rather see this be a little more flake-focused, and in particular I'd prefer to see flake-utils's eachSystem and eachDefaultSystem just copied as-is.

eachDefaultSystem could potentially be renamed to something like eachHydraSystem, but that's a little more opaque and not as approachable by folks who aren't familiar with hydra. The name eachDefaultSystem is much more obviously the right thing to pick for most people. It could still be defined in terms of lib.systems.supported.hydra but this could be considered an implementation detail (e.g. nixpkgs should feel free to add systems to the list even if they're not supported by hydra, though I don't know if there's a reason to ever do so).

Given the flake-specific nature of this, I would propose that this go into a new lib.flakes library. And flake-utils's allSystems should probably be copied into lib.systems somewhere. I don't know if the list corresponds with anything that exists in nixpkgs already, but I don't see anything obvious in lib.systems that matches it. (EDIT: It comes from lib.platforms.all)


Also as a side note, lib.systems.supported.tier1/tier2/tier3 appears unusable. I see this PR has a TODO referencing an RFC, but the tier1/tier2 lists don't even include all hydra platforms, and in particular don't include aarch64-darwin. nixpkgs should most definitely not encourage a standard flake pattern that does not support the current crop of Apple hardware.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 18, 2022
@zimbatm
Copy link
Member Author

zimbatm commented May 23, 2022

I'll close this because I don't have the energy to push through these changes. Hopefully, this will send the signal to somebody else to take over. I still think that something the useful subset of flake-utils should be added to nixpkgs directly.

@zimbatm zimbatm closed this May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants