Conversation
|
In case you want some feedback on the usability, I'm willing to give this a try on the weekend for my two usecases: integrating zsh into nixos and integrating direnv into zsh (these would both be solvable by this, right?) |
|
yeah I actually have the zsh one in this PR as an example! Do let me know what you think, I am not entirely set on the design and I am still deciding exactly how it should be, but hopefully this illustrates at least what I was talking about in that discussion. |
|
I also broke it apparently |
eb06466 to
339f056
Compare
|
OK I thought I broke it last night, turns out I was just importing it twice. I still have to test out some things though to see if this is viable. I am concerned about if importing the module in a home manager config, and then grabbing that module out of that home manager config and using the install module from that in another config, if that will cause any options added to the wrapper module by the install module to be declared multiple times or not. So, there is more testing to do and stuff of niche scenarios like that. Do let me know how it goes. I have in my config right now a branch with this new version where I am messing with it BirdeeHub/birdeeSystems@main...installmods Edit: It does appear to be an issue to import it after importing it........ |
58f96d4 to
58ee44d
Compare
|
https://github.com/NixOS/nixpkgs/blob/306d3438ad768b4056c4f1a21a53742695016649/lib/types.nix#L1245 ^ this module does not have key set in it https://github.com/NixOS/nixpkgs/blob/306d3438ad768b4056c4f1a21a53742695016649/lib/types.nix#L1302 ^ and neither does this one. This is actually is somewhat of a problem for the case when you try to grab the wrapper module out of a home manager or nixos config which imports it and then use the install module out of that and try to use that in another home manager or nixos config. So, I suppose I will be making a nixpkgs PR, because those should probably have key set, the internal module in lib.evalModules has it set, these should have it set too. |
64c385f to
beca1c2
Compare
|
To be honest, I am still not 100% sold on this design, although it is very nice actually in terms of what it allows a wrapper module to specify about how it is installed. It requires lib.evalModules to be called to grab the module for the other system though... it would be nice to see how much of that I am able to confine into the submodule level and leave the top level of the generated module as lazy as possible. That being said, as-is, it is still quite lazy, at least none of these are attrsOf, it doesnt depend on pkgs, and there is no interdependence that would be relevant for that. And also, we are literally trying to define modules for another module system inside our module, so I really am not sure how possible it would be to get around that but I will try? Basically I would need to write a really clever But regardless, that PR to nixpkgs is still something which should be done |
18a4541 to
1b067b2
Compare
|
But the problem that makes it non-trivial to generate this value is this You (might in some scenario related to this, outlined above) pass the same module twice. To the same function twice. How does it know they are the same module, and not different ones? Because if it just adds a unique key, then, it would add a different key to both copies of the same module, and then they would not deduplicate still. It literally does not matter what the value of key is, as long as there is not another module you want to keep imported in that particular wrapper module with that same key, but the ones you do want to de-duplicate have the same key As for it being weird... the situation it solves is weird too... It solves a really niche problem... I am not the happiest about it being there either Its a good part of why this is still a draft I feel like there should be some way around most of these problems, that I have not seen yet. Some slightly different design that solves these problems in a more elegant way. I haven't come up with it yet though. To be honest, I haven't even fully solved them in an inelegant way yet. |
| lib.setAttrByPath config.optionLocation { | ||
| imports = lib.toList module; | ||
| key = builtins.unsafeDiscardStringContext "${wlib.core}:${ | ||
| if lib.isStringLike module then "${module}." else "" | ||
| }install.addWrapperModule:${toString key}"; | ||
| }; |
There was a problem hiding this comment.
So this creates a set:
wrappers.<name> = {
imports = [module];
key = ...
}which gets then assigned to config in the __functor below.
Do imports get resolved also when being set at arbitrary positions in the config tree?
I thought they must be top level?
{
imports = ...
options = ...
config = ...
}There was a problem hiding this comment.
submodules are modules. You can import in them.
subWrapperModules are wrapper modules. Which are just modules with wlib.core imported and wlib passed as a specialArg, and modulesPath specialArg set to the nix-wrapper-modules root dir
wlib.types.subWrapperModule is a lib.types.submoduleWith alias, so you dont even have to pass them the function form of a module to make use of the whole module, because they have shorthandOnlyDefinesConfig defaulting to false unlike lib.types.submodule where it defaults to true
moduleType is also a submoduleWith type.
There was a problem hiding this comment.
hmm ok.. So in the functor you define an option at the same location (wrappers.<name>) of type moduleType:
nix-wrapper-modules/lib/core.nix
Line 317 in 3da8aa9
I guess this is how you indicate that whatever is configured there is a module and hence the module system understands the import = [...] key there. Is this the idea?
moduleType is some kind lowever level detail of the module system? Couldn't find much about it.
There was a problem hiding this comment.
moduleType is some kind lowever level detail of the module system? Couldn't find much about it.
It is a submoduleWith type with the current module evaluation's state in it
https://github.com/NixOS/nixpkgs/blob/d83c818aea5e339a71744e01e15fe54af93aa0da/lib/modules.nix#L250
https://github.com/NixOS/nixpkgs/blob/d83c818aea5e339a71744e01e15fe54af93aa0da/lib/modules.nix#L94
| This is to prevent the option location depending on the `pkgs` of the target evaluation, as it would if we used `config.binName`. | ||
| ''; | ||
| }; | ||
| addWrapperModule = lib.mkOption { |
There was a problem hiding this comment.
Maybe wrapperModule is the wrong term here because wrapperModules refer to the "wrapper modules" this repo provides (i.e. wlib.wrappers.<wrapperModule>). This module is a plain nixos/hm/etc module that integrates the wrapperModule, no?
Hence, I would use something else here.
integrationModulewrapperIntegrationModule
Also, imo the prefix add is not necessary. The option does not "add" the module, it merely provided ist.
It gets "added" in the functor.
There was a problem hiding this comment.
modules.nixos is a plain nixos module
addWrapperModule adds a module to the wrapper module which is currently imported inside a submoduleWith option in said nixos module
(but that module it adds to the wrapper module is not imported inside the wrapper module normally, only if you import the install module of that wrapper module in that particular system does it get imported in that particular submodule option containing the wrapper module)
There was a problem hiding this comment.
I am of course happy to change the name still to something more instructive if you have one, but it is definitely adding a module to the wrapper module which is imported inside that nixos module as a submodule option, so it is adding a module to the wrapper module, or, adding a wrapper module. IDK. addWrapperModule was what I came up with, there is probably better, do let me know
wrapperIntegrationModule is ok. integrationWrapperModule? config.install.putitintherightplaceplease? (joking with that last one obviously) honestly add felt about as good, and was shorter? extraWrapperModule? extendSubWrapperModule? config.install.extendWrapper?
There was a problem hiding this comment.
AI SLOP FUNCTION NAME BRAINSTORM COURTESY OF GIPPITY BEING FED THIS CONVERSATION
extendWrapperModule← probably the cleanestwrapperModuleExtensionwrapperModuleExtenderwrapperModuleAugmentaugmentWrapperModulewrapperModulePatchpatchWrapperModule
If you want to emphasize composition / merging
composeWrapperModulemergeIntoWrapperModulewrapperModuleOverlayoverlayWrapperModule(Nix-y, but slightly overloaded with overlays conceptually)
If you want to emphasize scope (submodule context)
Since this happens inside a submoduleWith, you can lean into that:
extendWrapperSubmodulewrapperSubmoduleExtensionwrapperSubmoduleModule(a bit clunky but precise)injectWrapperSubmodule
If you want to emphasize “hooking into install flow”
Because it only applies when used via the install module:
wrapperInstallExtensioninstallWrapperExtensionwrapperIntegrationExtensionextendWrapperIntegration
Short + pragmatic (closest to what you had)
extendWrapper← good balance of brevity + meaningextraWrapperModulewrapperExtension
What to avoid (based on your own confusion)
addWrapperModule→ implies side-effecting behavior, not declarative provisionintegrationModule→ too generic, loses targetwrapperIntegrationModule→ still ambiguous direction- anything with just
module→ doesn’t distinguish which module is affected
Strong recommendation
If you want something that will still make sense in 6 months:
extendWrapperModule
It encodes:
- target:
WrapperModule - action:
extend(fits Nix mental model: overlays, mkMerge, etc.) - avoids imperative “add”
If you want slightly shorter:
wrapperModuleExtension
Same meaning, more data-oriented naming.
There was a problem hiding this comment.
wrapperSubmoduleModule really seems like the winner here of course 🤣 🤣 🤣 🤣 🤣 🤣
(more seriously, config.install.extendWrapperModule or config.install.extendWrapper)
There was a problem hiding this comment.
(more seriously, config.install.extendWrapperModule or config.install.extendWrapper)
Maybe I am missing something but to me it's unintuitive to have verb in the name because as far as I understand this option merely returns some module/submodule. It doesn't DO anything with it. Hence I would rather name it wrapperModuleExtension/wrapperExtension instead of extendWrapperModule/extendWrapper if you were to go for this one.
If this option was responsible for integrating it in the "consuming" module system (which I think is done in the __functor then extendWrapperModule or even add... would be adequate.
There was a problem hiding this comment.
I see what you are saying actually. config.install.wrapperExtension seems ok to me actually
or maybe mkWrapperExtention because that is common in nixpkgs for like mkIf and stuff where it just returns a value with some specific type that if put into the module system will have some effect. Actually, I think I just talked myself out of mkWrapperExtension? unsure.
There was a problem hiding this comment.
I still need to solve the _module.args.name problem though, and despite this repository being enjoyable to mess with I do have stuff to do, and I have some ideas I think but I havent gotten to trying them out, so there will be some time between now and when this is ready
I need to fix up the mpv module, Im most of the way done with that.
There was a problem hiding this comment.
I see what you are saying actually.
config.install.wrapperExtensionseems ok to me actuallyor maybe
mkWrapperExtentionbecause that is common in nixpkgs for likemkIfand stuff where it just returns a value with some specific type that if put into the module system will have some effect. Actually, I think I just talked myself out of mkWrapperExtension? unsure.
I think using mk... makes sense (no pun intended).
I'm also in favor of having "module" somewhere in the name because it indicates the type of the option (lib.types.raw does not make this obvious for me).
I'm still debating whether Integration is better than Extension but in the end, probably both are fine for me.
Extension is adequate as you are free to extend the wrapperModules in whatever way you want, although the main reason for having this option, is to add "integration capability" to the module. mkIntegrationModule would make this purpose more explicit but mkWrapperExtension (or mkWrapperModuleExtension) is correct too.
There was a problem hiding this comment.
mkWrapperExtension is probably the one then
Unless I find (or am told of) a better design that doesnt have it at all or something
3204e76 to
c96e7d6
Compare
|
So, I think I fixed the _module.args.name problem at least. Interface is otherwise still the same. Still trying to think of a better way, but at least it solves the problem it was trying to solve. |
fc8a513 to
de51f8d
Compare
Also, I somehow just read this from your very first comment correctly. Unless you set That being said... devenv actually accepts modules, right? So, if you were passing your zsh to the devenv module evaluation used by the direnv config, and it sets class, then you could declare an install module for devenv within your zsh module. These install modules would be for anything which is another module system which does set Wrapper modules do not set class because it doesnt make sense, for particularly that reason.
For wrapper modules, each one would need to have its own So, for integrating zsh into your direnv wrapper module, you would have to make a |
|
Thanks for clearing things up. My feedback, after I tested it, was solely for the nixos/hm integration |
Work in progress, but, the start of addressing: #370
|
So, I am not really thinking of anything actually better for The best I have is: Instead of we have just a set of extra wrapper modules. But then... is that, like, do the names have to match up with install.modules then? Do we have to have it be like, a set of sets maybe so that they can be grouped with their correct install module and then also have a name? And then they can't access the config from that other module system in it... So, that doesn't really feel better. To be fair, most of the time, people will just add a new option to their wrapper module rather than messing around with this anyway 🤣 I really wish I didn't have to have name as the first parameter, and have name not really matter whatsoever other than being different. It is somewhat confusing. But this makes it more technically sound and forms some kind of guarantee that this will be handled. Because if it isn't forced, nobody would do it, practically guaranteed. Theres like 20 people in the whole world who know how I could maybe add a second helper that takes care of the That being said, I still like what it enables, and I have been using it and it works nicely, and it allows for something like a dendritic config for that particular program without even using flake-parts for that, which is somewhat satisfying to use. So, like, not fully decided, but it is growing on me more, especially now that I fixed the (admittedly somewhat niche, but still important) duplicate name issue |
Work in progress, but, the start of addressing:
#370