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

Backslash-ed curly bracket in settings #1708

Closed
jayvdb opened this issue Oct 22, 2020 · 4 comments
Closed

Backslash-ed curly bracket in settings #1708

jayvdb opened this issue Oct 22, 2020 · 4 comments
Labels
bug:normal affects many people or has quite an impact

Comments

@jayvdb
Copy link

jayvdb commented Oct 22, 2020

config.rst states

You can escape curly braces with the \ character if you need them, for example:
commands = echo "\{posargs\}" = {posargs}

A minor note, the = {posargs} should be moved so that it is clearer that it is output, and not part of the commands.

The docs don't indicate where this is possible, which led to #1656 and #1690 to get it working in setenv and in the subprocess runtime environment variables.

And there is one very obvious omission from the docs. Any time an escaping character is introduced, there needs to be a way to escape the escape character. The typical way to escape \ is \\, and this is de-facto implemented because of use of shlex. #1691 added the first test for \\{ escaping the \ to become literal \{. \\ should be mentioned in the docs. However, \\ is not implemented consistently; all of the non-shlex parsing ignores it.

e.g. commands = echo "\\{posargs}" should be \ followed by substitution of {posargs}. Instead it results in literal \{posargs}.

An additional complication is that \ is valid in many contexts, e.g.

[testenv:env\bar]
foo\ = bar
\foo = bar
setenv =
  \ENV_VAR = {[testenv:env\bar]\foo}
  ENV_VAR\ = {[testenv:env\bar]foo\}
commands = echo '{}' {env:\ENV_VAR} {env:ENV_VAR\}

\ is valid in paths on Linux, so the above works fine on Linux. The above would probably have strange results on Windows as \ is a path separator. I think and hope it would be reasonable to prohibit \ in envname and any paths on all platforms.

#1698 goes some of the way to handle paths a bit more intelligently, but there are some very low level problems, and there is at least one case where better docs can help. Because the implementation uses shlex, the behaviour of \ depends on whether it is within quotes or not.

e.g. the following will put literal \{ into the setting, and almost every other setting is based on that setting, so that \{ will appear in many other paths.

toxworkdir = {toxinidir}/.tox\{dir

It also causes subprocess env to contain

'TOX_WORK_DIR': '/path/to/tox{foo/tests/.tox\\{dir'
'VIRTUAL_ENV': '/path/to/tox{foo/tests/.tox\\{dir/bracketpath2'

however the \ is unnecessary there. This has the same effect:

toxworkdir = {toxinidir}/.tox{dir

and this also works sometimes

toxworkdir = {toxinidir}/.tox}dir

but ^ that fails if the user is in directory ~/tmp/tox{foo

ConfigError: substitution key 'foo/tests/.toxdir' not found

The more common/obvious failure is something like

toxworkdir = {toxinidir}/.tox{dir}

That causes

ConfigError: substitution key 'dir' not found

So we can improve the docs to state that \ is only needed to escape balanced curly brackets. i.e. when both { and } are literals in the path.

Most escaping problems wrt paths can be fixed if getpath does unescaping of path elements before joining path elements together. I have a fix for this.

And to fix

toxinidir = tox{foo
toxworkdir = {toxinidir}/.tox}dir

path settings need to be escaped before they are pushed into the substitution engine. This is the bit I havent started yet.

Then there is still the issue of literal curly brackets not in paths, especially inside and outside of quotes.

There is another way forward, which is to not promote backslash escaping. The benefit would be for the top level ini syntax to not be reliant on the low level shlex parsing.

This can be done at two levels:

  1. Instead of encouraging \\ to escape \, we could introduce {\}. On its face, this is easy to implement.

  2. instead of relying on \{ and \}, we can use a balanced parsing mechanism like {{} and {}}, which should be safe because {} is currently unspecified (it actually returns literal :). I havent tried this yet, so it might have some serious stumbling block.

I havent fully thought through those, and am not advocating them, but I have frequently done parsers, and been finding and fixing escaping bugs in tox, and sticking with \\, \{ and \} has problems worth looking for alternatives, especially for \\ which isnt documented yet.

@jayvdb jayvdb added the bug:normal affects many people or has quite an impact label Oct 22, 2020
@jayvdb
Copy link
Author

jayvdb commented Oct 22, 2020

#1709 includes what should be an acceptable implementation of {\} which I think stands alone and complements \{ allowing {\}{..}, and a partial implementation of {{} and {}} which I am less convinced about and need discussion before I would bother completing the implementation.

@jayvdb
Copy link
Author

jayvdb commented Oct 23, 2020

#1710 provides an almost complete implementation of \\ escaping, however there is still cases which fail because \ is valid throughout section headers, key names and even paths on Linux, as noted at #1710 (comment)

IMO rejecting \ in path segments (illegal on Windows, legal on unix), section headers and key names is worth the breakage.

If we dont reject \ , and we implement both {\} and \\, a lot of tests are needed to ensure they work together sanely in most scenarios, we can document the ways each work, and when they dont work, and the users should be able to combined them both to achieve anything. Not ideal, but pragmatic.

@hexagonrecursion
Copy link
Contributor

One more corner case:

[tox]
isolated_build = true
envlist = py3

[testenv]
commands = python -c 'import sys; print(sys.argv)' "\{" []

Run:

tox -- foo

Expected:

['-c', '{', 'foo']

Got:

['-c', '{', '[]']

@gaborbernat
Copy link
Member

❯ tox4 -- foo
py3: commands[0]> python -c 'import sys; print(sys.argv)' '{' foo
['-c', '{', 'foo']
  py3: OK (0.05=setup[0.03]+cmd[0.02] seconds)
  congratulations :) (0.08 seconds)

Has been fixed with v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:normal affects many people or has quite an impact
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants