NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.86k stars 13.93k forks source link

Simplify RFC42 settings declarations using circular programs #144216

Open roberth opened 2 years ago

roberth commented 2 years ago

Describe the challenge

Dealing with types and format functions separately is slightly cumbersome.

Not much of a challenge really, but let me present a potential solution using laziness / a technique called "circular programming".

Status quo

The relevant bits from the example from Detailed design in RFC 42.

let
  settingsFormat = pkgs.formats.json {};
in
{
  options = {
    settings = lib.mkOption {
      type = lib.types.submodule {

        # Declare that the settings option supports arbitrary format values, json here
        freeformType = settingsFormat.type;
      };
    };
  };
  config = {
    environment.etc."foo.json".source =
      # The formats generator function takes a filename and the Nix value
      # representing the format value and produces a filepath with that value
      # rendered in the format
      settingsFormat.generate "foo-config.json" cfg.settings;
  }
}

New situation

let
  fooConfig = pkgs.fileFormats.json {
    name = "foo-config.json";
    value = cfg.settings;
  };
in
{
  options = {
    settings = lib.mkOption {
      type = lib.types.submodule {

        # Declare that the settings option supports arbitrary format values, json here
        freeformType = fooConfig.type;
      };
    };
  };
  config = {
    environment.etc."foo.json".source = fooConfig;  # using outPath
  }
}

All arguments are specified in one place, which simplifies the api.

This should be possible to implement, thanks to laziness. However, the above formulation does create a dependency from some items in the options tree to pkgs, which is defined in config. This may work, because pkgs itself isn't defined through pkgs.fileFormats, but perhaps there's something I'm not seeing. An alternative may be as follows:

  fooConfig = lib.fileFormats.json {
    inherit pkgs;
    name = "foo-config.json";
    value = cfg.settings;
  };

While it is easier to see that this can be implemented without causing too much strictness, it makes lib depend on the interface of pkgs, which is not desirable.

Proof of concept

I've tested the idea in a single file; see details below.

Note thate `builtins.seq` was used to simulate depending on a `pkgs` attribute in an option type. ```diff diff --git a/nixos/modules/virtualisation/containers.nix b/nixos/modules/virtualisation/containers.nix index cea3d51d3ae..8fd026cbf0c 100644 --- a/nixos/modules/virtualisation/containers.nix +++ b/nixos/modules/virtualisation/containers.nix @@ -4,7 +4,30 @@ let inherit (lib) literalExpression mkOption types; - toml = pkgs.formats.toml { }; + tomlFile = builtins.seq pkgs.formats ({ value, name ? "config.toml" }: + let format = pkgs.formats.toml { }; + in { + type = format.type // { description = "TOML value (${name})"; }; + inherit (format.generate name value) outPath; + }); + + containersConf = tomlFile { + value = cfg.containersConf.settings; + name = "containers.conf"; + }; + + storageSettings = tomlFile { + value = cfg.storage.settings; + name = "storage.conf"; + }; + + registriesSettings = tomlFile { + value = { + registries = lib.mapAttrs (n: v: { registries = v; }) cfg.registries; + }; + name = "registries.conf"; + }; + in { meta = { @@ -43,7 +66,7 @@ in }; containersConf.settings = mkOption { - type = toml.type; + inherit (containersConf) type; default = { }; description = "containers.conf configuration"; }; @@ -66,7 +89,7 @@ in }; storage.settings = mkOption { - type = toml.type; + inherit (storageSettings) type; default = { storage = { driver = "overlay"; @@ -138,15 +161,11 @@ in }; }; - environment.etc."containers/containers.conf".source = - toml.generate "containers.conf" cfg.containersConf.settings; + environment.etc."containers/containers.conf".source = containersConf; - environment.etc."containers/storage.conf".source = - toml.generate "storage.conf" cfg.storage.settings; + environment.etc."containers/storage.conf".source = storageSettings; - environment.etc."containers/registries.conf".source = toml.generate "registries.conf" { - registries = lib.mapAttrs (n: v: { registries = v; }) cfg.registries; - }; + environment.etc."containers/registries.conf".source = registriesSettings; environment.etc."containers/policy.json".source = if cfg.policy != {} then pkgs.writeText "policy.json" (builtins.toJSON cfg.policy) ```

Notify maintainers

@roberth @Infinisil

Metadata

Maintainer information:

# a list of nixpkgs attributes affected by the problem
attribute:
  - formats   # not really affected, but potentially obsolete
  - lib.generators
# a list of nixos modules affected by the problem
module:
infinisil commented 2 years ago

I feel like this is a bit too unintuitive. How about just

let
  settingsFormat = pkgs.formats.json {};
  configFile = settingsFormat.generate "foo-config.json" cfg.settings;
in
{
  options = {
    settings = lib.mkOption {
      type = lib.types.submodule {

        # Declare that the settings option supports arbitrary format values, json here
        freeformType = settingsFormat.type;
      };
    };
  };
  config.environment.etc."foo.json".source = configFile;
}
roberth commented 2 years ago

That's just the "status quo" example and then putting generate in a binding, right? It's definitely a step in the right direction, but not that much of a difference.

I would argue that putting everything about the file in a single binding is actually more intuitive, but that just goes to show that "intuitive" is a subjective, ill-defined concept.

What I was actually going for is, considering that we now know how these interfaces are used, we could unify things a bit. I was inspired by this newcomer https://github.com/NixOS/nixpkgs/pull/139075#discussion_r739765668.

config.environment.etc."foo.json".source = configFile;

This is nice. We could set outPath on the "file" attrset. I'll update my example and proof of concept (in the details section).

roberth commented 2 years ago

Done. It is a bit radical, I admit.

roberth commented 2 years ago

But radically intuitive, because it's what a newcomer may expect.

mkg20001 commented 2 years ago

Something that can also be done is instead of using .source, .text could be used

let 
  config = format.output cfg.settings; # .output would need to be added
in
 {
   environment.etc."bla.json".text = config;
 }
roberth commented 2 years ago

instead of using .source, .text could be used

This would require that the final text can be generated at evaluation time and this is how lib.generators operates. While possible for JSON and some simple formats, this isn't always convenient, because we don't get to use remarshal for example. It also precludes validation using other existing tools.

So while it is a valid thing to do, it doesn't make much sense for formats, which is separate from lib.generators for these reasons.

lib.generators does have a reason to exist besides continuity, which is for working with configuration that contains secrets in tools that can work with values without writing them to the store, such as NixOps and nix-instantiate --read-only.

pasqui23 commented 2 years ago

Another proposal:

let
  settingsFormat = pkgs.formats.json {};
in
  {
    imports=[(mkModuleFiles settingsFormat ["services" "foo" "settings"] "bla.json" config.services.foo.enable)];
}
pasqui23 commented 2 years ago

It could even be shortned to

{
  imports=[(mkSettingsModule settingsFormat ["services" "foo"]  "bla.json")];
}

for the common case of options called enable and settings