-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
posargs configerror #150
Comments
Original comment by @chmouel cool! thanks for fixing that. This has been a major painpoint for us |
Original comment by @hpk42 fix issue150: parse {posargs} more like we used to do it pre 1.7.0. → <> |
Original comment by @chr0n1x Any progress on this? I'm working with the OpenStack projects ;) |
Original comment by @msabramo Yeah most
So it probably doesn't matter a whole lot to most people. But OpenStack has a bazillion |
Original comment by @hpk42 @offbyone do i understand correctly that this would make the open stack use case @msabramo i tend to agree that if we don't find a solution soon, we should just revert the behaviour and aim for something better in 1.8. I don't think it's going to break things too much because not too many people do wild things with posargs from my experience. |
Original comment by @offbyone I have a proposed solution that should address the main concerns: pseudocode:
Proposed result matrix:
|
Original comment by @offbyone So, here's a summary of behaviour since 1.4. Details follow, but : 1.4-1.6
1.7
A more complex example, with mixed quoting:tox-args.ini
This setup exists:
tox 1.6tox -c tox-args.ini -- 'a file' a-fileexpectations
actual
tox -c tox-args.ini -- "'a file' a-file"expectations
actual
tox -c tox-args.ini -- a\ file a-fileexpectations
actual
tox -c tox-args.ini -- 'a\ file a-file'expectations
actual
tox 1.7tox -c tox-args.ini -- 'a file' a-fileexpectations
actual
tox -c tox-args.ini -- "'a file' a-file"expectations
actual
tox -c tox-args.ini -- a\ file a-fileexpectations
actual
tox -c tox-args.ini -- 'a\ file a-file'expectations
actual
|
Original comment by @msabramo Here's another example that illustrates an earlier point that @cboylan made using the make# Makefile.ls_example
POSARGS = A file with multiple words
test:
ls -l '$(POSARGS)'
echo
ls -l $(POSARGS)
The point here is that tox # ls_example.ini
[testenv]
whitelist_externals = ls
commands =
ls -l '{posargs}'
ls -l {posargs}
Here, the quoted form and non-quoted form do exactly the same thing, so expressiveness has been lost because the substitution eats the quotes. I personally would rather have See:
So I would say that it would be good to have |
Original comment by @msabramo @hpk42 mentioned bash on IRC as a possible model to follow since tox is a command executor. It just occurred to me that perhaps For example, take this # Makefile
POSARGS = --failing --parallel
test:
python setup.py test --slowest --testr-args='$(POSARGS)'
I think tox should function the same way but it doesn't. Note below that the quotes are stripped out: # testr.ini
[testenv]
commands =
python setup.py test --slowest --testr-args '{posargs}'
Two problems in
|
Original comment by @msabramo Personally, I think the most intuitive behavior (how I expected it to work and was surprised when it didn't work this way) was to do simple, dumb string substitution -- I gravitate towards simple string substitution with no quote eating, along the lines of what @cboylan is suggesting. To me that is easy to implement without bugs and understandable. Trying to emulate bash or do anything complex I think is going to be fraught with peril and I don't see why it's necessary. |
Original comment by @cboylan @hpk42 Something like {posargs-string} would restore the old behavior if I rewrite all of my tox.ini files. Ideally I wouldn't need to do that, but if tox is committed to not introducing a second backward incompatible change in as many releases I will have to live with that. @offbyone I don't think it is hard if you don't overthink it. The variable substitutions should be regular (IMO all of them should be not just special ones) then allow shlex to parse the resulting string. This makes the behavior consistent and predictable. I'm not sure I have time to update my pull request to support {posargs-string} prior to Friday (I think I would need to rewrite much of it to deal with the special case instead of the current behavior of that PR which is to apply the same rules to every variable substitution). |
Original comment by @offbyone That's the thing; the handling of whitespace really depends on context. In a shell command, escaping can take several forms -- single-quoting, double-quoting, -escaping -- to say nothing of other shells (I assume that Windows shells will do something different). How big of a change is someone willing to write in here? Because to handle the various whitespace scenarios we really need to have support for these: Given the command
Given the command
The takeaway, shell quoting is hard. I don't think a one-size-fits-all solution will work. |
Original comment by @hpk42 Hum, good point regarding escaping with |
Original comment by @offbyone It feels clunky, but it's better than making the string usage impossible. It's a shame we aren't using a template engine in here :) We could use Jinja2-style pipe syntax:
That may be a wee bit too general purpose. The point remains, though; posargs-string will do ... what, exactly, with arguments that contain spaces? Will it quote them? Escape them? Ignore them? |
Original comment by @hpk42 I have read @cboylan 's PR (also thanks for digging up the commit context) and the issue comments here but am not sure at the moment how to best resolve this issue. We need a clear and predictable behaviour of substitutions and the fact that we are treating
|
Original comment by @cboylan https://bitbucket.org/hpk42/tox/pull-request/85/fix-command-expansion-and-parsing/diff fixes this bug. After reading the changes that introduced the bug (https://bitbucket.org/hpk42/tox/commits/88a503e7e5aefe509be8f6c3108a84baa20c3c26 and https://bitbucket.org/hpk42/tox/commits/f15199a9ec78e052b9f9513a34e0e5768e8affdd) I am further convinced that this was not intended behavior and is instead an unintentional side effect. I wrote a fix to illustrate that. |
Original comment by jesusaurus The posargs substitution should be regular, not context-sensitive. Given |
Original comment by @offbyone While I concede that there is not a sufficient explanation of the quoting behaviour there, nothing in that disagrees with what I said. Anyway, the upshot of it is, a behaviour changed here. I personally think the old way was a bug, and it got inadvertently permitted by parsing logic that was fixed in 1.7.x. We could circle around on this all week. Right now, I'd suggest that we document the current (As of 1.7) quoting behaviour, and consider ways to extend the concept of substitution to permit the desired use case. |
Original comment by @cboylan I have to disagree. The tox docs explicitly document {posargs} as a substitution, http://tox.readthedocs.org/en/latest/config.html#substitutions-for-positional-arguments-in-commands. This is consistent with the old behavior but not your previous statement. |
Original comment by @offbyone I'd consider the second version to be a bug. Certainly, it violates my expectations based on what I wrote, and it also breaks the contract of the code that implements posargs. Which is not to say that the behaviour in the case of standalone single-quoted posargs could not be considered, but that's not what the {posargs} placeholder is meant for; the command ls a list, and the {posargs} marker denotes the point in that list where the list of posargs entries from the command line should be inserted. Not substituted in a string. |
Original comment by @cboylan Given Given 1.6.1 did the correct thing here and gave you ls -l 'foo bar baz' in both cases. 1.7 does not. |
Original comment by @cboylan Wouldn't an inplace insertion preserve any quotes? And so I understand, are we asserting that the old behavior was a bug that we were exploiting? |
Original comment by @offbyone Right. posargs is not supposed to be a quoted insertion into the commands I think the gist of this is that what you are asking is something that the |
Original comment by @offbyone Clark, your workaround example should result in the shell equivalent of ls -l 'foo bar baz', right? so: tox config:
And given the CLI:
That should do what you expect; run |
Original comment by @offbyone Right. posargs is not supposed to be a quoted insertion into the commands in the tox command list, it's supposed to be an in-place insertion into the command. The distinction is perhaps a subtle one, but important. The placeholder is there to indicate the point in the list of command arguments into which we should insert the list of additional command line positional arguments before constructing the complete command. I think the gist of this is that what you are asking is something that the component you are using is not designed to do. |
Original comment by @cboylan After more fiddling I have discovered by workaround isn't quite sufficient in cases where the option being modified by posargs requires arguments. In that case users of tox would be required to pass posargs on every command which is not user friendly. The only other option I can think of is to pass the entire string through as posargs. What is the possibility of updating the posargs parser to stop eating quotes? It really should preserve them. |
Original comment by @cboylan So the '=' thing is an easy work around. We can use -t '{posargs}' instead. But the handling of single quotes around {posargs} seems to have changed too... Using a different example command, say |
Original comment by @d0ugal We are also having this issue with this config - https://github.com/openstack/python-tuskarclient/blob/master/tox.ini Broken on 1.7, fine in 1.6.1 |
Original comment by @offbyone My read on this is that the CommandParser could probably do a better job of splitting out words. The contract of {posargs} is that it be a single-word replacement; in this instance, it's part of a "word" consisting of --testr-args='{posargs}', which is not going to work as written. We might be able to support equality notation there as well, by making some assumptions about the structure of CLI arguments, but I worry that those heuristics may bite us in the end. Basically, short answer: Changing the argument parser to make this work may have unexpected consequences; I'm leery of making that change unilaterally. |
Original comment by @offbyone I am not really sure why that isn't working, but I can look into it. |
Original comment by @msabramo I don't recall implementing Looking at https://bitbucket.org/hpk42/tox/commits/all?search=posargs, I see 5 interesting-looking commits from @offbyone -- maybe see what he says...? |
Original comment by ehopemorley unfortunately --testr-args reuires the '=' with no spaces. In any case this seem like a parse error on the part of tox. |
With tox version 1.7.0 I get the following error:
Which appears to be caused by the following line in my tox.ini:
commands = python setup.py test --slowest --testr-args='{posargs}'
This worked fine with the previous version of tox i.e. 1.6.1
The text was updated successfully, but these errors were encountered: