lib/modules: replace nixvim builders #3964
Conversation
Introduce a thin wrapper around `<configuration>.config.build.package`, intended to replace the legacy `makeNixvim` and `makeNixvimWithModule` functions.
Introduce a thin wrapper around `<configuration>.config.build.test`, intended to replace the legacy `mkTestDerivationFromNvim` and `mkTestDerivationFromNixvimModule` functions.
Similar to the legacy "standalone wrapper" `makeNixvimWithModule`, thread the configuration's `config` and `options` to the final package. This allows using such a package as the input to functions like `testNixvim`.
GaetanLepage
left a comment
GaetanLepage
left a comment
There was a problem hiding this comment.
LGTM. Looks more straightforward.
khaneliman
left a comment
khaneliman
left a comment
There was a problem hiding this comment.
LGTM overall just couple thoughts on living doc stuff
| mkConfigAt = "It has been moved to `lib.plugins.utils`"; | ||
| }; | ||
|
|
||
| wrapEvalNixvim = |
There was a problem hiding this comment.
Could do with a docstring as well explaining the purpose and what it does for us smooth brains.
Seems like this is the part that can handle a raw configuration or an already evaluated one?
There was a problem hiding this comment.
I got lazy because this is private/internal and was extracted from functions that already had doc-comments. But you're right; this has enough logic and complexity to warrant some explanation.
There was a problem hiding this comment.
You're fine, just figured I'd try to suggest something to aid anyone working with it who might not be as familiar with the nixpkgs lib functions involved to make it easier to understand what's happening here.
| /** | ||
| Build a Nixvim package. | ||
| This is a thin wrapper around `<nixvim-configuration>.config.build.package`. |
There was a problem hiding this comment.
This line kinda confused me in the context of the rest of the docstring. We are building a package but it says its a thin wrapper around the output of a built package? Or am I reading it wrong?
There was a problem hiding this comment.
This is definitely challenging to phrase clearly.
It is saying that it returns the build.package option from a nixvim configuration.
In module system terminology, a "configuration" is the canonical name for the result of evaluating modules using evalModules. I.e. it has _type = "configuration";.
Technically, buildNixvim also accepts anything that has a config attr, including packages with passthru.config. But I don't want to distract from the key point.
I'm guessing there's simply not enough context that we're talking about the module system?
There was a problem hiding this comment.
Ah I see. so ...around the ... option ?
There was a problem hiding this comment.
Maybe, but "option" alone doesn't do it justice, since it extracts the option value from a configuration (or any duck that looks like a configuration).
Perhaps something like "this function returns the build.package option's value, as defined in the supplied nixvim configuration"?
There was a problem hiding this comment.
Yeah, that sounds good to me.
lib/modules.nix
Outdated
| 3. A Nixvim package, as produced by `buildNixvim`. | ||
| # Output | ||
| An installable Nixvim package. | ||
| */ | ||
| buildNixvim = lib.mirrorFunctionArgs self.modules.evalNixvim ( |
There was a problem hiding this comment.
Is the buildNixvim mentionned in the docstring the same as this buildNixvim?
Otherwise, wouldn't you feed buildNixvim with a Nixvim package, as produced by buildNixvim?
lib/modules.nix
Outdated
| testNixvim = lib.mirrorFunctionArgs self.modules.evalNixvim ( | ||
| input: | ||
| let | ||
| configuration = if input ? config.build.package then input else self.modules.evalNixvim input; |
There was a problem hiding this comment.
Maybe overkilled, but we could factorize this line in a separate function. Maybe that won't help with understanding though.
3 participants
This PR adds two new builder functions,
buildNixvimandtestNixvim, which provide a simpler interface for building and testing Nixvim configurations. This is the first step toward deprecating the older builders:makeNixvimmakeNixvimWithModulemkTestDerivationFromNvimmkTestDerivationFromNixvimModuleBoth functions accept any of the following:
evalNixvimconfigpassthruA new helper,
wrapEvalNixvim, normalizes these inputs so callers do not need to distinguish between forms.Built packages now expose
configandoptionsin passthru, which allows them to be reused as input totestNixvim. Tests ensure that the new builders mirror the argument structure ofevalNixvim.Doccomments are provided, but prominent user documentation is deferred until the new interfaces have time to settle.
Top-level lib or flake aliases are also not added; for now these functions are only available under the
lib.nixvim.modulesnamespace.The
extendfunction (added by the legacymakeNixvimWithModulestandalone wrapper) is also not implemented for packages built without the legacy builder. Currently I'm unsure if we should replace that interface, or if we should suggest users use alternative methods likeextendModulesand/or importing common modules into different configurations.