posargs with `:` crashes virtualenv

[tibortakacs]

Issue

posargs with : are not properly passed, virtualenv crashes.

Environment

Provide at least:

• OS: Ubuntu 20.04
• pip list of the host Python where tox is installed:

Package               Version
--------------------- ---------
anyio                 3.6.2
artifacts-keyring     0.3.2
attrs                 22.2.0
bleach                5.0.1
cachetools            5.2.1
certifi               2022.12.7
cffi                  1.15.1
chardet               5.1.0
charset-normalizer    2.1.1
click                 8.1.3
colorama              0.4.6
commonmark            0.9.1
cryptography          39.0.0
distlib               0.3.6
docutils              0.19
exceptiongroup        1.1.0
filelock              3.9.0
h11                   0.14.0
html5lib              1.1
httpcore              0.16.3
httpx                 0.23.3
idna                  3.4
importlib-metadata    6.0.0
importlib-resources   5.10.2
iniconfig             2.0.0
jaraco.classes        3.2.3
jeepney               0.8.0
keyring               23.13.1
more-itertools        9.0.0
packaging             23.0
pip                   22.3.1
pkginfo               1.9.6
platformdirs          2.6.2
pluggy                1.0.0
pycparser             2.21
pydantic              1.10.4
Pygments              2.14.0
pyproject_api         1.4.0
pytest                7.2.0
pytest-httpx          0.21.2
readme-renderer       37.3
requests              2.28.1
requests-toolbelt     0.10.1
rfc3986               1.5.0
rich                  13.0.1
SecretStorage         3.3.3
setuptools            57.5.0
simpleindex           0.5.0
simpleindex_artifacts 0.3
six                   1.16.0
sniffio               1.3.0
starlette             0.23.1
toml                  0.10.2
tomli                 2.0.1
tox                   4.2.8
twine                 4.0.2
typing_extensions     4.4.0
urllib3               1.26.14
uvicorn               0.20.0
virtualenv            20.17.1
webencodings          0.5.1
wheel                 0.38.4
zipp                  3.11.0

Output of running tox

Provide the output of tox -rvv:

ROOT: 89 D setup logging to DEBUG on pid 26084 [tox/report.py:221]
.pkg: 156 I find interpreter for spec PythonSpec(path=/usr/local/bin/python) [virtualenv/discovery/builtin.py:56]
.pkg: 156 I proposed PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:63]
.pkg: 156 D accepted PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:65]
.pkg: 158 D filesystem is case-sensitive [virtualenv/info.py:24]
.pkg: 180 I find interpreter for spec PythonSpec(path=/usr/local/bin/python) [virtualenv/discovery/builtin.py:56]
.pkg: 180 I proposed PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:63]
.pkg: 181 D accepted PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:65]
.pkg: 184 I find interpreter for spec PythonSpec(path=/usr/local/bin/python) [virtualenv/discovery/builtin.py:56]
.pkg: 185 I proposed PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:63]
.pkg: 185 D accepted PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:65]
.pkg: 189 I find interpreter for spec PythonSpec(path=/usr/local/bin/python) [virtualenv/discovery/builtin.py:56]
.pkg: 190 I proposed PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:63]
.pkg: 190 D accepted PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:65]
.pkg: 204 I find interpreter for spec PythonSpec(path=/usr/local/bin/python) [virtualenv/discovery/builtin.py:56]
.pkg: 204 I proposed PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:63]
.pkg: 205 D accepted PythonInfo(spec=CPython3.8.16.final.0-64, exe=/usr/local/bin/python, platform=linux, version='3.8.16 (default, Dec  8 2022, 03:18:29) \n[GCC 10.2.1 20210110]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:65]
usage: virtualenv [--version] [--with-traceback] [-v | -q] [--read-only-app-data] [--app-data APP_DATA] [--reset-app-data] [--upgrade-embed-wheels] [--discovery {builtin}] [-p py] [--try-first-with py_exe]
                  [--creator {builtin,cpython3-posix,venv}] [--seeder {app-data,pip}] [--no-seed] [--activators comma_sep_list] [--clear] [--no-vcs-ignore] [--system-site-packages] [--symlinks | --copies] [--no-download | --download]
                  [--extra-search-dir d [d ...]] [--pip version] [--setuptools version] [--wheel version] [--no-pip] [--no-setuptools] [--no-wheel] [--no-periodic-update] [--symlink-app-data] [--prompt prompt] [-h]
                  dest
virtualenv: error: argument dest: destination '--junitxml=unit_tests_results.xml --cov=autoavatar --cov-report=xml:coverage.xml' must not contain the path separator (:) as this would break the activation scripts

Minimal example

If possible, provide a minimal reproducer for the issue:

UPDATE After doing the investigation, it turned out that one environment influences the other. Here is a minimum tox.ini to reproduce the issue: tox.int:

[tox]
minversion = 4.3.1

[testenv:hello]
commands =
    python ./helloworld.py {posargs:World}

[testenv:dev]
envdir = {posargs:venv}
usedevelop = True
commands =
    python ./helloworld.py World

tox -e hello -- x:y

...
usage: virtualenv [--version] [--with-traceback] [-v | -q] [--read-only-app-data] [--app-data APP_DATA] [--reset-app-data] [--upgrade-embed-wheels] [--discovery {builtin}] [-p py] [--try-first-with py_exe]
                  [--creator {builtin,cpython3-posix,venv}] [--seeder {app-data,pip}] [--no-seed] [--activators comma_sep_list] [--clear] [--no-vcs-ignore] [--system-site-packages] [--symlinks | --copies] [--no-download | --download]
                  [--extra-search-dir d [d ...]] [--pip version] [--setuptools version] [--wheel version] [--no-pip] [--no-setuptools] [--no-wheel] [--no-periodic-update] [--symlink-app-data] [--prompt prompt] [-h]
                  dest
virtualenv: error: argument dest: destination 'x:y' must not contain the path separator (:) as this would break the activation scripts

[gaborbernat]

PR welcome.

[tibortakacs]

Happy to give it a try next week if that is okay.

[masenf]

~This should be addressed by the parser rewrite i'm working on for #2732~ 🤞

Nevermind, I'm actually unable to reproduce this in my environment. It's possible that the issues are related, but less hopeful that it "just works".

[gaborbernat]

4.3.0 is out so please try out and see if it helped or not.

[tibortakacs]

@gaborbernat It is still there, could you please re-open this ticket?

[tibortakacs]

I have updated the issues with my findings. It seems that if usedevelop is used, posargs is used unexpectedly. I hope I can fix this quickly.

[tibortakacs]

Okay, the "I hope I can fix this quickly" was a famous last sentence, this bug is actually quite complicated, at least for a newbie in the tox codebase. Let me summarize the background of the bug here.

First, all (both active and inactive) tox environments are created (and later the active ones are executed):

https://github.com/tox-dev/tox/blob/a2222e98b320467e821cf9444113e9b40ab3ad1b/src/tox/session/env_select.py#L365

In this call, if the package option is set (e.g., use_develop = True or package = wheel is used), tox builds a corresponding package environment:

https://github.com/tox-dev/tox/blob/a2222e98b320467e821cf9444113e9b40ab3ad1b/src/tox/session/env_select.py#L197-L202

This requires the wheel_build_env configuration:

https://github.com/tox-dev/tox/blob/a2222e98b320467e821cf9444113e9b40ab3ad1b/src/tox/tox_env/python/package.py#L114

That, after a few indirections, calls the default_wheel_tag function that tries to get the base Python environment:

https://github.com/tox-dev/tox/blob/a2222e98b320467e821cf9444113e9b40ab3ad1b/src/tox/tox_env/python/package.py#L82-L91

This goes to VirtualEnv class, here is the relevant section:

https://github.com/tox-dev/tox/blob/a2222e98b320467e821cf9444113e9b40ab3ad1b/src/tox/tox_env/python/virtual_env/api.py#L102-L136

The creator property first gets the env_dir (line 105), that triggers the posargs replacement. However, since this is an inactive environment, this replaces the default value with some "garbage" (to the value that comes after -- that is intented to use in the active environment) since it is not related to this (inactive) environment. And later, when the session_via_cli is called, the environment variables already have the wrongly replaced values that causes the crash.

---

Solution: I have pushed a draft PR https://github.com/tox-dev/tox/pull/2875 to show one approach that solves the problem: the active flag is propagated to default_wheel_tag that returns with the default configuration value, skipping the parts that causes the crash. It works well, but I am not convinced that this is the right solution, it would require more domain-specific knowledge about the internal structure of tox.

For example, we could skip the package environment generation for the inactive environments here:

https://github.com/tox-dev/tox/blob/a2222e98b320467e821cf9444113e9b40ab3ad1b/src/tox/session/env_select.py#L197

This has bigger effect though that I am not sure whether it is expected for inactive environments.

Happy to take further steps based on your inputs.

[masenf]

I'm not a maintainer, but I've been going through the codebase recently.

I personally think the if pkg_name_type is not None and is_active fix in env_select is shorter and more elegant than plumbing is_active through several layers. It does result in a test failure though: packaging environments are not listed when enumerating environments via tox list

[gaborbernat]

For example, we could skip the package environment generation for the inactive environments here:

We cannot do this. We need to discover all packaging environments for active or inactive envs because otherwise, we don't know if a target is valid or is not. Consider:

[testenv]
envlist = a
[testenv:inactive]
wheel_build_env = magic

In this case, if someone does tox -e magic, we should fail hard rather than run testenv as a run env; otherwise tox -e a,magic would work, while tox -e a,magic,inactive would fail. For that reason, at startup time, we must create both active and inactive envs.

[tibortakacs]

I see, it makes sense, and clarifies a lot of things I was not certain about. In this case, I believe the PoC PR is not a good fix either because it does not create the expected packiging environment. (My assumption was that an inactive environment is not used, but your example shows it was a wrong one.)

So, the main challenge is that we must create all environments, but if some environment-related option is extended with posargs, they are also replaced with the passed arguments that are intended for the active env.

Is there any clear UX specification in this regard? If not, we might consider starting with that one.

For example:

• In case of one active tox environment, only the active one is replaced. The other posargs are not replaced, and the default values are used.
• in case of multiple active environments, all of them are replaced. (Alternative: an error is thrown which can be bypassed by the - - replace-all parameter.)

[gaborbernat]

There's no such thing as posargs is intended for active env only. It's passed on for all envs by design, active or inactive. So I'm tempted to close this issue as working as intended and the problem is with your configuration.

[tibortakacs]

I do agree that passing posargs for all environments is okay if no environment is selected with - e. But it not really expected when a specific environment is selected. Posargs can be used in many different contexts, in commands, in options, and it is not expected and even not possible that one posargs is meaningful and working by all environments. When a specific, isolated environment is called, and another one that has no connection with called one fails is not the expected user experience, in my opinion.

[gaborbernat]

I do agree that passing posargs for all environments is okay if no environment is selected with - e. But it not really expected when a specific environment is selected

We'll need to agree to disagree on this. The configuration does not change depending on active or inactive state of an env. That would be confusing. If for some envs the posargs does not make sense do not call it.

[gaborbernat]

One thing we could do here is to move the configuration error raised by virtualenv to a runtime error. A PR in that direction would be accepted.

[tibortakacs]

Okay, it makes sense to improve the error messaging. Do you mean that if an environment is inactive but fails the initialization due to posargs replacement, we throw a more meaningful error message what has happened? Or, do you have another UX idea here?

[gaborbernat]

I was thinking more to ignore errors during setup, but during the execution of that tox environment reraise it. So:

[tox]
minversion = 4.3.1

[testenv:hello]
commands =
    python ./helloworld.py {posargs:World}

[testenv:dev]
envdir = {posargs:venv}
usedevelop = True
commands =
    python ./helloworld.py World

tox -e hello -- x:y will work

but

tox -e dev -- x:y

will still fail.

[tibortakacs]

This is a great idea. I will put a draft PR together soonish.

[tibortakacs]

@gaborbernat I gave a try to fix this issue with small intervention. I am not sure it is easy to hide the virtualenv error message which would be necessary in my opinion to avoid confusion. Do you have any clue whether it is possible to do with any major change in virtualenv?

[gaborbernat]

I don't follow.

[tibortakacs]

The issue is that session_via_cli of virtualenv throws an error if the path of the virtual environment has a : character. If this happens in an inactive, unused tox environment, we do not want to fail (based on your suggestion above). There are two challenges. The first is that session_via_cli throws a SystemExit error which is not the best to catch but doable. The other is that session_via_cli always prints the long error message mentioned in the bug description.

The idea is that I would call session_via_cli always, and if it fails during configuration, we let it go, because it will fail later in the runtime phase if it is used. However, if the error message is always printed, this could be very confusing.

[gaborbernat]

The first is that session_via_cli throws a SystemExit error which is not the best to catch but doable.

Really not an issue, SystemExit is very much catch-able.

The other is that session_via_cli always prints the long error message mentioned in the bug description. The idea is that I would call session_via_cli always, and if it fails during configuration, we let it go

No, we don't. We only let it go if it is an inactive environment. We'd save the exception and reraise it when called at evaluation time of the tox env.

[tibortakacs]

No, we don't. We only let it go if it is an inactive environment. We'd save the exception and reraise it when called at evaluation time of the tox env.

Okay, that would also work. What is the call that you would use as "evaluation time"? Accessing the session property?

However, the error message should be hid which is not, virtualenv always prints it. Do you think we can modify virtualenv here?

[gaborbernat]

No need, can just use https://docs.python.org/3.5/library/contextlib.html#contextlib.redirect_stderr

[tibortakacs]

Thank you for the great hint. Regarding the evaluation call, I thought that I would just let the session property failing. Is that a good track?

[gaborbernat]

Something like that.