Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Jan 4, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 48 additions & 21 deletions modules/programs/xplr.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,47 @@
{ config, lib, pkgs, ... }:
let
inherit (lib)
concatStringsSep types mkIf mkOption mkEnableOption mkPackageOption
literalExpression;
types mkIf mkOption mkEnableOption mkPackageOption literalExpression;

cfg = config.programs.xplr;

initialConfig = ''
version = '${cfg.package.version}'
'';

# We provide a default version line within the configuration file, which is
# obtained from the package's attributes. Merge the initial configFile, a
# mapped list of plugins and then the user defined configuration to obtain the
# final configuration.
pluginPath = if cfg.plugins != [ ] then
(''
package.path=
'' + (concatStringsSep " ..\n"
(map (p: ''"${p}/init.lua;${p}/?.lua;"'') cfg.plugins)) + ''
..
package.path
'')
# If `value` is a Nix store path, create the symlink `/nix/store/newhash/${name}/*`
# to `/nix/store/oldhash/*` and returns `/nix/store/newhash`.
wrapPlugin = name: value:
if lib.isStorePath value then
pkgs.symlinkJoin {
name = name;
paths = [ value ];
postBuild = ''
mkdir '${name}'
mv $out/* '${name}/'
mv '${name}' $out/
'';
}
else
builtins.dirOf value;

makePluginSearchPath = p: "${p}/?/init.lua;${p}/?.lua";
Copy link

@eclairevoyant eclairevoyant Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@sayanarijit sayanarijit Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@eclairevoyant eclairevoyant Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@sayanarijit sayanarijit Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@eclairevoyant eclairevoyant Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@sayanarijit sayanarijit Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


pluginPath = if cfg.plugins != { } then
let
wrappedPlugins = lib.mapAttrsToList wrapPlugin cfg.plugins;
searchPaths = map makePluginSearchPath wrappedPlugins;
pluginSearchPath = lib.concatStringsSep ";" searchPaths;
in (''
package.path = "${pluginSearchPath};" .. package.path
'')
else
"\n";

# We provide a default version line within the configuration file, which is
# obtained from the package's attributes. Merge the initial configFile, a
# mapped list of plugins and then the user defined configuration to obtain
# the final configuration.
configFile = initialConfig + pluginPath + cfg.extraConfig;
in {
meta.maintainers = [ lib.maintainers.NotAShelf ];
Expand All @@ -35,15 +52,25 @@ in {
package = mkPackageOption pkgs "xplr" { };

plugins = mkOption {
type = with types; nullOr (listOf (either package str));
default = [ ];
defaultText = literalExpression "[]";
type = with types; nullOr (attrsOf (either package str));
default = { };
defaultText = literalExpression "{ }";
description = ''
Plugins to be added to your configuration file.
An attribute set of plugin paths to be added to the [package.path]<https://www.lua.org/manual/5.4/manual.html#pdf-package.path> of the {file}`~/config/xplr/init.lua` configuration file.
ChanceHarrison marked this conversation as resolved.
Show resolved Hide resolved

Must be a package, an absolute plugin path, or string to be recognized
by xplr. Paths will be relative to
{file}`$XDG_CONFIG_HOME/xplr/init.lua` unless they are absolute.
Must be a package or string representing the plugin directory's path.
If the path string is not absolute, it will be relative to {file}`$XDG_CONFIG_HOME/xplr/init.lua`.
'';
example = literalExpression ''
{
wl-clipboard = fetchFromGitHub {
owner = "sayanarijit";
repo = "wl-clipboard.xplr";
rev = "a3ffc87460c5c7f560bffea689487ae14b36d9c3";
hash = "sha256-I4rh5Zks9hiXozBiPDuRdHwW5I7ppzEpQNtirY0Lcks=";
}
local-plugin = "/home/user/.config/plugins/local-plugin";
};
'';
};

Expand Down