Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support shells with "set -u" #345

Merged
merged 1 commit into from
May 28, 2024
Merged

Support shells with "set -u" #345

merged 1 commit into from
May 28, 2024

Conversation

TomiBelan
Copy link
Contributor

Currently the nodeenv activation code cannot run in bash/zsh shells with "set -u" enabled. This PR fixes it.

Usually you activate virtual environments in an interactive shell, but sometimes it is useful to do it inside a shell script. When writing shell scripts it is often recommended to enable additional strict error checking options, for example set -e -u -o pipefail. -u means reading undefined variables is an error.

It is based on the equivalent change in CPython:

Example before this PR:

$ python3 -m venv test1
$ test1/bin/pip install nodeenv
Collecting nodeenv
  Using cached nodeenv-1.8.0-py2.py3-none-any.whl (22 kB)
Requirement already satisfied: setuptools in ./test1/lib/python3.10/site-packages (from nodeenv) (59.6.0)
Installing collected packages: nodeenv
Successfully installed nodeenv-1.8.0
$ test1/bin/nodeenv -p
 * Install prebuilt node (21.2.0) ..... done.
 * Appending data to /home/tomi/tmpt/nodeenv/test1/bin/activate
 * Appending data to /home/tomi/tmpt/nodeenv/test1/bin/activate.fish
$ test1/bin/npm
ERROR: npm v10.2.3 is known not to run on Node.js v12.22.9.  This version of npm supports the following node versions: `^18.17.0 || >=20.5.0`. You can find the latest version at https://nodejs.org/.
...
$ bash -c 'source test1/bin/activate; npm --version'
10.2.3
$ bash -c 'set -u; source test1/bin/activate; npm --version'
test1/bin/activate: line 82: _OLD_NODE_VIRTUAL_PATH: unbound variable

After this PR it works properly:

$ test3/bin/pip install ./nodeenv/
...
$ test3/bin/nodeenv -p
...
$ bash -c 'source test3/bin/activate; npm --version'
10.2.3
$ bash -c 'set -u; source test3/bin/activate; npm --version'
10.2.3

@ekalinin ekalinin merged commit de428ee into ekalinin:master May 28, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants