From b9a0e3aaac5aca0f9cf7e179fc7c92727d8a0251 Mon Sep 17 00:00:00 2001 From: Eugene Kalinin Date: Mon, 26 Oct 2026 00:40:06 +0300 Subject: [PATCH] fix(nodeenv): do not put the venv bin on PATH twice with -p The python venv's own activate and activate.fish put bin/ on PATH, and the part nodeenv -p appends to them added it a second time. That part now adds only node_modules/.bin. A file nodeenv wrote itself (a conda env has none) keeps the entry, so a rerun with other options does not take bin/ off PATH again. Windows keeps it too: before python 3.12 the venv's git-bash activate puts a C:\ path there, which the shell splits at the colon. Fixes #332 --- CHANGES | 6 +++ nodeenv.py | 29 ++++++++++++ tests/test_activate_shells.py | 35 +++++++++++++++ tests/test_install_activate.py | 82 ++++++++++++++++++++++++++++++++++ 4 files changed, 152 insertions(+) diff --git a/CHANGES b/CHANGES index 56e84d4..c42af9e 100644 --- a/CHANGES +++ b/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 `_ - 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 diff --git a/nodeenv.py b/nodeenv.py index 7177ae1..c643526 100644 --- a/nodeenv.py +++ b/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) diff --git a/tests/test_activate_shells.py b/tests/test_activate_shells.py index b6a60ef..78fc7ef 100644 --- a/tests/test_activate_shells.py +++ b/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']) diff --git a/tests/test_install_activate.py b/tests/test_install_activate.py index e83aae5..c7f5623 100644 --- a/tests/test_install_activate.py +++ b/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`