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

shlexing happens _after_ substitution causing space paths to be multiple arguments #924

Closed
asottile opened this issue Jul 24, 2018 · 4 comments
Labels
bug:minor does not affect many people or has no big impact feature:change something exists already but should behave differently help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.

Comments

@asottile
Copy link
Contributor

[tox]
skipsdist = true

[testenv]
commands =
    python -c 'import sys; print(sys.argv[1:])' -- {toxinidir}

In /private/tmp/t t:

$ tox -e py37 -qq
['--', '/private/tmp/t', 't']

Expected:

$ tox -e py37 -qq
['--', '/private/tmp/t t']

If I quote {toxinidir} it works though:

[tox]
skipsdist = true

[testenv]
commands =
    python -c 'import sys; print(sys.argv[1:])' -- '{toxinidir}'
$ tox -e py37 -qq
['--', '/private/tmp/t t']

Intuitively, I think {toxinidir} should probably be considered a specific argument, though if people are already depending on this behaviour it would potentially break their configuration.

Related, if we treat it as an indivisible argument, there would have to probably be some special handling for things like {toxinidir}/.coveragerc 🤔

@asottile asottile added bug:minor does not affect many people or has no big impact feature:change something exists already but should behave differently labels Jul 24, 2018
@asottile
Copy link
Contributor Author

CC @eirnym (originally reported #121 (comment))

@gaborbernat gaborbernat added the help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. label May 2, 2019
@jayvdb
Copy link

jayvdb commented Oct 19, 2020

Confirmed it is still a problem, and is the same underlying problem as #763 .
i.e. using {envpython} with a path containing space fails in the same way.
While using # in path does fail in more confusing ways, the manual quoting workaround used in this issue, like putting '{toxinidir}' in the .ini file, also fixes the problem with #.

@jayvdb
Copy link

jayvdb commented Oct 19, 2020

There is one difference to #763 that makes spaces 'harder', the word splitter in config/__init__.py:CommandParser.words adds another level of whitespace handling in addition to the shlex'ing.

jayvdb added a commit to jayvdb/tox that referenced this issue Oct 24, 2020
Paths were already stored inside SectionReader._subs as
path objects, however they are turned into strings when they
go through Replacer, and various transforms occur on these
paths, for example breaking paths containing '#' and ' '.

This change defaults to path objects being retained through the
Replacer, and path segments being joined together using path object
joining semantics.

This mode can be disabled for settings of type `path` by wrapping
the value in quotes, and disabled for other values by disabling
new global setting literal_paths.

Fixes tox-dev#763
and tox-dev#924
jayvdb added a commit to jayvdb/tox that referenced this issue Oct 24, 2020
Paths were already stored inside SectionReader._subs as
path objects, however they are turned into strings when they
go through Replacer, and various transforms occur on these
paths, for example breaking paths containing '#' and ' '.

This change defaults to path objects being retained through the
Replacer, and path segments being joined together using path object
joining semantics.

This mode can be disabled for settings of type `path` by wrapping
the value in quotes, and disabled for other values by disabling
new global setting literal_paths.

Fixes tox-dev#763
and tox-dev#924
@gaborbernat gaborbernat added this to the 4.0 milestone Jan 13, 2022
@gaborbernat gaborbernat modified the milestones: 4.0, 4.1 Nov 20, 2022
@masenf
Copy link
Collaborator

masenf commented Jan 19, 2023

Requiring users to quote substitutions is essentially how shell scripting has handled this problem: https://www.shellcheck.net/wiki/SC2086

Not saying shell scripting is the epitome of style and safety, but keeping the substitution expansion separate from any sort quoting or splitting makes the configuration language more flexible and composable (albeit adding some footguns to watch for)

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 feature:change something exists already but should behave differently 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

4 participants