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

nixos/nix-daemon: use structural settings #139075

Merged
merged 1 commit into from
Jan 27, 2022
Merged

nixos/nix-daemon: use structural settings #139075

merged 1 commit into from
Jan 27, 2022

Conversation

polykernel
Copy link
Contributor

@polykernel polykernel commented Sep 23, 2021

The nix.* options, apart from options for setting up the
daemon itself, currently provide a lot of setting mappings
for the Nix daemon configuration. The scope of the mapping yields
convience, but the line where an option is considered essential
is blurry. For instance, the extra-sandbox-paths mapping is
provided without its primary consumer, and the corresponding
sandbox-paths option is also not mapped.

This increases the maintenance burden as maintainers have to
closely follow upstream changes which in the case of Nix is made
worse by having to juggle between two state versions, the stable release
and the unstable release, which may include or deprecate features.

One conditional option publicHostKey was added to the buildMachines
submodule corresponding to the newly added base64 encoded public host
key setting exposed in the builder syntax. The build machine generation
was subsequently rewritten to use concatStringsSep to group concatenations.

This commit aims to following the standard outlined in RFC 42[1] to
implement a structural setting pattern. The Nix configuration is encoded
at its core as key-value pairs which maps nicely to attribute sets, making
it feasible to express in the Nix language itself. Some existing options are
kept such as buildMachines and registry which present a simplified interface
to managing the respective settings.

Legacy configurations are mapped to their corresponding options under the
structural settings for backwards compatibility.

Various options settings in other nixos modules and relevant tests have been
updated to use the structural setting for consistency.

The generation and validation of the configration file has been modified to
use writeTextFile instead of runCommand for clarity. Note that validation
is now mandatory as strict checking of options has been pushed down to the
derivation level due to freeformType consuming unmatched options. Furthermore,
validation can not occur when cross-compiling due to current limitations.

[1] - https://github.com/NixOS/rfcs/blob/master/rfcs/0042-config-option.md

Motivation for this change

Initial effort for nix-community/home-manager#2324

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages 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/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/local-flake-based-nix-search-nix-run-and-nix-shell/13433/12


settings = mkOption {
type = types.submodule {
freeformType = semanticConfType;
Copy link
Member

Choose a reason for hiding this comment

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

Having a freeformType means that we don't have implicit checking for typos anymore.
Can we check the config file in a derivation using nix.package?
Otherwise this is somewhat of a regression. For some modules this would be an acceptable trade-off, but for such a core element of NixOS it is not.
Perhaps using something like

HOME=..... nix show-config |& sed -e 's/^warning:/error:/' | (! grep unknown\ setting)

Copy link
Contributor Author

@polykernel polykernel Oct 22, 2021

Choose a reason for hiding this comment

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

You are right, using freeformType causes typos to remain silent. I think we can incorporate checking into the checkPhase of the derivation which generates nix.conf, but this has the side effects that validation for options specified in the submodule do not occur at the module system level. This essentially means typos and invalid settings are now indistinguishable. This is ok but checking will always have to occur instead of being optional.

An alternate approach is to push the freeformType one level down, so nix.settings becomes a standard submodule, and a descendant typed option like nix.settings.extraConfig is used to for additional settings. This also means merging behavior of the the extra settings and the "main" settings have to be handled manually and outside of the module system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the first approach(your suggestion) is easier to implement and less error prone, but I am not sure if having the error/warning at the derivation level as opposed to the module system level is acceptable.

Copy link
Contributor

@ShamrockLee ShamrockLee Oct 31, 2021

Choose a reason for hiding this comment

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

but I am not sure if having the error/warning at the derivation level as opposed to the module system level is acceptable.

I'm still a novice here. IMHO, the modular configurations eventually comes down to the derivation that generates the nix.conf file. As the generation process itself is already outside (or underneath) the modular expressions, the vlidation of the output file nix.conf would not create an extra "non-modulalized" part.

By the way, there are some modules who use the generator attribute provided by type to generate the output file, where the type attribute is specified under the options tree. Maybe the validation could be specified as part of the type or options to go even more "modular". This would probably go through some "consesus-reaching" process such as an additional issue or an RFC, and modules using a rather imperative way of validation would then be modified to fit into the standardized pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, there are some modules who use the generator attribute provided by type to generate the output file, where the type attribute is specified under the options tree. Maybe the validation could be specified as part of the type or options to go even more "modular".

Could you elaborate on the validation mechanism, I am not sure I understand correctly but do you mean using lib.generators to generate the config which itself and specifying a schema under an option which gets match to its value? The issue when I thought about it is using a schema which is analogous to the mapping all config options in the options tree is it has to be total and essentially match upstream, which increases maintenance burden and is what RFC 42 tries to avoid.

Copy link
Contributor

@ShamrockLee ShamrockLee Nov 2, 2021

Choose a reason for hiding this comment

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

I cannot recall where I saw them. Maybe I have mixed something up. Sorry for that.

It seems that the generators produce strings instead of files. It then makes more sense to attach the validation through package overriding or similar approaches.

Copy link
Member

Choose a reason for hiding this comment

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

@polykernel

not sure if having the error/warning at the derivation level as opposed to the module system level is acceptable.

While earlier is better, it is still early enough, as derivation builds are also free of side effects.

@ShamrockLee You're thinking of pkgs.formats. Not being in lib, those can produce a derivation. It's also a better name :)

Maybe the validation could be specified as part of the type or options to go even more "modular". This would probably go through some "consesus-reaching" process such as an additional issue or an RFC, and modules using a rather imperative way of validation would then be modified to fit into the standardized pattern.

The separation we have right now, with formats producing a type that returns plain Nix values instead of a derivation, is quite useful because it allows modules to use values from the config. This wouldn't be possible if the type converted the settings to a derivation directly.
I suppose we could use the formats as 'circular programs' though, taking more advantage of laziness. See #144216

Copy link
Member

@dasJ dasJ left a comment

Choose a reason for hiding this comment

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

Big 👍 for making the settings consistent with the nix manual and for using a much nicer way of specifying them. However I am not sure if this should go into the 21.11 release or if we should wait for 22.05. In any case we should probably wait for the branch-off and backport to 21.11 if necessary

@ShamrockLee
Copy link
Contributor

It has been a while after the 21.11 branch-off.

Looking forward to its merging!

@thiagokokada
Copy link
Contributor

Is there something blocking this merge?

(Except from the conflicts).

@roberth
Copy link
Member

roberth commented Jan 21, 2022

I don't see any blockers and the conflict seems simple.

@polykernel
Copy link
Contributor Author

I have resolved the conflict and updated the publicHostKey option to be static.

@ShamrockLee
Copy link
Contributor

As the target changelog release note is 21.11, this is to be back-ported to release-21.11. Is that right?

It seems alright as the backward-compatibility is kept with the mapping layer.

@dasJ
Copy link
Member

dasJ commented Jan 26, 2022

Thanks for pointing this out, I was close to merging this ;)
I don't think we should backport this since it introduces a lot of warnings for existing setups and the large size of this PR leaves room for regressions.
If the release notes are moved to 22.05, I will happily merge this (unless @roberth or anyone else intervenes)

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Small things and off-topic things. Looking pretty good otherwise.

Comment on lines +46 to +50
nix.settings = {
substituters = lib.mkForce [];
hashed-mirrors = null;
connect-timeout = 1;
};
Copy link
Member

Choose a reason for hiding this comment

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

There's a bunch of these. Should probably be the default for VM tests. Not a requirement for this PR though.

nixos/modules/services/misc/nix-daemon.nix Outdated Show resolved Hide resolved
Comment on lines 693 to 706
assertions =
let badMachine = m: m.system == null && m.systems == [];
in [
let badMachine = m: m.system == null && m.systems == [ ];
in
[
{
assertion = !(builtins.any badMachine cfg.buildMachines);
assertion = !(any badMachine cfg.buildMachines);
message = ''
At least one system type (via <varname>system</varname> or
<varname>systems</varname>) must be set for every build machine.
Invalid machine specifications:
'' + " " +
(builtins.concatStringsSep "\n "
(builtins.map (m: m.hostName)
(builtins.filter (badMachine) cfg.buildMachines)));
(concatStringsSep "\n "
(map (m: m.hostName)
(filter (badMachine) cfg.buildMachines)));
}
];
Copy link
Member

Choose a reason for hiding this comment

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

This would benefit from a second attempt at submodule assertions, #97023 (comment), both in code complexity and error message quality.
(off-topic)

nixos/doc/manual/from_md/release-notes/rl-2111.section.xml Outdated Show resolved Hide resolved
The `nix.*` options, apart from options for setting up the
daemon itself, currently provide a lot of setting mappings
for the Nix daemon configuration. The scope of the mapping yields
convience, but the line where an option is considered essential
is blurry. For instance, the `extra-sandbox-paths` mapping is
provided without its primary consumer, and the corresponding
`sandbox-paths` option is also not mapped.

The current system increases the maintenance burden as maintainers have to
closely follow upstream changes. In this case, there are two state versions
of Nix which have to be maintained collectively, with different options
avaliable.

This commit aims to following the standard outlined in RFC 42[1] to
implement a structural setting pattern. The Nix configuration is encoded
at its core as key-value pairs which maps nicely to attribute sets, making
it feasible to express in the Nix language itself. Some existing options are
kept such as `buildMachines` and `registry` which present a simplified interface
to managing the respective settings. The interface is exposed as `nix.settings`.

Legacy configurations are mapped to their corresponding options under `nix.settings`
for backwards compatibility.

Various options settings in other nixos modules and relevant tests have been
updated to use structural setting for consistency.

The generation and validation of the configration file has been modified to
use `writeTextFile` instead of `runCommand` for clarity. Note that validation
is now mandatory as strict checking of options has been pushed down to the
derivation level due to freeformType consuming unmatched options. Furthermore,
validation can not occur when cross-compiling due to current limitations.

A new option `publicHostKey` was added to the `buildMachines`
submodule corresponding to the base64 encoded public host key settings
exposed in the builder syntax. The build machine generation was subsequently
rewritten to use `concatStringsSep` for better performance by grouping
concatenations.

[1] - https://github.com/NixOS/rfcs/blob/master/rfcs/0042-config-option.md
teto pushed a commit to teto/home-manager that referenced this pull request Aug 22, 2022
Nix permits user level configurations through ~/.config/nix/nix.conf that allow
customization of system-wide settings and behavior. This is beneficial in chroot
environments and for per-user configurations. System level Nix configurations in the
form of /etc/nix/nix.conf can be specified declaratively via the NixOS nix module but as
of currently no counter part exists in home-manager.

This PR is a port of the RFC42 implementation for the NixOS nix module[1]
to home-manager. Non-applicable options have been excluded and the config generation
backends have been tweaked to the backends offered by home-manager. A notable change
from the NixOS module is a mandatory option to specify the Nix binary corresponding
to the version "nix.conf" should be generated against. This is necessary because
the validation phase is dependent on the `nix show-config` subcommand on the host platform.
While it is possible to avoid validation entirely, the lack of type checking was deemed too significant.
In NixOs, the version information can be retrieved from the `package` option itself which
declares the Nix binary system-wide. However in home-manager, there is no pure way to
detect the system Nix version and what state version the "nix.conf" should be generated
against. Thus an option is used to overcome this limitation by forcing the user to
specify the Nix package. Note this interaction can still be automated by forwarding
the system-wide Nix package to the home-manager module if needed.

Three unit tests were added to test the module behavior for the empty settings, the example
settings and the example registry configurations respectively.

[1] - NixOS/nixpkgs#139075
domenkozar pushed a commit to cachix/cachix that referenced this pull request Dec 14, 2022
spacekookie pushed a commit to spacekookie/home-manager that referenced this pull request Feb 10, 2023
Nix permits user level configurations through ~/.config/nix/nix.conf that allow
customization of system-wide settings and behavior. This is beneficial in chroot
environments and for per-user configurations. System level Nix configurations in the
form of /etc/nix/nix.conf can be specified declaratively via the NixOS nix module but as
of currently no counter part exists in home-manager.

This PR is a port of the RFC42 implementation for the NixOS nix module[1]
to home-manager. Non-applicable options have been excluded and the config generation
backends have been tweaked to the backends offered by home-manager. A notable change
from the NixOS module is a mandatory option to specify the Nix binary corresponding
to the version "nix.conf" should be generated against. This is necessary because
the validation phase is dependent on the `nix show-config` subcommand on the host platform.
While it is possible to avoid validation entirely, the lack of type checking was deemed too significant.
In NixOs, the version information can be retrieved from the `package` option itself which
declares the Nix binary system-wide. However in home-manager, there is no pure way to
detect the system Nix version and what state version the "nix.conf" should be generated
against. Thus an option is used to overcome this limitation by forcing the user to
specify the Nix package. Note this interaction can still be automated by forwarding
the system-wide Nix package to the home-manager module if needed.

Three unit tests were added to test the module behavior for the empty settings, the example
settings and the example registry configurations respectively.

[1] - NixOS/nixpkgs#139075
roberth added a commit to hercules-ci/nixpkgs that referenced this pull request Dec 1, 2023
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)
github-actions bot pushed a commit that referenced this pull request Dec 8, 2023
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
#139075 (comment)

(cherry picked from commit 2d0f4a7)
github-actions bot pushed a commit that referenced this pull request Dec 8, 2023
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
#139075 (comment)

(cherry picked from commit 2d0f4a7)
dansbandit pushed a commit to dansbandit/nixpkgs that referenced this pull request Dec 27, 2023
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)
bacchanalia added a commit to awakesecurity/nixpkgs that referenced this pull request May 7, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
bacchanalia added a commit to awakesecurity/nixpkgs that referenced this pull request May 7, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request May 10, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request May 10, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request May 16, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
awake-bot pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jun 6, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jun 6, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jun 6, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jun 6, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jun 7, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jun 7, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jun 7, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
awake-bot pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jul 3, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jul 3, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jul 3, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jul 4, 2024
Otherwise it shows a deprecation warning, which is escalated to
an error. For context, see
NixOS#139075 (comment)

backports: 2d0f4a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet