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

nix: Put config builder into pkgs #44322

Closed
wants to merge 1 commit into from
Closed

nix: Put config builder into pkgs #44322

wants to merge 1 commit into from

Conversation

infinisil
Copy link
Member

This is such that even people not using NixOS can use it to generate the
nix.conf file. See #35662

This doesn't change the semantics in any way, and in fact, config.system.build.etc is exactly the same derivation before and after this change.

Ping @FRidh @7c6f434c @matthewbauer

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@infinisil
nix: Put config builder into pkgs
6857db0 
This is such that even people not using NixOS can use it to generate the
nix.conf file. See NixOS#35662
grahamc
grahamc reviewed Aug 1, 2018
View reviewed changes
pkgs/tools/package-management/nix/config.nix
, nix, lib
}:

{ cfg }:
Copy link
Member

Choose a reason for hiding this comment

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

is cfg here a regular attrset, or a NixOS module style option variable?

Copy link
Member

Choose a reason for hiding this comment

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

If this is intended to be used outside of the module system it currently means that all config options must be defined, unlike with the module system there are no default values in this case.
Using instead { extraOptions ? '', maxJobs }: etc. makes the arguments explicit and simplifies usage outside of nixos for options that make sense without a value.

Copy link
Member

Choose a reason for hiding this comment

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

For non-NixOS users it's nicest to have the options here, along with defaults. At the same time, we shouldn't duplicate work. However, because this derivation would fail to build if certain attributes in cfg are missing, I am of the opinion they should be added directly as parameters of this function, and thus cfg be dropped.

Frankly, I don't mind so much how it looks. Either way works for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could of course (and I'd like this to be more common in nixpkgs) put the module options right next to the Nix package, such that anybody using nixpkgs can use the nice options and what comes with them (type checking, defaults, overridability, merging, etc.), the package would evaluate its own small module system. Then there wouldn't be any duplicated work either, the NixOS module can get the options from the Nix package (as a submodule). This would also work for any other package having a configuration associated with it but the options currently only being usable from NixOS.

Let me know what you think, if people agree I can do this.

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: nix

Partial log (click to expand)

  /nix/store/ljs86xdssnqnqvpa6xnyhvkzfv6w4prg-libsodium-1.0.16
  /nix/store/xzrma1ynwid70yyd13pb9lpzfcq4s0wg-nix-2.0.4-man
copying path '/nix/store/2las7k7566r89sdksa17nak3qizdls32-busybox-1.29.1' from 'https://cache.nixos.org'...
copying path '/nix/store/xzrma1ynwid70yyd13pb9lpzfcq4s0wg-nix-2.0.4-man' from 'https://cache.nixos.org'...
copying path '/nix/store/hlwxhq9m6ick5ki89c0c77kqv8z0cxkd-aws-sdk-cpp-1.4.82' from 'https://cache.nixos.org'...
copying path '/nix/store/hmqfxwn74sxzqy591cz4vn9yyxdda7cq-boehm-gc-7.6.6' from 'https://cache.nixos.org'...
copying path '/nix/store/iz3qa2akd93jhjni846a10i33z4hzd3n-curl-7.59.0' from 'https://cache.nixos.org'...
copying path '/nix/store/ljs86xdssnqnqvpa6xnyhvkzfv6w4prg-libsodium-1.0.16' from 'https://cache.nixos.org'...
copying path '/nix/store/hapi84n3005g5k11pl2zcwgq8maidsm8-nix-2.0.4' from 'https://cache.nixos.org'...
/nix/store/hapi84n3005g5k11pl2zcwgq8maidsm8-nix-2.0.4

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: nix

Partial log (click to expand)

/nix/store/nvisdvcr1vbdc0h8yjp33py0kwbd6q4x-nix-2.0.4

FRidh
FRidh reviewed Aug 2, 2018
View reviewed changes
pkgs/tools/package-management/nix/config.nix
, nix, lib
}:

{ cfg }:
Copy link
Member

Choose a reason for hiding this comment

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

If cfg stays, make it a lambda instead?

@7c6f434c
Copy link
Member

7c6f434c commented Aug 3, 2018

Listing all the parameters with no defaults provided seems annoying and britle… maybe load option definitions from the module and use them to provide defaults?

@edolstra
Copy link
Member

edolstra commented Aug 3, 2018

Not really in favor of this, since this kind of function has basically no discoverability: there is no way to discover or use this function without reading the source code.

Also, we should really stop adding things that are not packages (such as functions) to pkgs.

@edolstra
Copy link
Member

edolstra commented Aug 3, 2018

People who want this should probably just use the nix-daemon module directly, e.g.

(import <nixpkgs/nixos/lib/eval-config.nix> {
  system = "x86_64-linux";
  modules =
    [ <nixpkgs/nixos/modules/services/misc/nix-daemon.nix>
      {
        nix.useSandbox = true;
        nix.buildCores = 32;
      }
    ];
}).config.environment.etc."nix/nix.conf".source

or if you want to avoid evaluating all of NixOS:

(import <nixpkgs/nixos/lib/eval-config.nix> {
  system = "x86_64-linux";
  baseModules = [];
  check = false;
  modules =
    [ <nixpkgs/nixos/modules/services/misc/nix-daemon.nix>
      <nixpkgs/nixos/modules/system/etc/etc.nix>
      <nixpkgs/nixos/modules/misc/nixpkgs.nix>
      {
        nix.useSandbox = true;
        nix.buildCores = 32;
      }
    ];
}).config.environment.etc."nix/nix.conf".source

@infinisil
Copy link
Member Author

@edolstra Good point, that works for pretty much anything from NixOS. It is a bit more cumbersome, but oen doesn't need to generate such a nix config everyday anyways.

@infinisil infinisil closed this Aug 9, 2018
@infinisil infinisil mentioned this pull request Aug 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc grahamc left review comments

@LnL7 LnL7 LnL7 left review comments

@FRidh FRidh FRidh left review comments

Assignees
No one assigned
Labels
6.topic: nixos 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
7 participants
@infinisil @GrahamcOfBorg @7c6f434c @edolstra @grahamc @LnL7 @FRidh