lib/*-plugins: initial support for lib.mkPackageOption

[MattSturgeon]

Summary

Adds initial support for receiving a package option instead of a package derivation in mkVimPlugin and mkNeovimPlugin. This is done to enable the use of lib.mkPackageOption as discussed in #1950.

This soft-deprecates our current helpers, lib.nixvim.mkPluginPackageOption and lib.nixvim.mkPackageOption.

Also implemented this for the plugin TEMPLATE, cmp sources, telescope extensions, and lsp language-servers.

Why

An option with a package default, will be rendered using lib.generators.toPretty as (e.g.) <derivation vimplugin-cmp-ai-2024-08-01>.

We could specify a defaultText for all our package options, e.g. defaultText = lib.literalExpression "pkgs.cmp-ai", however lib.mkPackageOption automates this by setting both default and defaultText based on an attr-path.

Screenshots

Before+after screenshots

Before

After

Alternative/future approach

It would be possible to have mk*Plugin handle calling lib.mkPackageOption internally, however it'd have to do so within an imported module in order to gain access to the (correct) pkgs arg¹.

We may need to do this anyway to also gain access to options and config without having to pass them into the helpers, but that is out of scope for this PR...

See this POC:

Proof of concept

mkNeovimPlugin =
  {
    name,
    defaultPackage,
    # omitted for clarity
  }:
  {
    imports = [
      # Import a module, so we have access to the "correct" pkgs
      ({ config, options, pkgs, ... }: {
        options.plugins.${name} = {
          enable = lib.mkEnableOption name;
          package =
            if lib.isOption defaultPackage then
              defaultPackage
            else
              lib.mkPackageOption pkgs name {
                default = defaultPackage;
              };
        };
      })
    ];
    # Rest omitted for clarity
  }

Footnotes

1. As part of helpers, we do have access to a pkgs, but it may not be the correct one configured for use within the module eval. Incidentally, this is making me think we should separate out all helper functions that rely on pkgs and put them somewhere else so that helpers doesn't need a pkgs. ↩

[traxys]

It would have been nice to remove completly the defaultPackage in this PR, it would simplify a bit the helpers, though I think I saw there are around 150 occurences, it may be some work...

Maybe a regex could replace them all? Not sure

[MattSturgeon]

It would have been nice to remove completely the defaultPackage in this PR, though I think I saw there are around 150 occurences, it may be some work...

I agree it'd be nice, however I don't want to rush to replace all defaultPackages with lib.mkPackageOption if we're still considering the imported-module approach.

A treewide substitution of defaultPackage = pkgs.foo -> defaultPackage = "foo" would be much easier as well...

[traxys]

Indeed, it would be nice to leave the defaultPackage argument when no customization is required

[khaneliman]

So this gives us clearer documentation and nullable packages for plugins, which are big wins to resolve #1970 and #1950. Does it also have the benefit of the lazy package evaluation to prevent pkg errors being thrown from unused options or is that still an issue that requires a a different solution?

[traxys]

I think because we include the man page by default it will never be lazy, whatever we do. We may need to reconsider the man page inclusion by default

[MattSturgeon]

I have some gripes with how we build the man docs too; they will always use our flake's pkgs rather than the pkgs used in module eval.

To achieve laziness, I think we need to either ditch the man pages being included by default, and/or remove automatically getting the meta.homepage from the default package.

[khaneliman]

Did you wanna add a test to verify how it responds to null packages with the new functionality?

[MattSturgeon]

We already have some null packages in lsp/language-servers, and upstream document how it should behave when default = null, so I don't see it as a major issue. If you have a suggestion for a test case I'm happy to consider adding something though.

[khaneliman]

Sorry, was in the mindset of top level package option. These were already nullable.

[MattSturgeon]

So this gives us clearer documentation and nullable packages for plugins

@khaneliman just to clarify, this does not introduce nullable package options, unless a maintainer chooses to explicitly pass nullable = true to mkPackageOption, like I did in mkLsp.

[MattSturgeon]

@traxys this now migrates all call-sites for mkVimPlugin and mkNeovimPlugin to use the new pattern, so I've removed the old defaultPackage argument from those functions.

I've also verified that the docs still link to the correct option-declaration file (i.e. the file that used mk*Plugin) even when the options are declared in an imported module.

I don't want to remove helpers.mkPackageOption and helpers.mkPluginPackageOption in this PR, because the treewide parts are already getting large, and will be difficult to review if we also have to check various ad-hoc package options are migrated correctly (e.g. options like plugins.*.gitPackage).

After merging this PR, there's still 89 matches for mkPluginPackageOption and 60 matches for mkPackageOption remaining. Although some of those are for lib.mkPackageOption (there's 40 matches for /helpers\.mkPackageOption/).

[khaneliman]

LGTM, I very much appreciate the direction of this PR.

[khaneliman]

Did we want to handle this in this PR or follow up?

[MattSturgeon]

I think we'll leave it for later. Next time a user reports a mismatched nixpkgs error we can use the error message they report in the example here.

[MattSturgeon]

@Mergifyio queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 9c11b54