wrapNeovim: consolidate makeWrapper arguments and more

[doronbehar]

Motivation for this change

After discussion in #87929 I wanted to show how we can get the man page for Neovim installed in user environment, along with pretty much every other file Neovim pushes, and also make sure we don't get nvim double wrapped if configure != {} (argument given to the wrapper).

All of this was done while repetitions were reduced to a minumum, and hopefully it's now easier to maintain this wrapper.

cc @0paIescent for testing. I think most arguments wrapNeovim should know how to handle are taken care of but I tested only part of the possible options.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[doronbehar]

@GrahamcOfBorg build neovim

[infinisil]

In addition to this not really working in its current state, I made these changes to fix some argument and escaping issues:

diff --git a/pkgs/applications/editors/neovim/wrapper.nix b/pkgs/applications/editors/neovim/wrapper.nix
index 67f20732082..fe3220c8b37 100644
--- a/pkgs/applications/editors/neovim/wrapper.nix
+++ b/pkgs/applications/editors/neovim/wrapper.nix
@@ -74,7 +74,7 @@ let
   };
   ## Here we calculate all of the arguments to `makeWrapper`
   # We start with the exectutable itself
-  makeWrapperArgs = [ "${neovim}/bin/nvim" "$out/bin/nvim" "--argv0" "'$0'" ]
+  makeWrapperArgs = [ "${neovim}/bin/nvim" "${placeholder "out"}/bin/nvim" "--argv0" "$0" ]
     # For every "host" program neovim supports, we map according to the attrset
     # `hostprog_check_table` what flags to add to wrapper
     ++ (lib.lists.flatten (lib.attrsets.mapAttrsToList (
@@ -82,28 +82,28 @@ let
       withProg:
       [ "--add-flags"
         # --cmd is needed even if we disable this ${prog} provider or not
-        ("\"--cmd " +
+        (lib.escapeShellArgs [ "--cmd"
           (if withProg then
             # All host programs end up in the same path
-            "'let g:${prog}_host_prog=$out/bin/nvim-${prog}'"
+            "let g:${prog}_host_prog='${placeholder "out"}/bin/nvim-${prog}'"
           else
             # Tells nvim to practically not load this provider
-            "'let g:loaded_${prog}_provider=1'"
+            "let g:loaded_${prog}_provider=1"
           )
-        + "\"")
+        ])
       ]
     ) hostprog_check_table))
     ++ lib.optionals withRuby [
       "--set" "GEM_HOME" "${rubyEnv}/${rubyEnv.ruby.gemPath}"
     ]
     ++ lib.optionals (binPath != "") [
-      "--suffix" "PATH" binPath
+      "--suffix" "PATH" ":" binPath
     ]
     # this relies on a patched neovim, see
     # https://github.com/neovim/neovim/issues/9413
     ++ lib.optionals (configure != {}) [
-      "--set" "NVIM_SYSTEM_RPLUGIN_MANIFEST" "$out/rplugin.vim"
-      "--add-flags" "-u" "${vimUtils.vimrcFile configure}"
+      "--set" "NVIM_SYSTEM_RPLUGIN_MANIFEST" "${placeholder "out"}/rplugin.vim"
+      "--add-flags" "-u ${vimUtils.vimrcFile configure}"
     ]
   ;
 
@@ -114,7 +114,7 @@ let
         # Remove the symlinks created by symlinkJoin which we need to perform
         # extra actions upon
         rm $out/share/applications/nvim.desktop $out/bin/nvim
-        makeWrapper ${builtins.concatStringsSep " " makeWrapperArgs} ${extraMakeWrapperArgs}
+        makeWrapper ${lib.escapeShellArgs makeWrapperArgs} ${extraMakeWrapperArgs}
         substitute ${neovim}/share/applications/nvim.desktop $out/share/applications/nvim.desktop \
           --replace 'TryExec=nvim' "TryExec=$out/bin/nvim" \
           --replace 'Name=Neovim' 'Name=WrappedNeovim'

[infinisil]

Btw I'm testing this by nix-building this file in the nixpkgs tree:

with import ./. {};
neovim.override {
  vimAlias = true;
  configure = {
    packages.myPlugins = with pkgs.vimPlugins; {
      start = [ vim-lastplace vim-nix ];
      opt = [];
    };
    customRC = ''
      " your custom vimrc
      set nocompatible
      set backspace=indent,eol,start
      " ...
    '';
  };
}

I think the cleanups are nice, and I also wouldn't mind seeing the double-wrapping being prevented. But even just the symlinkJoin would be nice to have.

[teto]

is that a personal pref or enforced by a linter ?

[doronbehar]

What do you mean? In anycase, all of my nix syntax habits are meant to optimize future diffs.

[doronbehar]

@infinisil thanks for the review! I've patched the changes that fix the escaping, along with some changes that should prevent double wrapping. I've also added that override to my overlays so I should be able to quickly test it. I tested it briefly by running the executables and I inspected the directory and the files and it seems good to me. Please let me know if you find anything wrong.

[infinisil]

This changes it to call the unwrapped neovim, whereas previously it was wrapped with access to node/python/ruby. Does it matter for this not to be the case anymore for rplugin.vim generation?

[doronbehar]

The double-wrapping can't be removed this easily, because the previous code actually calls nvim to generate rplugin.vim after the first but before the second wrapping.

Indeed it isn't tricky at all. I didn't fully understand at first the functionality the wrapper should offer. I think I've finally managed to do it properly now. Currently rplugin.vim is generated at the end, as in the original design, and I've split the "calculation" of the flags to clean-nvim-flags vs makeWrapperArgs so that I do call ${neovim}/bin/nvim but with the flags and the environment the same as used in the wrapper (besides NVIM_SYSTEM_RPLUGIN_MANIFEST of course which shouldn't be used along with NVIM_RPLUGIN_MANIFEST).

[infinisil]

Looking better now :)

[doronbehar]

I double checked the executable to be working and this is how it's wrapper looks like:

#! /nix/store/58in3hsawf6g1adx99sphi460nzmaqxq-bash-4.4-p23/bin/bash -e
export GEM_HOME='/nix/store/cg1ndvqjfrjfn0mshk44mql6w3b6v84r-neovim-ruby-env/lib/ruby/gems/2.6.0'
export PATH=$PATH${PATH:+':'}'/nix/store/cg1ndvqjfrjfn0mshk44mql6w3b6v84r-neovim-ruby-env/bin'
export NVIM_SYSTEM_RPLUGIN_MANIFEST='/nix/store/mxahv8pbnsqf2ac25ndwk57nh6xdz8gr-neovim-0.4.3/rplugin.vim'
exec -a "$0" "/nix/store/fy3n7a7s2nxyrdlxfyvck9fckcpqgi5c-neovim-unwrapped-0.4.3/bin/nvim"  '--cmd' 'let g:loaded_node_provider=1' '--cmd' 'let g:python_host_prog='\''/nix/store/mxahv8pbnsqf2ac25ndwk57nh6xdz8gr-neovim-0.4.3/bin/nvim-python'\''' '--cmd' 'let g:python3_host_prog='\''/nix/store/mxahv8pbnsqf2ac25ndwk57nh6xdz8gr-neovim-0.4.3/bin/nvim-python3'\''' '--cmd' 'let g:ruby_host_prog='\''/nix/store/mxahv8pbnsqf2ac25ndwk57nh6xdz8gr-neovim-0.4.3/bin/nvim-ruby'\''' -u /nix/store/ihmncivvbb6m0myzravv0i4pxk8v030h-vimrc "$@"

[infinisil]

Looking good to me now! Can you squash the commits into one (or so)? I think this is mergeable then

[doronbehar]

Looking good to me now! Can you squash the commits into one (or so)? I think this is mergeable then

Awesome :). Now squashed all into 1 commit.

[infinisil]

Thanks for the PR!

[doronbehar]

Thank You!

[cideM]

@doronbehar

The line rm $out/share/applications/nvim.desktop $out/bin/nvim causes my home-manager switch to fail on MacOS with

rm: cannot remove '/nix/store/k3qk58j2yin6hild853fjhrcqcpgws5a-neovim-master/share/applications/nvim.desktop': No such file or directory

Does not happen on an older commit of nixpkgs (have it pinned with niv)

[doronbehar]

Right, I forgot that Darwin don't have desktop entries. Should be fixed with #94643 .