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

Tox not stripping ticks from paths in commands anymore #2635

Closed
johannesloibl opened this issue Dec 8, 2022 · 11 comments
Closed

Tox not stripping ticks from paths in commands anymore #2635

johannesloibl opened this issue Dec 8, 2022 · 11 comments
Labels
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.

Comments

@johannesloibl
Copy link

Issue

I have a tox test env which calls pytest like this:

commands =
    python -Wdefault -b -m pytest tests/unit \
        --in_memory=False \
        --basetemp="tests/tmp/{envname}" \
        --junitxml="tests/tmp/results/junit-out_memory-{envname}.xml" \
        --junit-prefix="foo-{envname}"

As you can see the paths are wrapped in ticks.
When running the tox env via tox<4.0.0, apparently the ticks get stripped before passing it to the pytest subprocess so there is NO problem.
Here a print of the arguments inside .tox/py39/Lib/site-packages/_pytest/config/__init__.py:306:
ARGS: ['tests/unit', '--in_memory=False', '--basetemp=tests/tmp/py39', '--junitxml=tests/tmp/results/junit-out_memory-py39.xml', '--junit-prefix=DLH5-py-py39']

In tox 4.0.2 however the ticks are not stripped anymore, which results in following arguments passed to pytest:
ARGS: ['tests/unit', '--in_memory=False', '--basetemp="tests/tmp/py39"', '--junitxml="tests/tmp/results/junit-out_memory-py39.xml"', '--junit-prefix="DLH5-py-py39"']

Then pytest fails with following stderr:

py39: install_deps> python -I -m pip install junitparser pytest
.pkg: _optional_hooks> python C:\Python39\lib\site-packages\pyproject_api\_backend.py True setuptools.build_meta
.pkg: get_requires_for_build_editable> python C:\Python39\lib\site-packages\pyproject_api\_backend.py True setuptools.build_meta
.pkg: get_requires_for_build_sdist> python C:\Python39\lib\site-packages\pyproject_api\_backend.py True setuptools.build_meta
.pkg: build_wheel> python C:\Python39\lib\site-packages\pyproject_api\_backend.py True setuptools.build_meta
.pkg: build_sdist> python C:\Python39\lib\site-packages\pyproject_api\_backend.py True setuptools.build_meta
py39: install_package_deps> python -I -m pip install h5py hdf5plugin==2.3.2 numpy pandas psutil scipy wavewatson==1.1.*
py39: install_package> python -I -m pip install --force-reinstall --no-deps C:\git\dlh5-py\.tox\.tmp\package\29\dlh5-py-1.9.0.tar.gz
py39: commands_pre[0]> python -c "import sys; print('Used Python: Python %s on %s @ %s' % (sys.version, sys.platform, sys.executable))"
Used Python: Python 3.9.13 (tags/v3.9.13:6de2ca5, May 17 2022, 16:36:42) [MSC v.1929 64 bit (AMD64)] on win32 @ C:\git\dlh5-py\.tox\py39\Scripts\python.EXE
py39: commands_pre[1]> python -c "from pathlib import Path;Path('tests/tmp/py39').mkdir(parents=True, exist_ok=True)"
py39: commands[0]> python -Wdefault -b -m pytest tests/unit --in_memory=False --basetemp=\"tests/tmp/py39\" --junitxml=\"tests/tmp/results/junit-out_memory-py39.xml\" --junit-prefix=\"DLH5-py-py39\"
ARGS:  ['tests/unit', '--in_memory=False', '--basetemp="tests/tmp/py39"', '--junitxml="tests/tmp/results/junit-out_memory-py39.xml"', '--junit-prefix="DLH5-py-py39"']
Traceback (most recent call last):
  File "C:\Python39\lib\runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Python39\lib\runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\pytest\__main__.py", line 5, in <module>
    raise SystemExit(pytest.console_main())
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\config\__init__.py", line 190, in console_main
    code = main()
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\config\__init__.py", line 148, in main
    config = _prepareconfig(args, plugins)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\config\__init__.py", line 329, in _prepareconfig
    config = pluginmanager.hook.pytest_cmdline_parse(
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\pluggy\_hooks.py", line 265, in __call__
    return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\pluggy\_manager.py", line 80, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\pluggy\_callers.py", line 55, in _multicall
    gen.send(outcome)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\helpconfig.py", line 103, in pytest_cmdline_parse
    config: Config = outcome.get_result()
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\pluggy\_result.py", line 60, in get_result
    raise ex[1].with_traceback(ex[2])
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\pluggy\_callers.py", line 39, in _multicall
    res = hook_impl.function(*args)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\config\__init__.py", line 1058, in pytest_cmdline_parse
    self.parse(args)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\config\__init__.py", line 1346, in parse
    self._preparse(args, addopts=addopts)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\config\__init__.py", line 1214, in _preparse
    self._initini(args)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\config\__init__.py", line 1130, in _initini
    ns, unknown_args = self._parser.parse_known_and_unknown_args(
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\config\argparsing.py", line 173, in parse_known_and_unknown_args
    return optparser.parse_known_args(strargs, namespace=namespace)
  File "C:\Python39\lib\argparse.py", line 1858, in parse_known_args
    namespace, args = self._parse_known_args(args, namespace)
  File "C:\Python39\lib\argparse.py", line 2067, in _parse_known_args
    start_index = consume_optional(start_index)
  File "C:\Python39\lib\argparse.py", line 2007, in consume_optional
    take_action(action, args, option_string)
  File "C:\Python39\lib\argparse.py", line 1919, in take_action
    argument_values = self._get_values(action, argument_strings)
  File "C:\Python39\lib\argparse.py", line 2450, in _get_values
    value = self._get_value(action, arg_string)
  File "C:\Python39\lib\argparse.py", line 2483, in _get_value
    result = type_func(arg_string)
  File "C:\git\dlh5-py\.tox\py39\lib\site-packages\_pytest\main.py", line 251, in validate_basetemp
    if is_ancestor(Path.cwd().resolve(), Path(path).resolve()):
  File "C:\Python39\lib\pathlib.py", line 1215, in resolve
    s = self._flavour.resolve(self, strict=strict)
  File "C:\Python39\lib\pathlib.py", line 215, in resolve
    s = self._ext_to_normal(_getfinalpathname(s))
OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect: '"tests\\tmp\\py39"'

The pytest version in both cases is the same 7.2.0.
Since tox 4 is claimed to be backwards compatible I would assume this is a bug in tox.

Environment

  • OS: Win10 64-bit
@jugmac00
Copy link
Member

jugmac00 commented Dec 8, 2022

Thanks for your report.

What happens when you leave out the quotes?

Having a look at pytest's documentation, they do not propose using them:
https://docs.pytest.org/en/6.2.x/usage.html#creating-junitxml-format-files

@johannesloibl
Copy link
Author

Just checked on the console with minimal options. pytest is working with and without ticks:
pytest tests/unit --in_memory=False --basetemp=tests/tmp/py39-dev
pytest tests/unit --in_memory=False --basetemp="tests/tmp/py39-dev"

But when i execute this:
pytest tests/unit --in_memory=False --basetemp=\"tests/tmp/py39-dev\"
i get the same error as i get with tox 4:
OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect: '"tests\\tmp\\py39-dev"'

@gaborbernat gaborbernat added this to the 4.0.x milestone Dec 8, 2022
@gaborbernat 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 Dec 8, 2022
@johannesloibl
Copy link
Author

image
Apparently tox escapes those ticks in contrast to tox<4.x.

@gaborbernat
Copy link
Member

This is one of those where you might be better off just removing the quotes, tox will take care not to need them 🤔

@johannesloibl
Copy link
Author

But ticks are needed if there's spaces in the file names, that's their whole purpose. tox is just not allowed to additionally escape them.

@johannesloibl
Copy link
Author

johannesloibl commented Dec 8, 2022

project_to_reproduce.zip

[tox]
requires =
    tox>4
envlist =
    must_pass_1
    must_pass_2
    must_pass_3
    must_fail


[testenv]
basepython = python3
passenv =
    *
skip_install = true
deps =
    pytest


[testenv:must_pass_1]
commands =
    pytest tests --basetemp=tests/tmp/{envname}


[testenv:must_pass_2]
commands =
    pytest tests --basetemp="tests/tmp/{envname}"


[testenv:must_pass_3]
commands =
    pytest tests --basetemp="tests/tmp with space/{envname}"


[testenv:must_fail]
commands =
    pytest tests --basetemp=tests/tmp with space/{envname}

image

@johannesloibl
Copy link
Author

johannesloibl commented Dec 12, 2022

I see that tox.config.loader.str_convert.StrConvert.to_command is doing a simple whitespace splitting, which also tears appart commands like --basetemp="tests/tmp with space/{envname}" into multiple parts.
Later in tox.execute.request.shell_cmd, subprocess.list2commandline is used to join the commands, making 'pytest tests --basetemp=\\"tests/tmp with space/must_pass_3\\"' out of it.
So IMHO either taking the command from the config file as a complete string for further processing or use a simple join in favor of list2commandline.

@stephane-caron
Copy link

Another reason why ticks were/would be nice is that they allow writing commands in tox.ini the same way one would write them in a shell. Breaking that transparency will likely cascade in confusing behaviors.

Example

For example, in the reproducer for #2778 one way to fix the issue with tox >=4 is to remove the quotes from a coverage report command. But then here is what happens when running that command without quotes in zsh:

src:tox-windows-issue$ coverage report --include=tox_windows_issue/*
zsh: no matches found: --include=tox_windows_issue/*

That is, in zsh we'd like the quotes (to avoid the shell trying to expand the wildcard):

src:tox-windows-issue$ coverage report --include="tox_windows_issue/*"
Name                            Stmts   Miss  Cover
---------------------------------------------------
tox_windows_issue/__init__.py       5      0   100%
---------------------------------------------------
TOTAL                               5      0   100%

There is of course another way to escape the wildcard with a backslash:

src:tox-windows-issue$ coverage report --include=tox_windows_issue/\*
Name                            Stmts   Miss  Cover
---------------------------------------------------
tox_windows_issue/__init__.py       5      0   100%
---------------------------------------------------
TOTAL                               5      0   100%

That one works both in zsh and in the aforementioned reproducer (this job). But then tox becomes harder to learn/more confusing, as it would need to document for all existing and future users why they can execute commands one way and not the other.

@gaborbernat
Copy link
Member

You're free to put in a PR to fix this 👍

@stephane-caron
Copy link

stephane-caron commented Dec 30, 2022

I wish there were room for it in my roadmap, but I must pass on that opportunity to graduate from tox user to contributor 😃

I hope the feedback will help though! Either for project maintainers, or future travelers who run into similar issues (e.g. "why did my CI start to break?") and can find a documented path to the quote-stripping change.

masenf added a commit to masenf/tox that referenced this issue Jan 18, 2023
for less surprises, stick to shlex POSIX mode, regardless of the running platform.

on win32 platforms, do not use backslash escape to make it easier to work with
windows style paths that use backslash as the separator

fix tox-dev#2635
masenf added a commit to masenf/tox that referenced this issue Jan 19, 2023
for less surprises, stick to shlex POSIX mode, regardless of the running platform.

on win32 platforms, explicitly convert a single backslash to double backslash,
unless the following character is "special"

fix tox-dev#2635

str_convert fixup
masenf added a commit to masenf/tox that referenced this issue Jan 19, 2023
for less surprises, stick to shlex POSIX mode, regardless of the running platform.

on win32 platforms, explicitly convert a single backslash to double backslash,
unless the following character is "special"

fix tox-dev#2635

str_convert fixup
masenf added a commit to masenf/tox that referenced this issue Jan 25, 2023
for less surprises, stick to shlex POSIX mode, regardless of the running platform.

on win32 platforms, explicitly convert a single backslash to double backslash,
unless the following character is "special"

fix tox-dev#2635

str_convert fixup

str_convert: always use shlex POSIX mode

for less surprises, stick to shlex POSIX mode, regardless of the running platform.

on win32 platforms, explicitly convert a single backslash to double backslash,
unless the following character is "special"

fix tox-dev#2635

str_convert fixup
@masenf
Copy link
Collaborator

masenf commented Jan 25, 2023

Fixed in tox-4.4.0

Repro from #2635 (comment)

must_pass_1: remove tox env folder C:\Users\masen\code\.tox\must_pass_1
must_pass_1: install_deps> python -I -m pip install pytest
must_pass_1: commands[0]> pytest tests --basetemp=tests/tmp/must_pass_1
================================================= test session starts =================================================
platform win32 -- Python 3.11.1, pytest-7.2.1, pluggy-1.0.0
cachedir: .tox\must_pass_1\.pytest_cache
rootdir: C:\Users\masen\code
collected 1 item

tests\unit\test_foo.py .                                                                                         [100%]

================================================== 1 passed in 0.01s ==================================================
must_pass_1: OK ✔ in 7.25 seconds
must_pass_2: remove tox env folder C:\Users\masen\code\.tox\must_pass_2
must_pass_2: install_deps> python -I -m pip install pytest
must_pass_2: commands[0]> pytest tests --basetemp=tests/tmp/must_pass_2
================================================= test session starts =================================================
platform win32 -- Python 3.11.1, pytest-7.2.1, pluggy-1.0.0
cachedir: .tox\must_pass_2\.pytest_cache
rootdir: C:\Users\masen\code
collected 1 item

tests\unit\test_foo.py .                                                                                         [100%]

================================================== 1 passed in 0.01s ==================================================
must_pass_2: OK ✔ in 4.91 seconds
must_pass_3: remove tox env folder C:\Users\masen\code\.tox\must_pass_3
must_pass_3: install_deps> python -I -m pip install pytest
must_pass_3: commands[0]> pytest tests "--basetemp=tests/tmp with space/must_pass_3"
================================================= test session starts =================================================
platform win32 -- Python 3.11.1, pytest-7.2.1, pluggy-1.0.0
cachedir: .tox\must_pass_3\.pytest_cache
rootdir: C:\Users\masen\code
collected 1 item

tests\unit\test_foo.py .                                                                                         [100%]

================================================== 1 passed in 0.01s ==================================================
must_fail: remove tox env folder C:\Users\masen\code\.tox\must_fail
must_pass_3: OK ✔ in 4.92 seconds
must_fail: install_deps> python -I -m pip install pytest
must_fail: commands[0]> pytest tests --basetemp=tests/tmp with space/must_fail
================================================= test session starts =================================================
platform win32 -- Python 3.11.1, pytest-7.2.1, pluggy-1.0.0
cachedir: .tox\must_fail\.pytest_cache
rootdir: C:\Users\masen\code
collected 0 items

================================================ no tests ran in 0.00s ================================================
ERROR: file or directory not found: with

must_fail: exit 4 (1.61 seconds) C:\Users\masen\code> pytest tests --basetemp=tests/tmp with space/must_fail pid=15532
  must_pass_1: OK (7.25=setup[6.03]+cmd[1.22] seconds)
  must_pass_2: OK (4.91=setup[4.03]+cmd[0.88] seconds)
  must_pass_3: OK (4.92=setup[4.03]+cmd[0.89] seconds)
  must_fail: FAIL code 4 (5.64=setup[4.03]+cmd[1.61] seconds)
  evaluation failed :( (22.81 seconds)

@masenf masenf closed this as completed Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants