WIP: feat: Namespaced includes #3
No reviewers
Labels
No labels
Compat
Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: auxolotl/labs#3
Loading…
Reference in a new issue
No description provided.
Delete branch "austreelis/feat/namespaced-includes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
__module.args.dynamic
d35d70d513d35d70d513
to211518bb61
feat: Namespaced includesto WIP: feat: Namespaced includes@ -169,0 +177,4 @@
if
builtins.isAttrs include
&& include != { }
&& !builtins.any (n: builtins.elem n lib.modules.VALID_KEYS) (builtins.attrNames include)
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:
At the very least we can switch on whether
include ? namespace
is a thing.@ -169,0 +204,4 @@
in
builtins.foldl' filter [ ] flattened;
doNamespace =
nit: maybe a name like
createNamespacedModule
would be informative here?@ -169,0 +219,4 @@
description = "include ${namespace}";
};
};
# config.${namespace} = lib.modules.override 1000 {};
Is this comment needed?
No, I thought I left it out when cleaning up my patch.
@ -169,0 +222,4 @@
# config.${namespace} = lib.modules.override 1000 {};
};
in
builtins.seq throwOnConflict builtins.map doNamespace flattened;
Can we extract the
builtins.map doNamespace flattened
bit to a variable so that this value is easier to read? Something likebuiltins.seq throwOnConflict namespacedModules
might make it easier to understand.@ -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 [ ];
Can we wrap usage of
or
in parens here? It can be a bit confusing otherwise.What is the rationale for this being a submodule?
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:
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 (includingconfig
,meta
,freeform
, etc.).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.I don't have strong arguments, but I do feel like the third approach is the better one.
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, themastodon
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.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:
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:
When loading under a prefix though, composition just cannot happen smoothly:
PS: I intended to implement your other comments today but that will wait until tomorrow.
211518bb61
to8288e21555
I just pushed commits that should resolve previous discussions, but I have some questions/notes:
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).lib.lists.combinations.unrepeated
or something like that. Should I create open a PR for that ?lib.modules.VALID_INCLUDE_KEYS
andlib.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. putnormalizeIncludes
underlib.modules.normalize.includes
, that would imply breaking the module's normalize by moving it underlib.modules.normalize.module
. I could see it underlib.modules.includes.normalize
inlib/src/modules/includes.nix
too. I also worry namespacing logic or other include-related business might bloatlib/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.load
(in collect, inlib.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
After talking with @jakehamilton, we decided not to go forward with this PR, because:
Pull request closed