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 v4 does not consistently require to escape the # #2893

Closed
lmazuel opened this issue Jan 25, 2023 · 4 comments
Closed

Tox v4 does not consistently require to escape the # #2893

lmazuel opened this issue Jan 25, 2023 · 4 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

@lmazuel
Copy link

lmazuel commented Jan 25, 2023

Issue

Using this in deps:

pylint-guidelines-checker @ git+https://github.com/Azure/azure-sdk-for-python#subdirectory=scripts/pylint_custom_plugin

works like a charm, but I was assuming I should escape based on the tox v4 updating doc to:

pylint-guidelines-checker @ git+https://github.com/Azure/azure-sdk-for-python\#subdirectory=scripts/pylint_custom_plugin

But this one actually crashes tox. My CI unfortunately didn't kept the log, but the message is something like cannot clone https://github.com/Azure/azure-sdk-for-python\ (note the \ at the end). If that matters, I can re-crash my CI on purpose to get the real error message.

I expect that when the tox updating doc says "The hash sign (#) now always acts as comment within tox.ini", then it's the case.

Environment

Provide at least:

  • OS: Linux or Windows
  • pip list of the host Python where tox is installed:
Jinja2-3.1.2 MarkupSafe-2.1.2 astroid-2.13.3 attrs-22.2.0 autorest-0.1.0 black-22.12.0 cachetools-5.3.0 chardet-5.1.0 click-8.1.3 colorama-0.4.6 dill-0.3.6 distlib-0.3.6 docutils-0.19 exceptiongroup-1.1.0 filelock-3.9.0 importlib-metadata-6.0.0 iniconfig-2.0.0 invoke-2.0.0 isort-5.11.4 json-rpc-1.14.0 lazy-object-proxy-1.9.0 m2r2-0.3.3 mccabe-0.7.0 mistune-0.8.4 mypy-0.991 mypy-extensions-0.4.3 nodeenv-1.7.0 packaging-23.0 pathspec-0.10.3 platformdirs-2.6.2 pluggy-1.0.0 ptvsd-4.3.2 pylint-2.15.10 pyproject-api-1.5.0 pyright-1.1.287 pytest-7.2.1 pyyaml-6.0 tomli-2.0.1 tomlkit-0.11.6 tox-4.3.5 typed-ast-1.5.4 types-PyYAML-6.0.12.3 typing-extensions-4.4.0 virtualenv-20.17.1 wrapt-1.14.1 zipp-3.11.0

I'm not blocked since I removed the backslah and it works.

@gaborbernat gaborbernat added the bug:minor does not affect many people or has no big impact label Jan 25, 2023
@gaborbernat
Copy link
Member

If someone wants to open a PR this would likely be here https://github.com/tox-dev/tox/blob/main/src/tox/config/loader/ini/__init__.py

@gaborbernat gaborbernat added the help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. label Jan 25, 2023
@masenf
Copy link
Collaborator

masenf commented Jan 25, 2023

I've had my eye on fixing #2365, which after digging in a bit, I've determined there's a problem when combining the new substitutions parser (4.3.x) with the shlex used in command arg splitting. Specifically, the double backslash gets handled during substitution parsing and thus shlex gets a string with a single backslash, even when the user expected that to be an escaped backslash.

All that to say, I think at least one problem here is that for commands the substitution parser should leave backslashes alone, but for other strings, then it should follow the current escaping rules.

I know one of the problems with my last PR for #2365 was related to handling the comment escapes. Definitely more test cases are needed in this area.

This isn't to discourage anyone from taking a stab at a fix, but be wary, since we are definitely missing some test cases in this area, and currently the command parser behaves differently on windows vs Linux (and we don't have tests for those differences)

@masenf
Copy link
Collaborator

masenf commented Jan 26, 2023

@lmazuel do you have a minimal reproducer?

I tried running with this

[testenv]
deps =
    pylint-guidelines-checker @ git+https://github.com/Azure/azure-sdk-for-python\#subdirectory=scripts/pylint_custom_plugin
commands = pip freeze

On windows and linux with tox-4.3.5 and tox-4.4.2, I don't seem to reproduce the issue.

@lmazuel
Copy link
Author

lmazuel commented May 8, 2023

@masenf sorry for the late answer, I paused my migration tox v4 for now. I can only tell you at this point this was a Azure DevOps job, likely on Linux that was showcasing this issue. I should have more time soon (worst this summer), to complete the tox v4 migration and retry it.

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

No branches or pull requests

3 participants