Skip to content

neovim: add 'autowrapRuntimeDeps' to append #358587

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

Merged
teto merged 1 commit into from
Dec 16, 2024
Merged

neovim: add 'autowrapRuntimeDeps' to append #358587

Show file tree
Hide file tree
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
24 changes: 21 additions & 3 deletions pkgs/applications/editors/neovim/tests/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,24 @@ in
'';

nvim_with_autoconfigure = pkgs.neovim.overrideAttrs(oa: {
plugins = [ vimPlugins.unicode-vim ];
plugins = [
vimPlugins.unicode-vim
vimPlugins.fzf-hoogle-vim
];
autoconfigure = true;
# legacy wrapper sets it to false
wrapRc = true;
});

nvim_with_runtimeDeps = pkgs.neovim.overrideAttrs({
plugins = [
pkgs.vimPlugins.hex-nvim
];
autowrapRuntimeDeps = true;
# legacy wrapper sets it to false
wrapRc = true;
});

nvim_with_ftplugin = let
# this plugin checks that it's ftplugin/vim.tex is loaded before $VIMRUNTIME/ftplugin/vim.tex
# $VIMRUNTIME/ftplugin/vim.tex sources $VIMRUNTIME/ftplugin/initex.vim which sets b:did_ftplugin
Expand Down Expand Up @@ -334,19 +346,25 @@ in
${nvim_with_opt_plugin}/bin/nvim -i NONE +quit! -e
'';

inherit nvim-with-luasnip;

autoconfigure = runTest nvim_with_autoconfigure ''
assertFileContains \
"$luarc" \
'${vimPlugins.unicode-vim.passthru.initLua}'
'';

autowrap_runtime_deps = runTest nvim_with_runtimeDeps ''
assertFileContains \
"${nvim_with_runtimeDeps}/bin/nvim" \
'${pkgs.xxd}/bin'
'';

inherit nvim-with-luasnip;
# check that bringing in one plugin with lua deps makes those deps visible from wrapper
# for instance luasnip has a dependency on jsregexp
can_require_transitive_deps =
runTest nvim-with-luasnip ''
cat ${nvim-with-luasnip}/bin/nvim
cat ${nvim-with-luasnip}/nvim
${nvim-with-luasnip}/bin/nvim -i NONE --cmd "lua require'jsregexp'" -e +quitall!
'';

Expand Down
16 changes: 14 additions & 2 deletions pkgs/applications/editors/neovim/wrapper.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ let
# to work with nix.
# if true, the wrapper automatically appends those snippets when necessary
, autoconfigure ? false

# append to PATH runtime deps of plugins
, autowrapRuntimeDeps ? false
Copy link
Copy Markdown
Member

@PerchunPak PerchunPak Nov 27, 2024

Choose a reason for hiding this comment

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

Why having it opt-in? We already apply a wrapper to each plugin, no?

Copy link
Copy Markdown
Member Author

@teto teto Nov 27, 2024

Choose a reason for hiding this comment

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

I thought we might have it opt-in at first to get feedback,to update the plugins with their runtimeDeps as well. And in a second step we would make it opt-out. We coul opt-in straightaway as well, I dont mind.

Copy link
Copy Markdown
Member Author

@teto teto Nov 27, 2024

Choose a reason for hiding this comment

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

one issue with the opt-out is that it forces us to update the doc in a way that works with the legacy and new wrapper, which should be done but I am too lazy to do yet :'( I would rather document the new wrapper in another PR

Copy link
Copy Markdown
Member

@PerchunPak PerchunPak Nov 27, 2024

Choose a reason for hiding this comment

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

Could you remind me please where is documentation for the wrapper?

Copy link
Copy Markdown
Member

@PerchunPak PerchunPak Nov 27, 2024

Choose a reason for hiding this comment

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

I don't see a reason for changing plugins to use runtimeDeps in a subsequent PR. We can do it here to tackle problems before merging the feature. Unless you don't want to do this right now

But yeah, we can keep it opt-in for now, though I doubt someone would use opt-in undocumented features outside of [@]NixOS/neovim team

Copy link
Copy Markdown
Member Author

@teto teto Nov 27, 2024

Choose a reason for hiding this comment

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

if we stop patching the plugins because of this PR, then disabling autowrapRuntimeDeps would entail user regressions so you are right we might need to enable it by default straightaway. @mrcjkb would opt-out by default be okay with you ?
You might see regressions for some plugins where paths would stop being hardcoded. Mixing an apt neovim with a nix plugin would stop working for instance (contrary to today). I beliee it's fine to not support these scenarios since not patching plugins makes maintaining plugins easier, we better respect upstream and it means plugins work on neovim as on other distributions: you can override the dependencies without having to regenerate the whole system


# should contain all args but the binary. Can be either a string or list
, wrapperArgs ? []
, withPython2 ? false
Expand Down Expand Up @@ -120,7 +124,15 @@ let
wrapperArgsStr = if lib.isString wrapperArgs then wrapperArgs else lib.escapeShellArgs wrapperArgs;

generatedWrapperArgs = let
binPath = lib.makeBinPath (lib.optional finalAttrs.withRuby rubyEnv ++ lib.optional finalAttrs.withNodeJs nodejs);
op = acc: normalizedPlugin: acc ++ normalizedPlugin.plugin.runtimeDeps or [];

runtimeDeps = lib.foldl' op [] pluginsNormalized;

binPath = lib.makeBinPath (
lib.optional finalAttrs.withRuby rubyEnv
++ lib.optional finalAttrs.withNodeJs nodejs
++ lib.optionals finalAttrs.autowrapRuntimeDeps runtimeDeps
);
in

# vim accepts a limited number of commands so we join them all
Expand Down Expand Up @@ -167,7 +179,7 @@ let
__structuredAttrs = true;
dontUnpack = true;
inherit viAlias vimAlias withNodeJs withPython3 withPerl withRuby;
inherit autoconfigure wrapRc providerLuaRc packpathDirs;
inherit autoconfigure autowrapRuntimeDeps wrapRc providerLuaRc packpathDirs;
inherit python3Env rubyEnv;
inherit wrapperArgs generatedWrapperArgs;
luaRcContent = rcContent;
Expand Down
4 changes: 1 addition & 3 deletions pkgs/applications/editors/vim/plugins/overrides.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1083,9 +1083,7 @@ in
};

hex-nvim = super.hex-nvim.overrideAttrs {
postPatch = ''
substituteInPlace lua/hex.lua --replace xxd ${xxd}/bin/xxd
'';
runtimeDeps = [ xxd ];
nvimRequireCheck = "hex";
};

Expand Down