WIP nixos/nix-daemon: Organize buildMachine options with a submodule #47636

Ericson2314 · 2018-10-02T00:20:53Z

Motivation for this change

What's the best way to test this sort of thing?

nixos-option -I nixpkgs=(pwd) nix.buildMachines.foo.bar

did not work for example.

Also, is this reasonable for 18.09 or is that fact that garbage attributes will be rejected a problem?

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.

grahamc · 2018-10-02T00:22:27Z

Too late for 18.09, but I like the idea.

Ericson2314 · 2018-10-02T00:24:25Z

@grahamc my other idea was to check if types.either types.attrs (types.submodule ..) would work to provide documentation but make no errors.

grahamc · 2018-10-02T00:28:10Z

nixos/modules/services/misc/nix-daemon.nix

+              default = "-";
+              description = ''
+                The path to the SSH private key with which to authenticate with
+                the build machine. <literal>"-"</literal> indicates falling back


Probably should either drop the " or at least move the quote out of the literal element.

I think I mean the quote to be there to show the Nix literal one would write, rather than string literal, leaving it to the literal to figure out that - only makes sense as a string.

grahamc · 2018-10-02T00:29:01Z

nixos/modules/services/misc/nix-daemon.nix

+              default = 1;
+              description = ''
+                Something at indicates how fast the machine is relative to an
+                arbitrary norm???


An arbitrary integer to indicate the build performance of this system relative to the other buildMachines.

... higher|lower is faster.

might be best to use unsigned/ints.between 0 100/positive as type.

grahamc · 2018-10-02T00:29:47Z

nixos/modules/services/misc/nix-daemon.nix

+              decriptions = ''
+                A list of features derivations built with this remote may choose
+                to use or not. (See the documentation on Nix itself for what
+                those features are.)


What do you mean by the above parentheticals?

I was hoping somewhere else might actually list valid features, but https://nixos.org/nix/manual/#chap-distributed-builds again just says they exist, other than the "kvm" example.

Ah, there is no list of valid features. You can make them up. I have a machine with the feature "big-mistake-this-ones-impure".

a grep on nixpkgs gives "big-parallel"/"kvm"/"benchmark"/"nixos-test". Even if it stays freefrom, it would be nice to list the default ones with a types.either( enum [ "kvm" ... ] or string).

grahamc · 2018-10-02T00:31:01Z

nixos/modules/services/misc/nix-daemon.nix

+            hostName = mkOption {
+              type = types.string;
+              description = ''
+                The hostname of the build machine.


Maybe note this hostname has to resolve.

what good can be hostname that doesn't resolve ?

grahamc · 2018-10-02T00:31:35Z

nixos/modules/services/misc/nix-daemon.nix

+              type = types.listOf types.string;
+              default = [];
+              description = ''
+                The system types the build machine can execute derivations on.


How about some examples on the above two?

Ericson2314 · 2018-10-02T00:41:21Z

Thanks @grahamc, I'll fix these later tonight.

mmahut · 2019-08-25T18:28:47Z

Are there any updates on this pull request, please?

teto · 2019-08-26T02:21:17Z

may be ok for 19.09. I can test once the patch is fixed.

ivan · 2019-09-06T15:54:27Z

For the author, reviewers, and committers: this PR was scanned and appears to add a use of the deprecated types.string, which emits a warning as of #66346. Before merging, please change this to another type, possibly:

types.str for a single string where merging does not make sense, or cannot work
types.lines for multi-line configuration or scripts where merging is possible
types.listOf types.str for a mergeable list of strings

infinisil · 2019-09-30T14:31:56Z

nixos/modules/services/misc/nix-daemon.nix

+            };
+            sshKey = mkOption {
+              type = types.string;
+              default = "-";


Because of #69900, it would be a good idea to force this option to not have a value from the store. This can be done with an apply = toString

This isn't enough to guarantee this at all actually. See #78640 for a better way in the future. For now this should be just types.str

matthewbauer · 2020-05-13T20:27:02Z

nixos/modules/services/misc/nix-daemon.nix

+              '';
+            };
+            system = mkOption {
+              type = types.string;


should use "nullOr"

infinisil · 2020-05-13T21:25:27Z

#83104 is an updated version of this, closing

nixos/nix-daemon: Organize buildMachine options with a submodule

bef47ff

Ericson2314 requested review from edolstra, matthewbauer and dezgeg October 2, 2018 00:21

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Oct 2, 2018

grahamc reviewed Oct 2, 2018

View reviewed changes

ryantm added the 2.status: work-in-progress label Feb 24, 2019

infinisil mentioned this pull request Sep 30, 2019

sshKey in nix.buildMachines can inadvertently be imported into the nix store #69900

Closed

infinisil reviewed Sep 30, 2019

View reviewed changes

Valodim mentioned this pull request Mar 21, 2020

nixos/nix-daemon: Organize buildMachine options with a submodule #83104

Merged

matthewbauer reviewed May 13, 2020

View reviewed changes

nixos/modules/services/misc/nix-daemon.nix

'';

};

system = mkOption {

type = types.string;

Copy link

Member

matthewbauer May 13, 2020

Choose a reason for hiding this comment

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

should use "nullOr"

infinisil closed this May 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP nixos/nix-daemon: Organize buildMachine options with a submodule #47636

WIP nixos/nix-daemon: Organize buildMachine options with a submodule #47636

Ericson2314 commented Oct 2, 2018

grahamc commented Oct 2, 2018

Ericson2314 commented Oct 2, 2018

grahamc Oct 2, 2018

Ericson2314 Dec 5, 2018

grahamc Oct 2, 2018

grahamc Oct 2, 2018

teto Oct 2, 2018

grahamc Oct 2, 2018

Ericson2314 Oct 2, 2018

grahamc Oct 2, 2018

Ericson2314 Oct 2, 2018

teto Oct 2, 2018

grahamc Oct 2, 2018

teto Oct 2, 2018

grahamc Oct 2, 2018

Ericson2314 commented Oct 2, 2018

mmahut commented Aug 25, 2019

teto commented Aug 26, 2019

ivan commented Sep 6, 2019

infinisil Sep 30, 2019

infinisil Mar 21, 2020

matthewbauer May 13, 2020

infinisil commented May 13, 2020

WIP nixos/nix-daemon: Organize buildMachine options with a submodule #47636

WIP nixos/nix-daemon: Organize buildMachine options with a submodule #47636

Conversation

Ericson2314 commented Oct 2, 2018

Motivation for this change

Things done

grahamc commented Oct 2, 2018

Ericson2314 commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Oct 2, 2018

mmahut commented Aug 25, 2019

teto commented Aug 26, 2019

ivan commented Sep 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented May 13, 2020