fix(nodeenv): let -p target a virtualenv nodeenv isn't installed in (#402)

* fix(nodeenv): let -p target a virtualenv nodeenv isn't installed in

-p now takes an optional directory, so a pipx/pipsi/uv tool installation
can set up node.js in any python virtualenv.

Without an argument the activated VIRTUAL_ENV is preferred over
sys.prefix: when nodeenv lives in its own virtualenv, sys.prefix points
at that virtualenv instead of the activated one.

Closes #156

* fix(nodeenv): warn when -p picks the activated virtualenv over its own

The activated VIRTUAL_ENV and the virtualenv nodeenv is installed in can
only differ when nodeenv is installed elsewhere, and then the choice is
ambiguous, so log which one is used and how to override it.

The three sys.prefix branches all resolved to the same value, they are
folded into a single check reused by the warning.

Author: ekalinin

CHANGES

@@ -16,6 +16,9 @@ Version [unreleased]
   virtualenv `#159 <https://github.com/ekalinin/nodeenv/issues/159>`_
 - Repeated `-p` runs no longer duplicate the `predeactivate` hook
   `#159 <https://github.com/ekalinin/nodeenv/issues/159>`_
+- `-p` accepts an optional virtualenv directory and prefers the activated
+  `VIRTUAL_ENV` over the virtualenv nodeenv itself is installed in
+  `#156 <https://github.com/ekalinin/nodeenv/issues/156>`_
 
 Version 1.3.1
 -------------

README.rst

@@ -276,9 +276,13 @@ Basic options
 ``-l, --list``
     Lists available node.js versions.
 
-``-p, --python-virtualenv``
-    Use current python virtualenv. Running it again with the same node
-    version does not reinstall node; pass ``--force`` to reinstall.
+``-p [VENV_DIR], --python-virtualenv [VENV_DIR]``
+    Use the given python virtualenv, or the current one if no directory
+    is given. Passing a directory is required when nodeenv lives in its
+    own virtualenv (``pipx``, ``pipsi``, ``uv tool``) and the activated
+    virtualenv doesn't export ``VIRTUAL_ENV``. Running it again with the
+    same node version does not reinstall node; pass ``--force`` to
+    reinstall.
 
 ``-r FILENAME, --requirements=FILENAME``
     Install all the packages listed in the given requirements file.

nodeenv.py

@@ -566,8 +566,9 @@ def make_parser():
 
     parser.add_argument(
         '--python-virtualenv', '-p', dest='python_virtualenv',
-        action='store_true', default=False,
-        help='Use current python virtualenv')
+        nargs='?', const=True, default=False, metavar='VENV_DIR',
+        help='Use the given python virtualenv, or the current one '
+        'if no directory is given')
 
     parser.add_argument(
         '--clean-src', '-c', dest='clean_src',
@@ -1381,14 +1382,27 @@ def resolve_node_version(spec):
 
 def get_env_dir(args):
     if args.python_virtualenv:
-        if hasattr(sys, 'real_prefix'):
-            res = sys.prefix
-        elif hasattr(sys, 'base_prefix') and sys.base_prefix != sys.prefix:
-            res = sys.prefix
-        elif 'CONDA_PREFIX' in os.environ:
-            res = sys.prefix
-        elif 'VIRTUAL_ENV' in os.environ:
+        # whether nodeenv itself is running inside a python virtualenv
+        in_virtualenv = (
+            hasattr(sys, 'real_prefix') or
+            (hasattr(sys, 'base_prefix') and sys.base_prefix != sys.prefix) or
+            'CONDA_PREFIX' in os.environ)
+        if args.python_virtualenv is not True:
+            res = args.python_virtualenv
+            if not os.path.isdir(res):
+                logger.error("Python virtualenv '%s' doesn't exist", res)
+                sys.exit(2)
+        # nodeenv itself can be installed into its own virtualenv
+        # (pipx, pipsi, uv tool), so the activated one wins over sys.prefix
+        elif os.environ.get('VIRTUAL_ENV'):
             res = os.environ['VIRTUAL_ENV']
+            if in_virtualenv and res != sys.prefix:
+                logger.warning(
+                    ' * Using activated virtualenv %s, not %s where nodeenv '
+                    'is installed, pass a directory to -p to override',
+                    res, sys.prefix)
+        elif in_virtualenv:
+            res = sys.prefix
         else:
             logger.error('No python virtualenv is available')
             sys.exit(2)

tests/nodeenv_test.py

@@ -698,6 +698,18 @@ def test_parse_args_prefer_system():
         assert nodeenv.parse_args().prefer_system is False
 
 
+def test_parse_args_python_virtualenv():
+    with mock.patch.object(sys, 'argv', ['nodeenv', '-p']):
+        assert nodeenv.parse_args().python_virtualenv is True
+    with mock.patch.object(sys, 'argv', ['nodeenv', '-p', 'venv']):
+        assert nodeenv.parse_args().python_virtualenv == 'venv'
+    with mock.patch.object(
+            sys, 'argv', ['nodeenv', '--python-virtualenv=venv']):
+        assert nodeenv.parse_args().python_virtualenv == 'venv'
+    with mock.patch.object(sys, 'argv', ['nodeenv', 'env']):
+        assert nodeenv.parse_args().python_virtualenv is False
+
+
 def test_isolate_npm_default():
     assert nodeenv.Config._default['isolate_npm'] is False
 
@@ -1622,7 +1634,8 @@ def test_with_python_virtualenv_real_prefix(self):
         test_prefix = '/path/to/virtualenv'
 
         with mock.patch.object(sys, 'real_prefix', test_prefix, create=True), \
-             mock.patch.object(sys, 'prefix', test_prefix):
+             mock.patch.object(sys, 'prefix', test_prefix), \
+             mock.patch.dict(os.environ, {}, clear=True):
             result = nodeenv.get_env_dir(args)
             assert result == test_prefix
 
@@ -1637,12 +1650,14 @@ def test_with_python_virtualenv_base_prefix(self):
         if hasattr(sys, 'real_prefix'):
             with mock.patch.object(sys, 'real_prefix', create=False):
                 with mock.patch.object(sys, 'prefix', test_prefix), \
-                     mock.patch.object(sys, 'base_prefix', test_base_prefix):
+                     mock.patch.object(sys, 'base_prefix', test_base_prefix), \
+                     mock.patch.dict(os.environ, {}, clear=True):
                     result = nodeenv.get_env_dir(args)
                     assert result == test_prefix
         else:
             with mock.patch.object(sys, 'prefix', test_prefix), \
-                 mock.patch.object(sys, 'base_prefix', test_base_prefix):
+                 mock.patch.object(sys, 'base_prefix', test_base_prefix), \
+                 mock.patch.dict(os.environ, {}, clear=True):
                 result = nodeenv.get_env_dir(args)
                 assert result == test_prefix
 
@@ -1658,14 +1673,14 @@ def test_with_python_virtualenv_conda_prefix(self):
                 env_dict = {'CONDA_PREFIX': test_prefix}
                 with mock.patch.object(sys, 'prefix', test_prefix), \
                      mock.patch.object(sys, 'base_prefix', test_prefix), \
-                     mock.patch.dict(os.environ, env_dict):
+                     mock.patch.dict(os.environ, env_dict, clear=True):
                     result = nodeenv.get_env_dir(args)
                     assert result == test_prefix
         else:
             env_dict = {'CONDA_PREFIX': test_prefix}
             with mock.patch.object(sys, 'prefix', test_prefix), \
                  mock.patch.object(sys, 'base_prefix', test_prefix), \
-                 mock.patch.dict(os.environ, env_dict):
+                 mock.patch.dict(os.environ, env_dict, clear=True):
                 result = nodeenv.get_env_dir(args)
                 assert result == test_prefix
 
@@ -1716,6 +1731,74 @@ def test_with_python_virtualenv_no_virtualenv_exits(self):
                 nodeenv.get_env_dir(args)
             assert exc_info.value.code == 2
 
+    def test_with_python_virtualenv_dir(self, tmpdir):
+        """Test get_env_dir when a virtualenv directory is given"""
+        args = mock.Mock()
+        args.python_virtualenv = str(tmpdir)
+
+        env_dict = {'VIRTUAL_ENV': '/path/to/other/venv'}
+        with mock.patch.dict(os.environ, env_dict, clear=True):
+            result = nodeenv.get_env_dir(args)
+            assert result == str(tmpdir)
+
+    def test_with_python_virtualenv_missing_dir_exits(self, tmpdir):
+        """Test get_env_dir exits when the given virtualenv doesn't exist"""
+        args = mock.Mock()
+        args.python_virtualenv = str(tmpdir.join('missing'))
+
+        with pytest.raises(SystemExit) as exc_info:
+            nodeenv.get_env_dir(args)
+        assert exc_info.value.code == 2
+
+    def test_with_python_virtualenv_prefers_virtual_env(self):
+        """Test get_env_dir prefers VIRTUAL_ENV over nodeenv's own venv"""
+        args = mock.Mock()
+        args.python_virtualenv = True
+        # nodeenv itself is installed into its own virtualenv
+        test_prefix = '/path/to/nodeenv/venv'
+        virtual_env = '/path/to/activated/venv'
+
+        env_dict = {'VIRTUAL_ENV': virtual_env}
+        with mock.patch.object(sys, 'real_prefix', test_prefix, create=True), \
+             mock.patch.object(sys, 'prefix', test_prefix), \
+             mock.patch.dict(os.environ, env_dict, clear=True), \
+             mock.patch.object(nodeenv.logger, 'warning') as mck:
+            result = nodeenv.get_env_dir(args)
+            assert result == virtual_env
+            # the ignored virtualenv is not silently dropped
+            assert mck.call_count == 1
+            assert mck.call_args[0][1:] == (virtual_env, test_prefix)
+
+    def test_with_python_virtualenv_same_venv_is_quiet(self):
+        """Test get_env_dir doesn't warn when both point to the same venv"""
+        args = mock.Mock()
+        args.python_virtualenv = True
+        test_prefix = '/path/to/venv'
+
+        env_dict = {'VIRTUAL_ENV': test_prefix}
+        with mock.patch.object(sys, 'real_prefix', test_prefix, create=True), \
+             mock.patch.object(sys, 'prefix', test_prefix), \
+             mock.patch.dict(os.environ, env_dict, clear=True), \
+             mock.patch.object(nodeenv.logger, 'warning') as mck:
+            result = nodeenv.get_env_dir(args)
+            assert result == test_prefix
+            mck.assert_not_called()
+
+    def test_with_python_virtualenv_system_python_is_quiet(self):
+        """Test get_env_dir doesn't warn when nodeenv runs system-wide"""
+        args = mock.Mock()
+        args.python_virtualenv = True
+        virtual_env = '/path/to/activated/venv'
+
+        env_dict = {'VIRTUAL_ENV': virtual_env}
+        with mock.patch.object(sys, 'prefix', '/usr'), \
+             mock.patch.object(sys, 'base_prefix', '/usr'), \
+             mock.patch.dict(os.environ, env_dict, clear=True), \
+             mock.patch.object(nodeenv.logger, 'warning') as mck:
+            result = nodeenv.get_env_dir(args)
+            assert result == virtual_env
+            mck.assert_not_called()
+
     def test_without_python_virtualenv(self):
         """Test get_env_dir when not using python virtualenv"""
         args = mock.Mock()