Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

passenv should fail on environment variables containing spaces #2658

Closed
ericzolf opened this issue Dec 9, 2022 · 4 comments · Fixed by #2671
Closed

passenv should fail on environment variables containing spaces #2658

ericzolf opened this issue Dec 9, 2022 · 4 comments · Fixed by #2671
Labels
enhancement help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.

Comments

@ericzolf
Copy link
Contributor

ericzolf commented Dec 9, 2022

Issue

Since tox 4.0, passenv is a comma separated list of environment variables, where it used to be a space separated list:
https://tox.wiki/en/4.0.3/faq.html#tox-4-changed-ini-rules

I'm fine with this, BUT tox silently accepts a space separated list of environment variables, even though the environment variables are silently not passed, making root cause analysis more difficult than necessary for people (like me) migrating from tox 3 to 4. Since no shell (AFAIK) accepts environment variables with spaces in it, once the list has been split, none of its elements should contain spaces, or a meaningful error should be output.

Environment

Provide at least:

  • OS: Fedora 37 or Ubuntu 20.02
  • pip list of the host Python where tox is installed:
$ pip list
Package       Version
------------- -------
cachetools    5.2.0
chardet       5.1.0
colorama      0.4.6
distlib       0.3.6
filelock      3.8.2
packaging     22.0
pip           22.3.1
platformdirs  2.6.0
pluggy        1.0.0
py            1.11.0
pyproject_api 1.2.1
setuptools    62.6.0
six           1.16.0
tox           4.0.3
virtualenv    20.17.1
wheel         0.38.4

Output of running tox

MYENV None

I'd expect an error message of some kind that the passenv format isn't correct.

Minimal example

[testenv]
passenv = MYENV YOURENV
commands =
    python -c 'import os; print("MYENV", os.getenv("MYENV"))'
@gaborbernat
Copy link
Member

PR welcome 😁

@ericzolf
Copy link
Contributor Author

ericzolf commented Dec 9, 2022

Taking the challenge but I might need a bit of help:

  • pass_env_post_process in src/tox/tox_env/api.py seems to be a good place to detect the error
  • I guess logging.error is to be used to report errors
  • but how to properly handle the error, exit the program? Raise a Fail exception?

@gaborbernat
Copy link
Member

gaborbernat commented Dec 9, 2022

I think raising the failure error.

raise HandledError(msg)

@gaborbernat gaborbernat added help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. enhancement labels Dec 9, 2022
@gaborbernat gaborbernat added this to the 4.0.x milestone Dec 9, 2022
ericzolf added a commit to ericzolf/tox that referenced this issue Dec 10, 2022
Avoids to oversee pass_env/passenv going from blank to comma separated list
Resolves tox-dev#2658
ericzolf added a commit to ericzolf/tox that referenced this issue Dec 10, 2022
Avoids to oversee pass_env/passenv going from blank to comma separated list
Resolves tox-dev#2658
ericzolf added a commit to ericzolf/tox that referenced this issue Dec 10, 2022
Avoids to oversee pass_env/passenv going from blank to comma separated list
Resolves tox-dev#2658
@ericzolf
Copy link
Contributor Author

Raising a HandledError made tox fail with a useless message (not mine at least!), so I raised "Fail", which looks fine to me. I'll complete the draft PR #2671 once I've got a sign that my changes look OK.

ericzolf added a commit to ericzolf/tox that referenced this issue Dec 10, 2022
Avoids to oversee pass_env/passenv going from blank to comma separated list
Resolves tox-dev#2658
gaborbernat pushed a commit that referenced this issue Dec 11, 2022
Co-authored-by: Eric L <ewl+git@lavar.de>
Co-authored-by: Bernát Gábor <bgabor8@bloomberg.net>
Resolves #2658
benjaoming added a commit to benjaoming/readthedocs.org that referenced this issue Dec 13, 2022
benjaoming added a commit to readthedocs/readthedocs.org that referenced this issue Dec 13, 2022
Fix silent, then loud failure after Tox 4 upgrade re: tox-dev/tox#2658
lpsinger added a commit to lpsinger/package-template that referenced this issue Jan 31, 2023
lpsinger added a commit to lpsinger/ligo.skymap that referenced this issue Jan 31, 2023
odl-github pushed a commit to opendaylight/transportpce that referenced this issue Sep 5, 2023
- use one variable per line for tox3 support

One variable is the only syntax supported by both tox3 and tox4.

https: //tox.wiki/en/4.0.3/faq.html#tox-4-changed-ini-rules
tox-dev/tox#2658

Signed-off-by: guillaume.lambert <guillaume.lambert@orange.com>
Change-Id: I7db5c9492ea28b4265fd0a126efe9137417d9a35
odl-github pushed a commit to opendaylight/transportpce that referenced this issue Oct 14, 2023
- use one variable per line for tox3 support

One variable is the only syntax supported by both tox3 and tox4.

https: //tox.wiki/en/4.0.3/faq.html#tox-4-changed-ini-rules
tox-dev/tox#2658

Signed-off-by: guillaume.lambert <guillaume.lambert@orange.com>
Change-Id: I7db5c9492ea28b4265fd0a126efe9137417d9a35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants