xplr: fix cfg.plugins implementation for more than one plugin

[ChanceHarrison]

Description

Would close #4520. Please see that issue (and the upstream discussion linked within) for more information.

Change the implementation of the programs.xplr.plugins option to correctly set the package.path in the written Lua config file.

Specifically, each component path of the package.path must have appropriate wildcard/template characters (what Lua calls "interrogation marks" or ? characters).

Furthermore, due to the need (in the current implementation) to use programs.xplr.extraConfig to call require(<plugin-name>).setup, this implementation ensures that plugins in the Nix store are inside a predictably-named directory.

Checklist

• Change is backwards compatible.

Unfortunately not. The cfg.plugins option currently takes a list for while it is proposed that it takes an attribute set.

While a change could be made that maintains complete backwards compatibility, one could argue that the usability improvements that accompany the change (of an already broken option) outweigh the cost of breaking back-compat.

• Code formatted with ./format nixfmt.

• Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

• Test cases updated/added. See example.

• Commit messages are formatted appropriately

Maintainer CC

@NotAShelf

[sayanarijit]

Minor suggestions. Other than that, pr looks good. cc @NotAShelf for further review.

[NotAShelf]

Minor suggestions. Other than that, pr looks good. cc @NotAShelf for further review.

I'm in favor of approving this PR as is (with our without the changes you've mentioned) but I really would like some second opinion on the plugin wrapper. I'll try communicating on matrix about this.

[eclairevoyant]

This whole wrapping thing seems strange, what happens if you wrap a plugin in a store path that doesn't have init.lua? I assume it won't match because it'd have to be ${p}/?/?.lua not just ${p}/?.lua

Alternatively, we might consider only allowing plugins with init.lua (and we can get rid of all this wrapping and wildcard fluff).

[sayanarijit]

init.lua is a must for every plugin as documented in https://xplr.dev/en/writing-plugins. There's no plugins without init.lua at root dir.

[eclairevoyant]

Actually wouldn't ${p}/?.lua be sufficient (without this wrapper)? Maybe I missed some discussion, but why does the init.lua case need to be handled separately?

[sayanarijit]

With only ${p}/?.lua, we'd need to import the plugin as require('name.init') rather than require('name'), assuming the entrypoint is name/init.lua. It's a lua convention to add init.lua in package path to make the lib easy to import.

Also, without the wrapper, there's no way to import the plugin by name (since init.lua will be inside hash, not hash/name), unless the plugin structure changes, breaking the entire ecosystem.
Trying to import plugins by hash instead will not work, since plugins also do importing.

[eclairevoyant]

Then I think my original comment applies?

what happens if you wrap a plugin in a store path that doesn't have init.lua? I assume it won't match because it'd have to be ${p}/?/?.lua not just ${p}/?.lua

[sayanarijit]

The wrapping does this:
/nix/store/hash/init.lua ->
/nix/store/hash/name/init.lua

So we can add package.path = "/nix/store/hash/?/init.lua;/nix/store/hash/?.lua" and require("name").

To understand the issue better, let's reproduce it.

1. Create /fakenixstore.
2. Git clone sayanarijit/trash-cli.xplr as /fakenixstore/hashsha123 such that sayanarijit/trash-cli.xplr/init.lua is now /fakenixstore/hashsha123/init.lua.
3. Get into lua repl (in any dir) and modify package.path in a way so that you can run require("trash-cli").setup().

Here you'll see that the only probably the best way to make it work is to:

1. move or symlink /fakenixstore/hashsha123/* as /fakenixstore/hashsha123/trash-cli/*.
2. In lua repl, run package.path = "/fakenixstore/hashsha123/?/init.lua;/fakenixstorestore/hashsha123/?.lua"

[ChanceHarrison]

@eclairevoyant Based on the most recent message, is it clear why the wrapping method is being used? Do you see any better way? If I or someone can provide more clarification, just say the word. Otherwise, feel free to resolve the conversation.

[ChanceHarrison]

@NotAShelf I'm in favor of approving this PR as is (with our without the changes you've mentioned) but I really would like some second opinion on the plugin wrapper. I'll try communicating on matrix about this.

Thanks for the review. Did anything come out of your communications on Matrix regarding the plugin wrapping?

---

To all: Aside from the unresolved conversation regarding the plugin wrapping, I am moving this PR out of draft status based on the feedback received. If there is anything else you would like addressed, just let me know.

[NotAShelf]

I haven't received any responses on matrix but rycee has already given a review - I also asked a few other people to provide their own.

I'll go ahead and approve this.

[sayanarijit]

Hi @rycee I think this is good to go, if no-one has any concern.

[rycee]

Thanks! Merged to master now. If there is any residual concerns about the wrapper, perhaps those could be resolved in a future PR.