Fix neovim PYTHONPATH handling #43388

symphorien · 2018-07-11T21:23:16Z

Motivation for this change

These are two more or less independent improvements to the neovim wrapper.
1st commit:

nix-shell -p pythonPackages.numpy
nvim
:!python -c "import numpy"

this will fail to see numpy.
This is because the neovim wrapper unsets PYTHONPATH.

With this fix, I checked that :checkhealth is fine with and without extraPythonPackages and within and out of a nix-shell modifying the PYTHONPATH.

2nd commit:
During testing the above fix, I built my system config with an overlay using the neovim from my nixpkgs checkout. I override this wrapper with a few extraPythonPackages. But these packages disappeared completely. They were not even a dependency of neovim. Indeed, a python module from stable will be silently ignored by unstable python.buildEnv.

nixpkgs/pkgs/top-level/python-packages.nix

Lines 89 to 98 in f601ecf

    
           # Check whether a derivation provides a Python module. 
        
           hasPythonModule = drv: (hasAttr "pythonModule" drv) && ( (getAttr "pythonModule" drv) == python); 
        
           # Get list of required Python modules given a list of derivations. 
        
           requiredPythonModules = drvs: let 
        
             filterNull = list: filter (x: !isNull x) list; 
        
             conditionalGetRecurse = attr: condition: drv: let f = conditionalGetRecurse attr condition; in 
        
               (if (condition drv) then unique [drv]++(concatMap f (filterNull(getAttr attr drv))) else []); 
        
             _required = drv: conditionalGetRecurse "propagatedBuildInputs" hasPythonModule drv; 
        
           in [python] ++ (unique (concatMap _required (filterNull drvs)));

nixpkgs/pkgs/development/interpreters/python/wrapper.nix

Line 12 in f601ecf

paths = requiredPythonModules (extraLibs ++ [ python ] ) ;

python.withPackages is just as powerful as python.buildEnv but not so treacherous, and I lost enough time with this, so I switched to python.withPackages.

This changes the type of extraPython{,3}Packages but in a backward compatible way.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
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 nox --run "nox-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)
Fits CONTRIBUTING.md.

symphorien · 2018-07-11T21:28:22Z

cc @manveru @garbas @rvolosatovs as maintainers

manveru · 2018-07-11T21:52:33Z

I love this change, thanks for fixing this, since I couldn't figure out a sane way to solve this issue.

FRidh · 2018-07-12T05:15:49Z

This wiping is necessary for the python providers, though, otherwise a
python2 nix-shell will make the python3 provider read python2 files.

Why set PYTHONPATH to begin with? I mean, isn't it expected behaviour that PYTHONPATH can break things. How about using nix-shell -p "python.withPackages(ps: with ps; numpy)" which is the recommended solution? Would the issue then still exist?

I think overall it is a good change, and I don't object to it.

manveru · 2018-07-12T06:57:39Z

There are a lot of neovim plugins written in python, and that was the only way I could get them to work more or less reliably, no matter what python version was in my environment at the time.

garbas

Looks good for me.

Mic92 · 2018-07-12T14:00:51Z

pkgs/applications/editors/neovim/wrapper.nix

-  };
+  pythonEnv = pythonPackages.python.withPackages(ps:
+        (if withPyGUI then [ ps.neovim_gui ] else [ ps.neovim ])
+        ++ (extraPythonPackagesFun ps) ++ pluginPythonPackages);


This will not work yet with pluginPythonPackages since ps is not passed to pluginPythonPackages. pluginPythonPackages also does not work with vim's own package manager. We only have one plugin that is using this mechanism - should it be removed for the moment? It is also not implemented for vim8.

Hum I think it still works, but indeed adding a plugin from stable will not work with neovim from unstable. I'll try to adapt it.

Hum does anyone have an example of how to use the configure argument of the wrapper ?

I'm using it here: https://github.com/manveru/vimrc/blob/master/default.nix

symphorien · 2018-07-12T14:07:28Z

Why set PYTHONPATH to begin with? I mean, isn't it expected behaviour that PYTHONPATH can break things. How about using nix-shell -p "python.withPackages(ps: with ps; numpy)" which is the recommended solution? Would the issue then still exist?

Indeed this way the issue disappears. But for example, I discovered this issue with pypi2nix, so current tooling sometimes uses PYTHONPATH.

symphorien · 2018-07-12T20:53:12Z

I pushed a commit making ensime-vim.pythonDependencies a function as well.
This was tested with this vim config:

let
  pkgs = import ./default.nix {};
  config = {
    packages.myVimPackage = with pkgs.vimPlugins; {
      start = [ ensime-vim ];
    };
  };
  neovim = pkgs.neovim.override {
    configure = config;
  };
in
  neovim

Note that this didn't work before, python dependencies were only taken into account for vam and pathogen plugins.
besides, it seems ensime switched to python3.

This solves the following bug: opening neovim in nix-shell -p pythonPackages.numpy does not enable to run successfully :!python -c "import numpy" because the PYTHONPATH is wiped by the neovim wrapper. This wiping is necessary for the python providers, though, otherwise a python2 nix-shell will make the python3 provider read python2 files. We wrap the providers only, instead of neovim as whole.

They are both as powerful, but buildEnv is treacherous: if you pass a package which depends on another python (for example the one of unstable when you are on stable) it will be *silently* dropped, leading to hair pulling. Use case: override neovim from unstable, but still keep stable's pythonPackages.

symphorien · 2018-07-24T19:26:11Z

I have resolved the merge conflict and but only tested a little.

A function of the same signature as the argument of python.withPackages

symphorien · 2018-08-13T19:16:38Z

Anything I should change or can this be merged ?

matthewbauer

Looks good to me! I'll let someone with more familiarity with neovim merge though.

Mic92 · 2018-08-17T14:53:31Z

cc @FRidh

xeji · 2018-08-30T07:33:42Z

@GrahamcOfBorg build vimPlugins.ensime-vim neovim-qt

GrahamcOfBorg · 2018-08-30T07:35:30Z

Failure on aarch64-linux (full log)

Attempted: vimPlugins.ensime-vim, neovim-qt

Partial log (click to expand)

        [C]: in function 'match'
        /build/source/src/nvim/generators/gen_declarations.lua:255: in main chunk
        [C]: at 0x00405660
make[2]: *** [src/nvim/CMakeFiles/nvim.dir/build.make:379: src/nvim/auto/ex_cmds.c.generated.h] Error 1
make[1]: *** [CMakeFiles/Makefile2:2865: src/nvim/CMakeFiles/nvim.dir/all] Error 2
make: *** [Makefile:152: all] Error 2
builder for '/nix/store/8wlprfis36fzvqd5a8shbxi0rp6qrshv-neovim-unwrapped-0.3.1.drv' failed with exit code 2
cannot build derivation '/nix/store/a1b5hvm78zx5d1k5vcnkv3dq6cxihc1x-neovim-0.3.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/4292n7x7fwqgp2g0qhx9pav86wjd30av-neovim-qt-0.2.9.drv': 1 dependencies couldn't be built
error: build of '/nix/store/4292n7x7fwqgp2g0qhx9pav86wjd30av-neovim-qt-0.2.9.drv' failed

GrahamcOfBorg · 2018-08-30T07:35:52Z

Success on x86_64-darwin (full log)

Attempted: vimPlugins.ensime-vim, neovim-qt

Partial log (click to expand)

-- Installing: /nix/store/5gbxqi5lqxkv4srr9qx62a38l43jsk7q-neovim-qt-0.2.9/bin/nvim-qt.app/Contents/Resources/runtime/doc/nvim_gui_shim.txt
-- Installing: /nix/store/5gbxqi5lqxkv4srr9qx62a38l43jsk7q-neovim-qt-0.2.9/bin/nvim-qt.app/Contents/Resources/runtime/doc/tags
-- Installing: /nix/store/5gbxqi5lqxkv4srr9qx62a38l43jsk7q-neovim-qt-0.2.9/bin/nvim-qt.app/Contents/Resources/neovim.icns
-- Installing: /nix/store/5gbxqi5lqxkv4srr9qx62a38l43jsk7q-neovim-qt-0.2.9/bin/nvim-qt.app/Contents/Info.plist
post-installation fixup
strip is /nix/store/df6k4mgdjxciy0f637lryp7c9ln7n1m3-cctools-binutils-darwin/bin/strip
patching script interpreter paths in /nix/store/5gbxqi5lqxkv4srr9qx62a38l43jsk7q-neovim-qt-0.2.9
postPatchMkspecs
/nix/store/ignjgbp7yypzvxz3iq6g4wrdj43i3fqs-vimplugin-ensime-vim-2018-04-21
/nix/store/5gbxqi5lqxkv4srr9qx62a38l43jsk7q-neovim-qt-0.2.9

GrahamcOfBorg · 2018-08-30T07:39:41Z

Success on x86_64-linux (full log)

Attempted: vimPlugins.ensime-vim, neovim-qt

Partial log (click to expand)

post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/62ird20c2bgki2l2g866f173kns15qmj-neovim-qt-0.2.9
shrinking /nix/store/62ird20c2bgki2l2g866f173kns15qmj-neovim-qt-0.2.9/bin/.nvim-qt-wrapped
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/62ird20c2bgki2l2g866f173kns15qmj-neovim-qt-0.2.9/bin
patching script interpreter paths in /nix/store/62ird20c2bgki2l2g866f173kns15qmj-neovim-qt-0.2.9
checking for references to /build in /nix/store/62ird20c2bgki2l2g866f173kns15qmj-neovim-qt-0.2.9...
postPatchMkspecs
/nix/store/m9qz81fbrx3k74n6330gxj62g0n1yd2b-vimplugin-ensime-vim-2018-04-21
/nix/store/62ird20c2bgki2l2g866f173kns15qmj-neovim-qt-0.2.9

xeji · 2018-08-30T07:43:59Z

failure on aarch64 is unrelated, has been there for a while: https://hydra.nixos.org/job/nixpkgs/trunk/neovim-unwrapped.aarch64-linux

symphorien changed the title ~~Neovim python env~~ Fix neovim PYTHONPATH handling Jul 11, 2018

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jul 11, 2018

FRidh added the 6.topic: python label Jul 12, 2018

garbas approved these changes Jul 12, 2018

View reviewed changes

Mic92 reviewed Jul 12, 2018

View reviewed changes

GrahamcOfBorg added 6.topic: vim and removed 6.topic: python labels Jul 12, 2018

matthewbauer added the 2.status: merge conflict label Jul 23, 2018

symphorien added 2 commits July 24, 2018 15:02

symphorien force-pushed the neovim-python-env branch from 908ab1c to 53bd547 Compare July 24, 2018 19:24

GrahamcOfBorg removed the 2.status: merge conflict label Jul 24, 2018

symphorien force-pushed the neovim-python-env branch from 53bd547 to dddaa94 Compare July 26, 2018 22:28

matthewbauer approved these changes Aug 16, 2018

View reviewed changes

Mic92 merged commit 663951e into NixOS:master Aug 30, 2018

rvolosatovs mentioned this pull request Sep 8, 2018

neovim: support new extraPython*Packages options, fix configure nix-community/home-manager#375

Closed

symphorien deleted the neovim-python-env branch March 21, 2020 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix neovim PYTHONPATH handling #43388

Fix neovim PYTHONPATH handling #43388

symphorien commented Jul 11, 2018

symphorien commented Jul 11, 2018

manveru commented Jul 11, 2018

FRidh commented Jul 12, 2018

manveru commented Jul 12, 2018

garbas left a comment

Mic92 Jul 12, 2018

symphorien Jul 12, 2018

symphorien Jul 12, 2018

manveru Jul 12, 2018

symphorien commented Jul 12, 2018

symphorien commented Jul 12, 2018

symphorien commented Jul 24, 2018

symphorien commented Aug 13, 2018

matthewbauer left a comment

Mic92 commented Aug 17, 2018

xeji commented Aug 30, 2018

GrahamcOfBorg commented Aug 30, 2018

GrahamcOfBorg commented Aug 30, 2018

GrahamcOfBorg commented Aug 30, 2018

xeji commented Aug 30, 2018

	# Check whether a derivation provides a Python module.
	hasPythonModule = drv: (hasAttr "pythonModule" drv) && ( (getAttr "pythonModule" drv) == python);

	# Get list of required Python modules given a list of derivations.
	requiredPythonModules = drvs: let
	filterNull = list: filter (x: !isNull x) list;
	conditionalGetRecurse = attr: condition: drv: let f = conditionalGetRecurse attr condition; in
	(if (condition drv) then unique [drv]++(concatMap f (filterNull(getAttr attr drv))) else []);
	_required = drv: conditionalGetRecurse "propagatedBuildInputs" hasPythonModule drv;
	in [python] ++ (unique (concatMap _required (filterNull drvs)));

Fix neovim PYTHONPATH handling #43388

Fix neovim PYTHONPATH handling #43388

Conversation

symphorien commented Jul 11, 2018

Motivation for this change

Things done

symphorien commented Jul 11, 2018

manveru commented Jul 11, 2018

FRidh commented Jul 12, 2018

manveru commented Jul 12, 2018

garbas left a comment

Choose a reason for hiding this comment

Mic92 Jul 12, 2018

Choose a reason for hiding this comment

symphorien Jul 12, 2018

Choose a reason for hiding this comment

symphorien Jul 12, 2018

Choose a reason for hiding this comment

manveru Jul 12, 2018

Choose a reason for hiding this comment

symphorien commented Jul 12, 2018

symphorien commented Jul 12, 2018

symphorien commented Jul 24, 2018

symphorien commented Aug 13, 2018

matthewbauer left a comment

Choose a reason for hiding this comment

Mic92 commented Aug 17, 2018

xeji commented Aug 30, 2018

GrahamcOfBorg commented Aug 30, 2018

GrahamcOfBorg commented Aug 30, 2018

GrahamcOfBorg commented Aug 30, 2018

xeji commented Aug 30, 2018