WIP: feat: Namespaced includes #3

Closed
austreelis wants to merge 6 commits from austreelis/feat/namespaced-includes into main AGit
First-time contributor
No description provided.
austreelis added 2 commits 2024-06-18 14:47:22 +00:00
This commit adds some logic to the modules normalization process to
allow including modules under a user-defined namespace. It achieves it
by:
- flattening any attribute sets in `includes` that are non-empty nor
  contain any of a module's valid attributes (`lib.modules.VALID_KEYS`).
- Erroring out on dupplicate namespaces.
- Mapping namespaced includes do normal modules declaring an option
  `${namespace}` of the include as a submodule.

This allows specifying includes in a module like:
```
{
  includes.mynamespace0 = ./mymodule0.nix;
  includes.mynamespace1 = ./mymodule1.nix;
}
```

and is approximatively desugared by `lib.modules.normalize` into:
```
{
  includes = [
    { options.mynamespace0 = lib.submodule mymodule0.nix; }
    { options.mynamespace1 = lib.submodule mymodule1.nix; }
  ];
}
```

This was inspired by nixpkgs' `lib.modules.doRename`.
austreelis force-pushed austreelis/feat/namespaced-includes from d35d70d513 to 211518bb61 2024-06-18 14:56:43 +00:00 Compare
austreelis changed title from feat: Namespaced includes to WIP: feat: Namespaced includes 2024-06-18 14:59:29 +00:00
jakehamilton reviewed 2024-06-18 16:36:22 +00:00
@ -169,0 +177,4 @@
if
builtins.isAttrs include
&& include != { }
&& !builtins.any (n: builtins.elem n lib.modules.VALID_KEYS) (builtins.attrNames include)
Owner

There may be issues with this approach if we end up doing shorthand syntax for all modules. I don't think that we will, but submodules do support it for convenience. I wonder if we can have a different kind of object used for namespaces includes. What about something like:

includes = [
  { namespace = "my-namespace"; module = ./mod.nix }
];

At the very least we can switch on whether include ? namespace is a thing.

There may be issues with this approach if we end up doing shorthand syntax for all modules. I don't _think_ that we will, but submodules do support it for convenience. I wonder if we can have a different kind of object used for namespaces includes. What about something like: ```nix includes = [ { namespace = "my-namespace"; module = ./mod.nix } ]; ``` At the very least we can switch on whether `include ? namespace` is a thing.
austreelis marked this conversation as resolved
@ -169,0 +204,4 @@
in
builtins.foldl' filter [ ] flattened;
doNamespace =
Owner

nit: maybe a name like createNamespacedModule would be informative here?

nit: maybe a name like `createNamespacedModule` would be informative here?
austreelis marked this conversation as resolved
@ -169,0 +219,4 @@
description = "include ${namespace}";
};
};
# config.${namespace} = lib.modules.override 1000 {};
Owner

Is this comment needed?

Is this comment needed?
Author
First-time contributor

No, I thought I left it out when cleaning up my patch.

No, I thought I left it out when cleaning up my patch.
austreelis marked this conversation as resolved
@ -169,0 +222,4 @@
# config.${namespace} = lib.modules.override 1000 {};
};
in
builtins.seq throwOnConflict builtins.map doNamespace flattened;
Owner

Can we extract the builtins.map doNamespace flattened bit to a variable so that this value is easier to read? Something like builtins.seq throwOnConflict namespacedModules might make it easier to understand.

Can we extract the `builtins.map doNamespace flattened` bit to a variable so that this value is easier to read? Something like `builtins.seq throwOnConflict namespacedModules` might make it easier to understand.
austreelis marked this conversation as resolved
@ -172,3 +230,2 @@
__file__ = builtins.toString module.__file__ or file;
__key__ = builtins.toString module.__key__ or key;
includes = module.includes or [];
includes = normalizeIncludes module.includes or [ ];
Owner

Can we wrap usage of or in parens here? It can be a bit confusing otherwise.

Can we wrap usage of `or` in parens here? It can be a bit confusing otherwise.
austreelis marked this conversation as resolved
Owner

What is the rationale for this being a submodule?

What is the rationale for this being a submodule?
Author
First-time contributor

I'm not sure what you're referring to as "this", if it's something specific I'm not seeing it (maybe I'm confused by the UI).

If it's in general, what's the rationale for modules to be included under a namespace as a submodule, I think the main point is that it gives more control e.g. visibility in documentation.
Tbh I don't see a lot of other ways to achieve it:

  • Like nixpkgs' lib.modules.doRename: include the module as normal, and another module that moves around the options' location. I think this composes badly (it doesn't allow nested namespaces easily), and I wasn't sure how to relocate everything (including config, meta, freeform, etc.).
  • Loading namespaced includes under an augmented prefix in aux's lib.modules.combine. I see this working but it will behave just as if the user had included the included module's source in its own config, and you can't ever tell what comes from namespaced includes or not once definitions are resolved. It's maybe a weird analogy but it feels more like C-style #includes than third-party library imported under a namespace. I have a hard time seeing how to expand and improve ergonomics from there. It trivially allows nested namespaces though, I think.
  • Inserting namespaced includes as submodules with a module like it's done in this first draft. It trivially allows nested namespace, it allows controlling namespaced includes like any other submodule, and the namespace information is not lost after resolving definitions. I also see a lot better how to improve ergonomics (e.g. warn about dupplicate namespaces declared in unrelated places, instead of saying an option is declared twice).

I don't have strong arguments, but I do feel like the third approach is the better one.

I'm not sure what you're referring to as "this", if it's something specific I'm not seeing it (maybe I'm confused by the UI). If it's in general, what's the rationale for modules to be included under a namespace as a submodule, I think the main point is that it gives more control e.g. visibility in documentation. Tbh I don't see a lot of other ways to achieve it: - Like nixpkgs' `lib.modules.doRename`: include the module as normal, and another module that moves around the options' location. I think this composes badly (it doesn't allow nested namespaces easily), and I wasn't sure how to relocate *everything* (including `config`, `meta`, `freeform`, etc.). - Loading namespaced includes under an augmented prefix in aux's `lib.modules.combine`. I see this working but it will behave just as if the user had included the included module's source in its own config, and you can't ever tell what comes from namespaced includes or not once definitions are resolved. It's maybe a weird analogy but it feels more like C-style `#include`s than third-party library imported under a namespace. I have a hard time seeing how to expand and improve ergonomics from there. It trivially allows nested namespaces though, I think. - Inserting namespaced includes as submodules with a module like it's done in this first draft. It trivially allows nested namespace, it allows controlling namespaced includes like any other submodule, and the namespace information is not lost after resolving definitions. I also see a lot better how to improve ergonomics (e.g. warn about dupplicate namespaces declared in unrelated places, instead of saying an option is declared twice). I don't have strong arguments, but I do feel like the third approach is the better one.
Owner

Oh weird, I thought I made this comment around like 216 where lib.types.submodules.of was used. Sorry for the confusion.

I think that all makes sense, thanks for the informative answer! One concern I would have is around the submodule having access to config and options outside of itself. This solution seems rather self-contained since it would have its own isolated config and any options set that aren't specified in this module itself will fail type checks and won't be applied to the rest of the evaluated module set. I think a large piece of what makes modules useful is their ability to compose. For example, the mastodon module being able to configure the web server module, database module, etc in order to bring up a new service. I think this composability should be retained in the event of namespacing.

Oh weird, I thought I made this comment around like 216 where `lib.types.submodules.of` was used. Sorry for the confusion. I think that all makes sense, thanks for the informative answer! One concern I would have is around the submodule having access to config and options outside of itself. This solution seems rather self-contained since it would have its own isolated `config` and any options set that aren't specified in this module itself will fail type checks and won't be applied to the rest of the evaluated module set. I think a large piece of what makes modules useful is their ability to compose. For example, the `mastodon` module being able to configure the web server module, database module, etc in order to bring up a new service. I think this composability should be retained in the event of namespacing.
Author
First-time contributor

I think composition is still partially unresolved, but given this was just the first draft I came up with I expect it to have some rough edges, particularly in that domain.

The way it work for now is that if modules are included without namespace, they're put under the same namespace as the including module (starting at no namespace for modules given to lib.modules.run). If a module is included under a namespace, anything it itself includes will be put under that namespace too (be it in the same or a nested subnamespace).

This means that one can always compose modules even if they declare conflicting options, but the user may have to pass down definitions:

# mastodon.nix
{ pkgs, lib, config, inputs }:
{
  includes = [
    # A modules giving options to configure web servers
    { namespace = "webservers"; module = inputs.webserver.nixosModules.default; }
  ];
  options = {
    enable = lib.options.create { type = lib.types.bool; default.value = false; };
  };
  # Declared by the webserver module
  config.webserver.servers.mastodon.settings = lib.modules.when config.enable {
    port = 80;
    # ...
  };
}
#---
# configuration.nix
{ config, inputs }:
{
  includes = [
    { namespace = "mastodon"; module = ./mastodon.nix; }
    { namespace = "webservers"; module = inputs.webserver.nixosModules.default; }
    { namespace = "nixos"; module = inputs.nixpkgs.nixosModules.default; }
  ];
  config = {
    mastodon.enable = true;
    # Imagine we want to tweak some global webserver config (or declare more web servers)
    # then we need to include webserver ourselve and tweak that...
    webserver.reverseProxy.caddy.enable = true;
    webserver.reverseProxy.nginx.enable = false;
    # ...or tweak mastodon's config, but this implies revealing internal details
    # mastodon.webserver.reverseProxy.caddy.enable = true;
    # mastodon.webserver.reverseProxy.nginx.enable = false;
    # And this all falls down if we namespaced several includes importing the same
    # module that we want to also use ourselve.
    # In the first and third cases, we need to pass config down:
    webserver.servers = config.mastodon.webserver.servers;
    # If the webserver module declared its own systemd services config, it would need
    # to include nixpkgs (or whatever one uses to configure systemd), and we'd need to
    # pass it down to "our" nixpkgs (or whatever).
    systemd.services = webservers.systemd.services;
  };
}

I don't see yet a way to avoid boilerplate to pass down config from modules included under a namespace. Maybe it's a bad idea to let users namespace things without any way for the included module to tell, or have access to the "global" config/options.
Maybe it's not an actual issue and anything that would need to be namespaced would be self-contained in the first place.
I'm thinking we could allow specifying a way to automatically pass down config, but I'm not sure how this would work, and I imagine it would have issues:

{ includes = [
  # Pass down everything untouched
  { namespace = "mastodon"; module = ./mastodon.nix; passDownConfig = true; }
  # Pass down only systemd config ?
  { namespace = "myblog"; module = ./myblog.nix; passDownConfig = { config }: { config.systemd = config.systemd; }; }
]; }

When loading under a prefix though, composition just cannot happen smoothly:

{ inputs }:
{
  includes = [
    { namespace = "nixos"; module = inputs.nixpkgs.nixosModules.default; }
    { namespace = "mailserv"; module = inputs.simple-nixos-mailserver.nixosModules.default; }
  ];
  config = {
    mailserv.mailserver = {
      enable = true;
      fqdn = "mail.example.com";
      domains = [ "example.com" ];
      certificateScheme = "acme-nginx";
    };
    # Oops ! options `mailserv.systemd.services` or whatever not declared !
  };
  # And we can't pass config down: configs are not isolated, so passing config down
  # doesn't solve it, we need to *declare* each one of them:
  options.mailserv.systemd.services = /* .... */ builtins.error "todo";
}

PS: I intended to implement your other comments today but that will wait until tomorrow.

I think composition is still partially unresolved, but given this was just the first draft I came up with I expect it to have some rough edges, particularly in that domain. The way it work for now is that if modules are included without namespace, they're put under the same namespace as the includ**ing** module (starting at no namespace for modules given to `lib.modules.run`). If a module is included under a namespace, anything it itself includes will be put under that namespace too (be it in the same or a nested subnamespace). This means that one can *always* compose modules even if they declare conflicting options, but the user may have to pass down definitions: ```nix # mastodon.nix { pkgs, lib, config, inputs }: { includes = [ # A modules giving options to configure web servers { namespace = "webservers"; module = inputs.webserver.nixosModules.default; } ]; options = { enable = lib.options.create { type = lib.types.bool; default.value = false; }; }; # Declared by the webserver module config.webserver.servers.mastodon.settings = lib.modules.when config.enable { port = 80; # ... }; } #--- # configuration.nix { config, inputs }: { includes = [ { namespace = "mastodon"; module = ./mastodon.nix; } { namespace = "webservers"; module = inputs.webserver.nixosModules.default; } { namespace = "nixos"; module = inputs.nixpkgs.nixosModules.default; } ]; config = { mastodon.enable = true; # Imagine we want to tweak some global webserver config (or declare more web servers) # then we need to include webserver ourselve and tweak that... webserver.reverseProxy.caddy.enable = true; webserver.reverseProxy.nginx.enable = false; # ...or tweak mastodon's config, but this implies revealing internal details # mastodon.webserver.reverseProxy.caddy.enable = true; # mastodon.webserver.reverseProxy.nginx.enable = false; # And this all falls down if we namespaced several includes importing the same # module that we want to also use ourselve. # In the first and third cases, we need to pass config down: webserver.servers = config.mastodon.webserver.servers; # If the webserver module declared its own systemd services config, it would need # to include nixpkgs (or whatever one uses to configure systemd), and we'd need to # pass it down to "our" nixpkgs (or whatever). systemd.services = webservers.systemd.services; }; } ``` I don't see yet a way to avoid boilerplate to pass down config from modules included under a namespace. Maybe it's a bad idea to let users namespace things without any way for the included module to tell, or have access to the "global" config/options. Maybe it's not an actual issue and anything that would need to be namespaced would be self-contained in the first place. I'm thinking we could allow specifying a way to automatically pass down config, but I'm not sure how this would work, and I imagine it would have issues: ```nix { includes = [ # Pass down everything untouched { namespace = "mastodon"; module = ./mastodon.nix; passDownConfig = true; } # Pass down only systemd config ? { namespace = "myblog"; module = ./myblog.nix; passDownConfig = { config }: { config.systemd = config.systemd; }; } ]; } ``` When loading under a prefix though, composition just cannot happen smoothly: ```nix { inputs }: { includes = [ { namespace = "nixos"; module = inputs.nixpkgs.nixosModules.default; } { namespace = "mailserv"; module = inputs.simple-nixos-mailserver.nixosModules.default; } ]; config = { mailserv.mailserver = { enable = true; fqdn = "mail.example.com"; domains = [ "example.com" ]; certificateScheme = "acme-nginx"; }; # Oops ! options `mailserv.systemd.services` or whatever not declared ! }; # And we can't pass config down: configs are not isolated, so passing config down # doesn't solve it, we need to *declare* each one of them: options.mailserv.systemd.services = /* .... */ builtins.error "todo"; } ``` PS: I intended to implement your other comments today but that will wait until tomorrow.
austreelis force-pushed austreelis/feat/namespaced-includes from 211518bb61 to 8288e21555 2024-06-22 18:55:31 +00:00 Compare
Author
First-time contributor

I just pushed commits that should resolve previous discussions, but I have some questions/notes:

  1. I have made include objects require a module key, so that attrs modules included inline can't be confused with include objects, by making modules and includes impossible to validate as each other. I think this is the right approach, maybe the error is not great though for now (there's also a test ensuring any key combinations can't be both a valid include object and a valid module).
  2. For the test I just mentioned, I wrote a small functions that could go in lib.lists.combinations.unrepeated or something like that. Should I create open a PR for that ?
  3. I have added lib.modules.VALID_INCLUDE_KEYS and lib.modules.validate.include, checking a single include object is valid. Is this okay to add this API to the lib ? I haven't put much thoughts into this so it's probably not ideal the way I've done it. More generally, should includes get their own functions exposed ? e.g. put normalizeIncludes under lib.modules.normalize.includes, that would imply breaking the module's normalize by moving it under lib.modules.normalize.module. I could see it under lib.modules.includes.normalize in lib/src/modules/includes.nix too. I also worry namespacing logic or other include-related business might bloat lib/src/modules/default.nix and make it unreadable (that was one of my grudges with nixpkgs' lib/modules.nix, everything is so deeply nested and scoped that it's hard to follow for me). Anyway I started pondering all this with a slightly fried brain so I don't have any answers.
  4. My last commit moves creating the submodule for namespacing into load (in collect, in lib.modules.run). This makes normalization output cleaner and free of big option-tyed attrs, and I guess it makes more sense to actually namespace things when loading rather than normalizing (which should probably just desugar things).

PS: because I rebased on main after the reformatting hammer hit, it seems review comments don't point at the right lines at all anymore :x

I just pushed commits that should resolve previous discussions, but I have some questions/notes: 1. I have made include objects require a `module` key, so that attrs modules included inline can't be confused with include objects, by making modules and includes impossible to validate as each other. I think this is the right approach, maybe the error is not great though for now (there's also a test ensuring any key combinations can't be both a valid include object and a valid module). 2. For the test I just mentioned, I wrote a small functions that could go in `lib.lists.combinations.unrepeated` or something like that. Should I create open a PR for that ? 3. I have added `lib.modules.VALID_INCLUDE_KEYS` and `lib.modules.validate.include`, checking a single include object is valid. Is this okay to add this API to the lib ? I haven't put much thoughts into this so it's probably not ideal the way I've done it. More generally, should includes get their own functions exposed ? e.g. put `normalizeIncludes` under `lib.modules.normalize.includes`, that would imply breaking the module's normalize by moving it under `lib.modules.normalize.module`. I could see it under `lib.modules.includes.normalize` in `lib/src/modules/includes.nix` too. I also worry namespacing logic or other include-related business might bloat `lib/src/modules/default.nix` and make it unreadable (that was one of my grudges with nixpkgs' `lib/modules.nix`, everything is so deeply nested and scoped that it's hard to follow for me). Anyway I started pondering all this with a slightly fried brain so I don't have any answers. 4. My last commit moves creating the submodule for namespacing into `load` (in collect, in `lib.modules.run`). This makes normalization output cleaner and free of big option-tyed attrs, and I guess it makes more sense to actually namespace things when loading rather than normalizing (which should probably just desugar things). PS: because I rebased on main after the reformatting hammer hit, it seems review comments don't point at the right lines at all anymore :x
Author
First-time contributor

After talking with @jakehamilton, we decided not to go forward with this PR, because:

  • We don't see it being extremely useful for now. It's pretty rare to include so many modules that their option declarations clash in a way requiring this feature.
  • It adds complexity in the module system, wish may not be worth it if it's not very useful.
  • There's still unresolved questions, like how to make includes compose well together regardless of the namespace under which they're put. Solving them will likely require more complexity in the module system.
  • The problem it was supposed to solve (modules inadvertently declaring options with clashing names) can be resolved by module writers simply namespacing their option under an appropriate name, which doesn't require any changes. This is today's situation and we don't see a real need to improve it.
After talking with @jakehamilton, we decided not to go forward with this PR, because: - We don't see it being extremely useful for now. It's pretty rare to include so many modules that their option declarations clash in a way requiring this feature. - It adds complexity in the module system, wish may not be worth it if it's not very useful. - There's still unresolved questions, like how to make includes compose well together regardless of the namespace under which they're put. Solving them will likely require more complexity in the module system. - The problem it was supposed to solve (modules inadvertently declaring options with clashing names) can be resolved by module writers simply namespacing their option under an appropriate name, which doesn't require any changes. This is today's situation and we don't see a real need to improve it.
austreelis closed this pull request 2024-06-28 19:23:07 +00:00

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: auxolotl/labs#3
No description provided.