fix(nodeenv): do not put the venv bin on PATH twice with -p

CHANGES

@@ -4,6 +4,12 @@ Nodeenv changelog
 Version [unreleased]
 --------------------
 
+- `nodeenv -p` no longer puts the python venv's `bin` directory on `PATH`
+  a second time: the venv's own `activate` and `activate.fish` put it there
+  already. Envs without those scripts, such as conda envs, and the git-bash
+  `activate` on Windows keep it, and running `nodeenv -p` again fixes an
+  existing venv
+  `#332 <https://github.com/ekalinin/nodeenv/issues/332>`_
 - Require Python 3.10 or later, matching the minimum version tested in CI.
   Update package classifiers and wheel tags, and remove the unused Python 2
   compatibility paths. Old Node.js source builds still require Python 2.7

nodeenv.py

@@ -1294,6 +1294,31 @@ def install_packages(env_dir, args):
     'Activate.ps1',
 ))
 
+# the bin/ entry `activate` and `activate.fish` put on PATH: the python
+# venv's own scripts they are appended to put it there already.  Windows
+# keeps it: before python 3.12 the venv's `activate` puts a `C:\...` path
+# there, which git-bash splits at the colon, and the cmd and PowerShell
+# scripts reset PATH to the venv's saved one first
+# https://github.com/ekalinin/nodeenv/issues/332
+# https://github.com/python/cpython/issues/103088
+VENV_BIN_PATH = {
+    'activate': '$NODE_VIRTUAL_ENV/__BIN_NAME__:',
+    'activate.fish': ' "$NODE_VIRTUAL_ENV/__BIN_NAME__"',
+}
+
+
+def is_python_venv_script(file_path, name):
+    """
+    Whether `nodeenv -p` appends to a script of the python venv.  An env
+    without one (conda) gets it written from scratch, starting with the
+    DISABLE_PROMPT part, and a rerun appends to that file
+    """
+    if not os.path.exists(file_path):
+        return False
+    with open(file_path, 'rb') as f:
+        content = f.read()
+    return not content.startswith(DISABLE_PROMPT[name].encode('utf-8'))
+
 
 def install_activate(env_dir, args):
     """
@@ -1338,6 +1363,10 @@ def install_activate(env_dir, args):
 
     for name, content in files.items():
         file_path = join(bin_dir, name)
+        if args.python_virtualenv and not is_WIN and \
+                name in VENV_BIN_PATH and \
+                is_python_venv_script(file_path, name):
+            content = content.replace(VENV_BIN_PATH[name], '')
         isolate = NPM_ISOLATE.get(name, '') if args.isolate_npm else ''
         unisolate = NPM_UNISOLATE.get(name, '') if args.isolate_npm else ''
         content = content.replace('__NPM_ISOLATE__', isolate)

tests/test_activate_shells.py

@@ -230,6 +230,27 @@ def env(request, tmpdir):
                str(dumper), bool(request.param))
 
 
+@pytest.fixture
+def python_venv(tmpdir):
+    """
+    A python venv with the activation of `nodeenv -p` appended to its own
+    activate scripts.
+    """
+    venv_dir = tmpdir.join('venv')
+    subprocess.check_call(
+        [sys.executable, '-m', 'venv', '--without-pip', str(venv_dir)])
+
+    dumper = tmpdir.join('dump.py')
+    dumper.write(DUMPER)
+
+    with mock.patch.object(sys, 'argv', ['nodeenv', '-p']):
+        opts = nodeenv.parse_args()
+        nodeenv.install_activate(str(venv_dir), opts)
+
+    return Env(str(venv_dir), str(tmpdir.join('home').ensure(dir=True)),
+               str(dumper), False)
+
+
 def _child_env(env):
     """
     A clean environment: no leftovers from a nodeenv active in the shell
@@ -310,6 +331,20 @@ def test_activate_sets_env(shell, env):
     assert _real(dump['npm_config_prefix']) == _real(env.path)
 
 
+@every_shell
+def test_python_virtualenv_puts_bin_on_path_once(shell, python_venv):
+    # the venv's own activate has put bin/ on PATH already, the part
+    # nodeenv -p appends adds only node_modules/.bin in front of it
+    # https://github.com/ekalinin/nodeenv/issues/332
+    dump = run(shell, python_venv)
+    baseline = run(shell, python_venv, source=False)
+
+    assert [_real(p) for p in dump['PATH'].split(os.pathsep)] == [
+        _real(os.path.join(python_venv.path, 'lib', 'node_modules', '.bin')),
+        _real(os.path.join(python_venv.path, 'bin')),
+    ] + [_real(p) for p in baseline['PATH'].split(os.pathsep)]
+
+
 @restoring_shell
 def test_deactivate_restores_env(shell, env):
     dump = run(shell, env, ['deactivate_node'])

tests/test_install_activate.py

@@ -112,6 +112,9 @@ def test_python_virtualenv(tmpdir, name, content_var):
         disable_prompt = nodeenv.DISABLE_PROMPT.get(name, '')
         enable_prompt = nodeenv.ENABLE_PROMPT.get(name, '')
         content = name + '\n' + disable_prompt + content + enable_prompt
+        # and leave bin/ out of PATH, the venv's activate put it there
+        content = content.replace('$NODE_VIRTUAL_ENV/__BIN_NAME__:', '')
+        content = content.replace(' "$NODE_VIRTUAL_ENV/__BIN_NAME__"', '')
     assert bin_dir.join(name).read() == fix_content(content, tmpdir)
 
 
@@ -262,6 +265,79 @@ def test_python_virtualenv_with_custom_prompt(tmpdir):
             assert 'NODE_VIRTUAL_ENV_DISABLE_PROMPT' in content
 
 
+# python's activate puts the venv's bin/ on PATH already, so the part
+# `nodeenv -p` appends to it must not put it there a second time
+# https://github.com/ekalinin/nodeenv/issues/332
+PATH_LINES = {
+    'activate': ('PATH="$NODE_VIRTUAL_ENV/lib/node_modules/.bin:'
+                 '$NODE_VIRTUAL_ENV/bin:$PATH"'),
+    'activate.fish': ('set -gx PATH "$NODE_VIRTUAL_ENV/lib/node_modules/.bin" '
+                      '"$NODE_VIRTUAL_ENV/bin" $PATH'),
+}
+VENV_PATH_LINES = {
+    'activate': 'PATH="$NODE_VIRTUAL_ENV/lib/node_modules/.bin:$PATH"',
+    'activate.fish': ('set -gx PATH "$NODE_VIRTUAL_ENV/lib/node_modules/.bin" '
+                      '$PATH'),
+}
+
+
+def _path_lines(content):
+    return [line for line in content.splitlines()
+            if line.startswith(('PATH=', 'set -gx PATH '))]
+
+
+def _install_python_virtualenv(tmpdir, *extra_args):
+    argv = ['nodeenv', '-p'] + list(extra_args)
+    with mock.patch.object(sys, 'argv', argv):
+        opts = nodeenv.parse_args()
+        nodeenv.install_activate(str(tmpdir), opts)
+
+
+@pytest.mark.skipif(nodeenv.is_WIN, reason='activate.fish is POSIX only')
+@pytest.mark.parametrize('name', sorted(VENV_PATH_LINES))
+def test_python_virtualenv_puts_bin_on_path_once(tmpdir, name):
+    bin_dir = tmpdir.join('bin')
+    bin_dir.mkdir()
+    for n in FILES:
+        bin_dir.join(n).write(n)
+
+    _install_python_virtualenv(tmpdir)
+
+    assert _path_lines(bin_dir.join(name).read()) == [VENV_PATH_LINES[name]]
+
+
+@pytest.mark.skipif(nodeenv.is_WIN, reason='activate.fish is POSIX only')
+@pytest.mark.parametrize('name', sorted(VENV_PATH_LINES))
+def test_python_virtualenv_rerun_leaves_activate_alone(tmpdir, name):
+    bin_dir = tmpdir.join('bin')
+    bin_dir.mkdir()
+    bin_dir.join('node').write('')
+    for n in FILES:
+        bin_dir.join(n).write(n)
+
+    _install_python_virtualenv(tmpdir)
+    content = bin_dir.join(name).read()
+    _install_python_virtualenv(tmpdir)
+
+    assert bin_dir.join(name).read() == content
+
+
+@pytest.mark.skipif(nodeenv.is_WIN, reason='activate.fish is POSIX only')
+@pytest.mark.parametrize('name', sorted(PATH_LINES))
+def test_python_virtualenv_own_activate_keeps_bin(tmpdir, name):
+    # a conda env has no bin/activate: nodeenv -p writes it from scratch
+    # and puts bin/ on PATH itself, also when a rerun with other options
+    # appends to the file nodeenv wrote
+    bin_dir = tmpdir.join('bin')
+    bin_dir.mkdir()
+    bin_dir.join('node').write('')
+
+    _install_python_virtualenv(tmpdir)
+    _install_python_virtualenv(tmpdir, '--isolate-npm')
+
+    assert _path_lines(bin_dir.join(name).read()) == [PATH_LINES[name]] * 2
+
+
 @pytest.mark.parametrize('extra_args', ([], ['--isolate-npm']))
 def test_all_placeholders_replaced(tmpdir, extra_args):
     """Test that all placeholders are properly replaced in generated files"""
@@ -545,6 +621,12 @@ def test_win_python_virtualenv_appends_to_activate(tmpdir, fake_win):
     content = bin_dir.join('activate').read()
     assert content.startswith('# python venv activate\n')
     assert 'NODE_VIRTUAL_ENV_DISABLE_PROMPT=1' in content
+    # and still puts Scripts on PATH: before python 3.12 the venv's
+    # activate puts it there as a `C:\...` path, which the shell splits
+    # at the colon (#332)
+    # https://github.com/python/cpython/issues/103088
+    assert ('PATH="$NODE_VIRTUAL_ENV/Scripts/node_modules/.bin:'
+            '$NODE_VIRTUAL_ENV/Scripts:$PATH"') in content
 
 
 # python's venv writes these for cmd and PowerShell, and `nodeenv -p`