You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2010/11/08 16:20:55 UTC
[PATCH] Fix Windows test failures - Python test suite bug in
quoting a trailing backslash
Here's a patch to fix the Windows buildbot failures that started at my
r1031610. The problem is our _quote_arg() function doesn't do the right
thing with a trailing backslash. For a solution it seems better to
simply pass the args separately to subprocess.Popen() and let it do the
quoting, than to try to fix and maintain our own quoting function. And
we can use the undocumented subprocess.list2cmdline() function for
generating a command-line string for display purposes. (We could also
use that for generating a single-string argument to Popen, but we don't
need to because it does it for us if we pass a list of arguments.)
I tested this in a WinXP VM, and it seemed to work properly with Python
2.4.1 and 2.4.3 and 2.7. (I ran 1.7-trunk's prop_tests.py 7 against an
installed svn 1.6.13, and it passed.)
[[[
Fix Windows command-line argument quoting in the Python test harness.
Arguments ending with a backslash were not correctly quoted.
This reverts r875257.
* subversion/tests/cmdline/svntest/main.py
(_safe_arg_re, _quote_arg): Delete, as this didn't quote properly.
(open_pipe): Pass the list of arguments directly to subprocess.Popen()
instead of trying to quote it ourselves. Use subprocess.list2cmdline()
to generate a command line for display.
(spawn_process): Use subprocess.list2cmdline() instead of _quote_arg()
to generate a command line for display.
]]]
It reverts Paul's r875257, which was:
[[[
Fix test suite's use of subprocess on earlier versions of Python and thus
fix the djh-xp-vse2005 buildbot.
* subversion/tests/cmdline/svntest/main.py
(open_pipe2): Quote arguments to subprocess.Popen(). Later versions of
subprocess (2.5.2+ that I know of for sure) don't require this quoting, but
versions < 2.4.3 do.
]]]
I was unable to see exactly what the problem was that r875257 fixed. I
would like to be able to say why it's OK to revert it, but I don't yet
know.
Can Paul or anyone comment or test or review?
Or shall I just commit this current patch and see if anyone has
problems?
- Julian
Re: [PATCH] Fix Windows test failures - Python test suite bug in
quoting a trailing backslash
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-11-09 at 20:20 +0200, Daniel Shahaf wrote:
> Julian Foad wrote on Tue, Nov 09, 2010 at 12:07:16 +0000:
> > (We like to be able to copy and paste and run the displayed command.)
>
> I recall two oddities (at least in -v mode) in the Python tests:
>
> * Sometimes they gave to absolute path to atomic-ra-revprop-change (the
> compiled helper) and sometimes just its basename.
Hi Daniel. For progress indications, the test suite intentionally shows
only the basename of the executable. For error reports, it shows the
full path. I'm not changing that.
> * Arguments containing spaces weren't always quoted properly.
My patch should fix the quoting so that the command is shown in a
suitable form for copying and pasting as input to the local system's
shell, using MSCRT shell quoting rules on Windows and a sh-like shell on
non-Windows systems.
- Julian
> Here's an example (after changing atomic_over_ra()'s expectations
> such that it FAILs):
>
> % ./prop_tests.py -v 34
> CMD: atomic-ra-revprop-change file:///home/daniel/src/svn/t20/subversion/tests/cmdline/svn-test-work/repositories/prop_tests-34 0 flower "( 11 old_value_p 0 5 value 11 wrong value )" serf 0
> CMD: /home/daniel/src/svn/t20/subversion/tests/cmdline/atomic-ra-revprop-change file:///home/daniel/src/svn/t20/subversion/tests/cmdline/svn-test-work/repositories/prop_tests-34 0 flower ( 11 old_value_p 0 5 value 11 wrong value ) serf 0 exited with 1
>
> Ideally, I'd want this:
>
> CMD: /home/daniel/src/svn/t20/subversion/tests/cmdline/atomic-ra-revprop-change
> file:///home/daniel/src/svn/t20/subversion/tests/cmdline/svn-test-work/repositories/prop_tests-34
> 0 flower "( 11 old_value_p 0 5 value 11 wrong value )" serf 0
>
> plus some indication of the exit code if appropriate.
>
>
> Haven't tested with your patch yet, sorry. If it doesn't cover that,
> I can take a look later.
Re: [PATCH] Fix Windows test failures - Python test suite bug in
quoting a trailing backslash
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Tue, Nov 09, 2010 at 12:07:16 +0000:
> (We like to be able to copy and paste and run the displayed command.)
I recall two oddities (at least in -v mode) in the Python tests:
* Sometimes they gave to absolute path to atomic-ra-revprop-change (the
compiled helper) and sometimes just its basename.
* Arguments containing spaces weren't always quoted properly.
Here's an example (after changing atomic_over_ra()'s expectations
such that it FAILs):
% ./prop_tests.py -v 34
CMD: atomic-ra-revprop-change file:///home/daniel/src/svn/t20/subversion/tests/cmdline/svn-test-work/repositories/prop_tests-34 0 flower "( 11 old_value_p 0 5 value 11 wrong value )" serf 0
CMD: /home/daniel/src/svn/t20/subversion/tests/cmdline/atomic-ra-revprop-change file:///home/daniel/src/svn/t20/subversion/tests/cmdline/svn-test-work/repositories/prop_tests-34 0 flower ( 11 old_value_p 0 5 value 11 wrong value ) serf 0 exited with 1
Ideally, I'd want this:
CMD: /home/daniel/src/svn/t20/subversion/tests/cmdline/atomic-ra-revprop-change
file:///home/daniel/src/svn/t20/subversion/tests/cmdline/svn-test-work/repositories/prop_tests-34
0 flower "( 11 old_value_p 0 5 value 11 wrong value )" serf 0
plus some indication of the exit code if appropriate.
Haven't tested with your patch yet, sorry. If it doesn't cover that,
I can take a look later.
Daniel
Re: [PATCH] Fix Windows test failures - Python test suite bug in
quoting a trailing backslash
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-11-09, Philip Martin wrote:
> Paul Burba <pt...@gmail.com> writes:
>
> > File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\main.py",
> > line 433, in spawn_process
> > subprocess.list2cmdline(varargs)))
> > File "C:\Python26\lib\subprocess.py", line 541, in list2cmdline
> > needquote = (" " in arg) or ("\t" in arg) or ("|" in arg) or not arg
> > TypeError: argument of type 'int' is not iterable
> > FAIL: commit_tests.py 41: set revision props during remote mkdir
> > ]]]
> >
> > No time to look into this further today, but I can check it out tomorrow.
>
> Mapping to strings looks like it should fix that:
>
> subprocess.list2cmdline(map(str varargs))))
Thanks: I forgot to run the full test suite. (With a comma: map(str,
varargs).)
But I realized list2cmdline() is Windows-specific, so although we're
only using it for display purposes it's not nice for us Unix types. (We
like to be able to copy and paste and run the displayed command.)
I have searched the web for a generic way of doing this, and found
references to several methods including subprocess.list2cmdline(),
pipes.quote(), commands.mkarg(), and various code snippets. None of
these are good generic solutions. I saw recent discussions on a Python
mailing list about the need for a generic solution.
So here is another patch, doing the best I can for now. I'm still
testing this one.
- Julian
Re: [PATCH] Fix Windows test failures - Python test suite bug in
quoting a trailing backslash
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-11-09, Philip Martin wrote:
> Paul Burba <pt...@gmail.com> writes:
>
> > File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\main.py",
> > line 433, in spawn_process
> > subprocess.list2cmdline(varargs)))
> > File "C:\Python26\lib\subprocess.py", line 541, in list2cmdline
> > needquote = (" " in arg) or ("\t" in arg) or ("|" in arg) or not arg
> > TypeError: argument of type 'int' is not iterable
> > FAIL: commit_tests.py 41: set revision props during remote mkdir
> > ]]]
> >
> > No time to look into this further today, but I can check it out tomorrow.
>
> Mapping to strings looks like it should fix that:
>
> subprocess.list2cmdline(map(str varargs))))
Thanks: I forgot to run the full test suite. (With a comma: map(str,
varargs).)
But I realized list2cmdline() is Windows-specific, so although we're
only using it for display purposes it's not nice for us Unix types. (We
like to be able to copy and paste and run the displayed command.)
I have searched the web for a generic way of doing this, and found
references to several methods including subprocess.list2cmdline(),
pipes.quote(), commands.mkarg(), and various code snippets. None of
these are good generic solutions. I saw recent discussions on a Python
mailing list about the need for a generic solution.
So here is another patch, doing the best I can for now. I'm still
testing this one.
- Julian
Re: [PATCH] Fix Windows test failures - Python test suite bug in quoting a trailing backslash
Posted by Philip Martin <ph...@wandisco.com>.
Paul Burba <pt...@gmail.com> writes:
> File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\main.py",
> line 433, in spawn_process
> subprocess.list2cmdline(varargs)))
> File "C:\Python26\lib\subprocess.py", line 541, in list2cmdline
> needquote = (" " in arg) or ("\t" in arg) or ("|" in arg) or not arg
> TypeError: argument of type 'int' is not iterable
> FAIL: commit_tests.py 41: set revision props during remote mkdir
> ]]]
>
> No time to look into this further today, but I can check it out tomorrow.
Mapping to strings looks like it should fix that:
subprocess.list2cmdline(map(str varargs))))
--
Philip
Re: [PATCH] Fix Windows test failures - Python test suite bug in
quoting a trailing backslash
Posted by Paul Burba <pt...@gmail.com>.
On Mon, Nov 8, 2010 at 12:05 PM, Paul Burba <pt...@gmail.com> wrote:
> On Mon, Nov 8, 2010 at 11:20 AM, Julian Foad <ju...@wandisco.com> wrote:
>> Here's a patch to fix the Windows buildbot failures that started at my
>> r1031610. The problem is our _quote_arg() function doesn't do the right
>> thing with a trailing backslash. For a solution it seems better to
>> simply pass the args separately to subprocess.Popen() and let it do the
>> quoting, than to try to fix and maintain our own quoting function. And
>> we can use the undocumented subprocess.list2cmdline() function for
>> generating a command-line string for display purposes. (We could also
>> use that for generating a single-string argument to Popen, but we don't
>> need to because it does it for us if we pass a list of arguments.)
>>
>> I tested this in a WinXP VM, and it seemed to work properly with Python
>> 2.4.1 and 2.4.3 and 2.7. (I ran 1.7-trunk's prop_tests.py 7 against an
>> installed svn 1.6.13, and it passed.)
>>
>> [[[
>> Fix Windows command-line argument quoting in the Python test harness.
>> Arguments ending with a backslash were not correctly quoted.
>> This reverts r875257.
>>
>> * subversion/tests/cmdline/svntest/main.py
>> (_safe_arg_re, _quote_arg): Delete, as this didn't quote properly.
>> (open_pipe): Pass the list of arguments directly to subprocess.Popen()
>> instead of trying to quote it ourselves. Use subprocess.list2cmdline()
>> to generate a command line for display.
>> (spawn_process): Use subprocess.list2cmdline() instead of _quote_arg()
>> to generate a command line for display.
>> ]]]
>>
>> It reverts Paul's r875257, which was:
>>
>> [[[
>> Fix test suite's use of subprocess on earlier versions of Python and thus
>> fix the djh-xp-vse2005 buildbot.
>>
>> * subversion/tests/cmdline/svntest/main.py
>> (open_pipe2): Quote arguments to subprocess.Popen(). Later versions of
>> subprocess (2.5.2+ that I know of for sure) don't require this quoting, but
>> versions < 2.4.3 do.
>> ]]]
>>
>> I was unable to see exactly what the problem was that r875257 fixed. I
>> would like to be able to say why it's OK to revert it, but I don't yet
>> know.
>>
>> Can Paul or anyone comment or test or review?
>>
>> Or shall I just commit this current patch and see if anyone has
>> problems?
>
> Hi Julian,
>
> +1 on "commit this current patch and see if anyone has problems".
>
> You've confirmed it works on Windows with 2.4.1, 2.4.3 and 2.7 (and
> 2.4 is the minimum we claim to support). I can confirm it works with
> 2.6.5.
Sorry Julian, scratch that last bit. I'm not sure what I did, but I
obviously ran the wrong thing (too many trunk WCs apparently!).
Running the tests for another change with your patch still in place I
noticed a lot of failures. Reverting my changes and running with just
your patch I still saw lots of problems, most of which are like this
failure of commit_tests.py 41:
[[[
CMD: svnadmin.exe create svn-test-work\repositories\commit_tests-41
--bdb-txn-nosync --fs-type=fsfs
<TIME = 0.094000>
CMD: svnadmin.exe dump svn-test-work\local_tmp\repos | svnadmin.exe
load svn-test-work\repositories\commit_tests-41 --ignore-uuid
<TIME = 0.002000>
CMD: svn.exe co
file:///C:/SVN/src-trunk-2/Release/subversion/tests/cmdline/svn-test-work/repositories/commit_tests-41
svn-test-work\working_copies\commit_tests-41 --config-dir
C:\SVN\src-trunk-2\Release\subversion\tests\cmdline\svn-test-work\local_tmp\config
--password rayjandom --no-auth-cache --username jrandom
<TIME = 0.197000>
A svn-test-work\working_copies\commit_tests-41\A
A svn-test-work\working_copies\commit_tests-41\A\B
A svn-test-work\working_copies\commit_tests-41\A\B\lambda
A svn-test-work\working_copies\commit_tests-41\A\B\E
A svn-test-work\working_copies\commit_tests-41\A\B\E\alpha
A svn-test-work\working_copies\commit_tests-41\A\B\E\beta
A svn-test-work\working_copies\commit_tests-41\A\B\F
A svn-test-work\working_copies\commit_tests-41\A\mu
A svn-test-work\working_copies\commit_tests-41\A\C
A svn-test-work\working_copies\commit_tests-41\A\D
A svn-test-work\working_copies\commit_tests-41\A\D\gamma
A svn-test-work\working_copies\commit_tests-41\A\D\G
A svn-test-work\working_copies\commit_tests-41\A\D\G\pi
A svn-test-work\working_copies\commit_tests-41\A\D\G\rho
A svn-test-work\working_copies\commit_tests-41\A\D\G\tau
A svn-test-work\working_copies\commit_tests-41\A\D\H
A svn-test-work\working_copies\commit_tests-41\A\D\H\chi
A svn-test-work\working_copies\commit_tests-41\A\D\H\omega
A svn-test-work\working_copies\commit_tests-41\A\D\H\psi
A svn-test-work\working_copies\commit_tests-41\iota
Checked out revision 1.
CMD: svn.exe mkdir -m msg --with-revprop bug=42
file:///C:/SVN/src-trunk-2/Release/subversion/tests/cmdline/svn-test-work/repositories/commit_tests-41/dir
--config-dir C:\SVN\src-trunk-2\Release\subversion\tests\cmdline\svn-test-work\local_tmp\config
--password rayjandom --no-auth-cache --username jrandom
<TIME = 0.201000>
Committed revision 2.
UNEXPECTED EXCEPTION:
Traceback (most recent call last):
File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\main.py",
line 1181, in run
rc = self.pred.run(sandbox)
File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\testcase.py",
line 243, in run
return self._delegate.run(sandbox)
File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\testcase.py",
line 170, in run
return self.func(sandbox)
File "C:\SVN\src-trunk-2\subversion/tests/cmdline/commit_tests.py",
line 2192, in mkdir_with_revprop
'--revprop', '-r', 2, sbox.repo_url)
File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\actions.py",
line 264, in run_and_verify_svn
expected_exit, *varargs)
File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\actions.py",
line 302, in run_and_verify_svn2
exit_code, out, err = main.run_svn(want_err, *varargs)
File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\main.py",
line 552, in run_svn
*(_with_auth(_with_config_dir(varargs))))
File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\main.py",
line 336, in run_command
None, *varargs)
File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\main.py",
line 471, in run_command_stdin
*varargs)
File "C:\SVN\src-trunk-2\subversion\tests\cmdline\svntest\main.py",
line 433, in spawn_process
subprocess.list2cmdline(varargs)))
File "C:\Python26\lib\subprocess.py", line 541, in list2cmdline
needquote = (" " in arg) or ("\t" in arg) or ("|" in arg) or not arg
TypeError: argument of type 'int' is not iterable
FAIL: commit_tests.py 41: set revision props during remote mkdir
]]]
No time to look into this further today, but I can check it out tomorrow.
Paul
Re: [PATCH] Fix Windows test failures - Python test suite bug in
quoting a trailing backslash
Posted by Paul Burba <pt...@gmail.com>.
On Mon, Nov 8, 2010 at 11:20 AM, Julian Foad <ju...@wandisco.com> wrote:
> Here's a patch to fix the Windows buildbot failures that started at my
> r1031610. The problem is our _quote_arg() function doesn't do the right
> thing with a trailing backslash. For a solution it seems better to
> simply pass the args separately to subprocess.Popen() and let it do the
> quoting, than to try to fix and maintain our own quoting function. And
> we can use the undocumented subprocess.list2cmdline() function for
> generating a command-line string for display purposes. (We could also
> use that for generating a single-string argument to Popen, but we don't
> need to because it does it for us if we pass a list of arguments.)
>
> I tested this in a WinXP VM, and it seemed to work properly with Python
> 2.4.1 and 2.4.3 and 2.7. (I ran 1.7-trunk's prop_tests.py 7 against an
> installed svn 1.6.13, and it passed.)
>
> [[[
> Fix Windows command-line argument quoting in the Python test harness.
> Arguments ending with a backslash were not correctly quoted.
> This reverts r875257.
>
> * subversion/tests/cmdline/svntest/main.py
> (_safe_arg_re, _quote_arg): Delete, as this didn't quote properly.
> (open_pipe): Pass the list of arguments directly to subprocess.Popen()
> instead of trying to quote it ourselves. Use subprocess.list2cmdline()
> to generate a command line for display.
> (spawn_process): Use subprocess.list2cmdline() instead of _quote_arg()
> to generate a command line for display.
> ]]]
>
> It reverts Paul's r875257, which was:
>
> [[[
> Fix test suite's use of subprocess on earlier versions of Python and thus
> fix the djh-xp-vse2005 buildbot.
>
> * subversion/tests/cmdline/svntest/main.py
> (open_pipe2): Quote arguments to subprocess.Popen(). Later versions of
> subprocess (2.5.2+ that I know of for sure) don't require this quoting, but
> versions < 2.4.3 do.
> ]]]
>
> I was unable to see exactly what the problem was that r875257 fixed. I
> would like to be able to say why it's OK to revert it, but I don't yet
> know.
>
> Can Paul or anyone comment or test or review?
>
> Or shall I just commit this current patch and see if anyone has
> problems?
Hi Julian,
+1 on "commit this current patch and see if anyone has problems".
You've confirmed it works on Windows with 2.4.1, 2.4.3 and 2.7 (and
2.4 is the minimum we claim to support). I can confirm it works with
2.6.5.
Paul
Re: [PATCH] Fix Windows test failures - Python test suite bug in
quoting a trailing backslash
Posted by Julian Foad <ju...@wandisco.com>.
Now with the patch attached.
And here's the email thread about the original change r875257 (aka
r35183): <http://svn.haxx.se/dev/archive-2009-01/0316.shtml>.
On Mon, 2010-11-08 at 16:20 +0000, Julian Foad wrote:
> Here's a patch to fix the Windows buildbot failures that started at my
> r1031610. The problem is our _quote_arg() function doesn't do the right
> thing with a trailing backslash. For a solution it seems better to
> simply pass the args separately to subprocess.Popen() and let it do the
> quoting, than to try to fix and maintain our own quoting function. And
> we can use the undocumented subprocess.list2cmdline() function for
> generating a command-line string for display purposes. (We could also
> use that for generating a single-string argument to Popen, but we don't
> need to because it does it for us if we pass a list of arguments.)
>
> I tested this in a WinXP VM, and it seemed to work properly with Python
> 2.4.1 and 2.4.3 and 2.7. (I ran 1.7-trunk's prop_tests.py 7 against an
> installed svn 1.6.13, and it passed.)
>
> [[[
> Fix Windows command-line argument quoting in the Python test harness.
> Arguments ending with a backslash were not correctly quoted.
> This reverts r875257.
>
> * subversion/tests/cmdline/svntest/main.py
> (_safe_arg_re, _quote_arg): Delete, as this didn't quote properly.
> (open_pipe): Pass the list of arguments directly to subprocess.Popen()
> instead of trying to quote it ourselves. Use subprocess.list2cmdline()
> to generate a command line for display.
> (spawn_process): Use subprocess.list2cmdline() instead of _quote_arg()
> to generate a command line for display.
> ]]]
>
> It reverts Paul's r875257, which was:
>
> [[[
> Fix test suite's use of subprocess on earlier versions of Python and thus
> fix the djh-xp-vse2005 buildbot.
>
> * subversion/tests/cmdline/svntest/main.py
> (open_pipe2): Quote arguments to subprocess.Popen(). Later versions of
> subprocess (2.5.2+ that I know of for sure) don't require this quoting, but
> versions < 2.4.3 do.
> ]]]
>
> I was unable to see exactly what the problem was that r875257 fixed. I
> would like to be able to say why it's OK to revert it, but I don't yet
> know.
>
> Can Paul or anyone comment or test or review?
>
> Or shall I just commit this current patch and see if anyone has
> problems?
>
> - Julian