posargs with `:` crashes virtualenv #2860

tibortakacs · 2023-01-13T17:43:45Z

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

The text was updated successfully, but these errors were encountered:

gaborbernat · 2023-01-13T17:49:03Z

PR welcome.

tibortakacs · 2023-01-13T19:33:25Z

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

masenf · 2023-01-15T00:06:36Z

~~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 · 2023-01-16T00:26:26Z

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

tibortakacs · 2023-01-16T11:55:06Z

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

tibortakacs · 2023-01-16T11:59:53Z

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 · 2023-01-16T17:44:10Z

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):

tox/src/tox/session/env_select.py

Line 365 in a2222e9

envs, active = self._defined_envs, self._env_name_to_active()

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:

tox/src/tox/session/env_select.py

Lines 197 to 202 in a2222e9

 if pkg_name_type is not None: 

 # build package env and assign it, then register the run environment which can trigger generation 

 # of additional run environments 

 start_package_env_use_counter = self._pkg_env_counter.copy() 

 try: 

 run_env.package_env = self._build_pkg_env(pkg_name_type, name, env_name_to_active)

This requires the wheel_build_env configuration:

tox/src/tox/tox_env/python/package.py

Line 114 in a2222e9

pkg_env = run_env.conf["wheel_build_env"]

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

tox/src/tox/tox_env/python/package.py

Lines 82 to 91 in a2222e9

 def default_wheel_tag(conf: Config, env_name: str | None) -> str: # noqa: U100 

 # https://www.python.org/dev/peps/pep-0427/#file-name-convention 

 # when building wheels we need to ensure that the built package is compatible with the target env 

 # compatibility is documented within https://www.python.org/dev/peps/pep-0427/#file-name-convention 

 # a wheel tag example: {distribution}-{version}(-{build tag})?-{python tag}-{abi tag}-{platform tag}.whl 

 # python only code are often compatible at major level (unless universal wheel in which case both 2/3) 

 # c-extension codes are trickier, but as of today both poetry/setuptools uses pypa/wheels logic 

 # https://github.com/pypa/wheel/blob/master/src/wheel/bdist_wheel.py#L234-L280 

 try: 

 run_py = cast(Python, run_env).base_python

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

tox/src/tox/tox_env/python/virtual_env/api.py

Lines 102 to 136 in a2222e9

 @property 

 def session(self) -> Session: 

 if self._virtualenv_session is None: 

 env_dir = [str(self.env_dir)] 

 env = self.virtualenv_env_vars() 

 self._virtualenv_session = session_via_cli(env_dir, options=None, setup_logging=False, env=env) 

 return self._virtualenv_session 

 def virtualenv_env_vars(self) -> dict[str, str]: 

 env = self.environment_variables.copy() 

 base_python: list[str] = self.conf["base_python"] 

 if "VIRTUALENV_NO_PERIODIC_UPDATE" not in env: 

 env["VIRTUALENV_NO_PERIODIC_UPDATE"] = "True" 

 env["VIRTUALENV_CLEAR"] = "False" 

 env["VIRTUALENV_SYSTEM_SITE_PACKAGES"] = str(self.conf["system_site_packages"]) 

 env["VIRTUALENV_COPIES"] = str(self.conf["always_copy"]) 

 env["VIRTUALENV_DOWNLOAD"] = str(self.conf["download"]) 

 env["VIRTUALENV_PYTHON"] = "\n".join(base_python) 

 if hasattr(self.options, "discover"): 

 env["VIRTUALENV_TRY_FIRST_WITH"] = os.pathsep.join(self.options.discover) 

 return env 

 @property 

 def creator(self) -> Creator: 

 return self.session.creator 

 def create_python_env(self) -> None: 

 self.session.run() 

 def _get_python(self, base_python: list[str]) -> PythonInfo | None: # noqa: U100 

 # the base pythons are injected into the virtualenv_env_vars, so we don't need to use it here 

 try: 

 interpreter = self.creator.interpreter 

 except RuntimeError: # if can't find 

 return None

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 #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:

tox/src/tox/session/env_select.py

Line 197 in a2222e9

if pkg_name_type is not None:

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 · 2023-01-16T19:07:12Z

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 · 2023-01-16T19:14:24Z

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 · 2023-01-16T20:09:42Z

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 · 2023-01-16T21:18:39Z

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 · 2023-01-16T21:31:44Z

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 · 2023-01-16T22:02:42Z

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 · 2023-01-17T15:39:26Z

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 · 2023-01-17T20:39:37Z

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 · 2023-01-17T20:43:26Z

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 · 2023-01-17T20:55:56Z

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

tibortakacs · 2023-02-06T15:37:40Z

@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 · 2023-02-06T15:42:49Z

I don't follow.

tibortakacs · 2023-02-06T16:02:58Z

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 · 2023-02-06T16:06:41Z

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 · 2023-02-06T17:16:40Z

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 · 2023-02-06T17:28:01Z

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

tibortakacs · 2023-02-06T18:42:45Z

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 · 2023-02-06T20:37:48Z

Something like that.

gaborbernat added bug:minor does not affect many people or has no big impact help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. labels Jan 13, 2023

gaborbernat added this to the P-0 milestone Jan 13, 2023

gaborbernat closed this as completed Jan 16, 2023

jugmac00 reopened this Jan 16, 2023

tibortakacs mentioned this issue Jan 16, 2023

PoC to fix posargs issue. #2875

Closed

5 tasks

gaborbernat closed this as completed Jan 16, 2023

gaborbernat reopened this Jan 17, 2023

gaborbernat removed this from the P-0 milestone Jun 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

posargs with `:` crashes virtualenv #2860

posargs with `:` crashes virtualenv #2860

tibortakacs commented Jan 13, 2023 •

edited

Loading

gaborbernat commented Jan 13, 2023

tibortakacs commented Jan 13, 2023

masenf commented Jan 15, 2023 •

edited

Loading

gaborbernat commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

masenf commented Jan 16, 2023

gaborbernat commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

gaborbernat commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

gaborbernat commented Jan 16, 2023 •

edited

Loading

gaborbernat commented Jan 17, 2023

tibortakacs commented Jan 17, 2023

gaborbernat commented Jan 17, 2023

tibortakacs commented Jan 17, 2023

tibortakacs commented Feb 6, 2023

gaborbernat commented Feb 6, 2023

tibortakacs commented Feb 6, 2023

gaborbernat commented Feb 6, 2023

tibortakacs commented Feb 6, 2023

gaborbernat commented Feb 6, 2023

tibortakacs commented Feb 6, 2023

gaborbernat commented Feb 6, 2023

posargs with : crashes virtualenv #2860

posargs with : crashes virtualenv #2860

Comments

tibortakacs commented Jan 13, 2023 • edited Loading

Issue

Environment

Output of running tox

Minimal example

gaborbernat commented Jan 13, 2023

tibortakacs commented Jan 13, 2023

masenf commented Jan 15, 2023 • edited Loading

gaborbernat commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

masenf commented Jan 16, 2023

gaborbernat commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

gaborbernat commented Jan 16, 2023

tibortakacs commented Jan 16, 2023

gaborbernat commented Jan 16, 2023 • edited Loading

gaborbernat commented Jan 17, 2023

tibortakacs commented Jan 17, 2023

gaborbernat commented Jan 17, 2023

tibortakacs commented Jan 17, 2023

tibortakacs commented Feb 6, 2023

gaborbernat commented Feb 6, 2023

tibortakacs commented Feb 6, 2023

gaborbernat commented Feb 6, 2023

tibortakacs commented Feb 6, 2023

gaborbernat commented Feb 6, 2023

tibortakacs commented Feb 6, 2023

gaborbernat commented Feb 6, 2023

posargs with `:` crashes virtualenv #2860

posargs with `:` crashes virtualenv #2860

tibortakacs commented Jan 13, 2023 •

edited

Loading

masenf commented Jan 15, 2023 •

edited

Loading

gaborbernat commented Jan 16, 2023 •

edited

Loading