Re: [PATCH] Use subprocess.Popen for executing commands in cmdline tests

2008/4/19 Daniel Shahaf <d.s_at_daniel.shahaf.co.il>:
> jeremy hinds wrote on Fri, 18 Apr 2008 at 19:01 -0600:
>
> > I started working on getting a Windows dev environment set up so I can
> > at least make sure things can be expected to work before asking for
> > anybody else's time to test it. But I'm not quite there yet. In the
> > meantime, if someone else happens to get a chance to look at it, that
> > would be great.
> >
>
> I'm getting this error with your patch on Windows (ra_svn fsfs
> VC9Express):
>
> START: changelist_tests.py
> CMD: svnadmin.exe create svn-test-work\local_tmp\repos --bdb-txn-nosync --fs-type=fsfs
> Traceback (most recent call last):
> File "F:\Data\Unzip\svn\subversion/tests/cmdline/changelist_tests.py", line 885, in <module>
> svntest.main.run_tests(test_list)
> File "F:\Data\Unzip\svn\subversion\tests\cmdline\svntest\main.py", line 1386, in run_tests
> actions.setup_pristine_repository()
> File "F:\Data\Unzip\svn\subversion\tests\cmdline\svntest\actions.py", line 44, in setup_pristine_repository
> main.create_repos(main.pristine_dir)
> File "F:\Data\Unzip\svn\subversion\tests\cmdline\svntest\main.py", line 581, in create_repos
> path, *opts)
> File "F:\Data\Unzip\svn\subversion\tests\cmdline\svntest\main.py", line 309, in run_command
> None, *varargs)
> File "F:\Data\Unzip\svn\subversion\tests\cmdline\svntest\main.py", line 398, in run_command_stdin
> *varargs)
> File "F:\Data\Unzip\svn\subversion\tests\cmdline\svntest\main.py", line 365, in spawn_process
> infile, outfile, errfile, kid = open_pipe(cmd_argv, binary_mode)
> File "F:\Data\Unzip\svn\subversion\tests\cmdline\svntest\main.py", line 326, in open_pipe
> stdin=PIPE, stdout=PIPE, stderr=PIPE, close_fds=True)
> File "E:\Python\lib\subprocess.py", line 551, in __init__
> raise ValueError("close_fds is not supported on Windows "
> ValueError: close_fds is not supported on Windows platforms
> END: changelist_tests.py
>
> (The error repeats for every *_tests.py file.)
>
> Python docs agree: (http://docs.python.org/lib/node528.html)
>
> "If close_fds is true, all file descriptors except 0, 1 and 2 will
> be closed before the child process is executed. (Unix only)"
> ^^^^^^^^^^^
>
> Daniel

Thanks for giving it a try, Daniel. I saw that "Unix only" thing in
the documentation, but it seemed unclear what the behavior would be on
non-Unix. I was rather hoping for "silently ignored." My plan was to
get a Windows build working to test this before submitting the updated
patch, but I think I'm going to have to throw in the towel on that for
now. I did at least extract the Popen functionality into a separate
script and ran that on Windows (I should have done this before). The
pertinent lines in the patch are

+ close_fds=False
+ if is_posix_os():
+ close_fds=True
+ # (use universal newlines for text mode)
+ kid = Popen(command, shell=False, bufsize=-1,
+ universal_newlines=(not binary_mode),
+ stdin=PIPE, stdout=PIPE, stderr=PIPE, close_fds=close_fds)
+ return kid.stdin, kid.stdout, kid.stderr, (kid, command)

The only other difference between this and the previous iteration is
the removal of another comment in svntest/main.py.

[[[
In cmdline tests, use subprocess.Popen for executing commands to allow
support for exit-code checks on both Windows and posix systems. This makes
the test suite require Python version >= 2.4.

* subversion/tests/cmdline/svntest/main.py
  (global): Import subprocess instead of popen2. Remove variable
    "platform_with_popen3_class".
  (_quote_arg): Removed, since commands + args to open_pipe are now lists.
  (open_pipe): Replace "mode" character parameter with a "binary_mode" boolean
    value. Use subprocess.Popen for spawning the child process.
  (wait_on_pipe): Interpret the return value of wait() according to
    subprocess.Popen semantics.
  (spawn_process, copy_repos): When calling open_pipe, pass the command as
    a list and binary_mode as a boolean.
  (run_svn2): New, like run_svn but with a "binary_mode" boolean param.
  (TestSpawningThread.run_one): Remove the caveat comment stating that
    result (exit-code) is None on Windows.

* subversion/tests/cmdline/svntest/actions.py
  (run_and_verify_svnlook, run_and_verify_svnlook2, run_and_verify_svnadmin,
   run_and_verify_svnadmin2, run_and_verify_svnversion,
   run_and_verify_svnversion2,
   run_and_verify_svn_match_any, run_and_verify_svn_match_any2):
    Remove the caveat comment stating that exit-code checks are skipped
    for some platforms.
  (run_and_verify_svn): Remove the caveat comment stating that exit-code
    checks are skipped for some platforms. Pass binary_mode=0 to
    run_and_verify_svn2.
  (run_and_verify_svn2): Remove the caveat comment stating that exit-code
    checks are skipped for some platforms. Add a binary_mode boolean
    parameter. Replace call to run_svn with run_svn2.

* subversion/tests/cmdline/import_tests.py
  (import_eol_style): Call run_and_verify_svn2 with binary_mode=1.

* subversion/tests/cmdline/cat_tests.py,
  subversion/tests/cmdline/lock_tests.py,
  subversion/tests/cmdline/stat_tests.py:
    Pass binary_mode parameter to run_and_verify_svn2.

]]]

>
>
>
> >
> >
> > On Fri, Apr 18, 2008 at 6:35 AM, Branko Čibej <brane_at_xbc.nu> wrote:
> > > Daniel Shahaf wrote:
> > >
> > > > jeremy hinds wrote on Wed, 9 Apr 2008 at 22:49 -0600:
> > > >
> > > >
> > > > > 2008/4/8 jeremy hinds <jeremy.hinds_at_gmail.com>:
> > > > >
> > > > >
> > > > > > Thanks for clearing that up. While rewriting the patch to handle
> > > > > > universal_newlines and shell=False, another question came up.
> > > > > >
> > > > > > I made universal_newlines apply on any platform rather than just
> > > > > > Windows, and that seemed to work fine on everything except
> > > > > > import_tests.import_eol_style(). The problem is that
> > > > > > run_and_verify_svn() has text mode hard-coded in it, so the "\r\n"
> > > > > > sequences were converted to "\n". (Honestly, I don't understand how
> > > > > > that test worked accurately on Windows before.)
> > > > > >
> > > > > > The question is whether it is better to preserve old behavior by
> > > never
> > > > > > using universal_newlines on posix, or should I add a "binary_mode"
> > > > > > parameter to run_and_verify_svn2()?
> > > > > >
> > > > > >
> > > > > >
> > > > > I decided err on the side of flexibility and add a "binary_mode" param
> > > > > to run_and_verify_svn2(). So here is my new and improved (and
> > > > > hopefully good) patch. Again, I've only been able to test this on
> > > > > Linux.
> > > > >
> > > > > [[[
> > > > > In cmdline tests, use subprocess.Popen for executing commands to allow
> > > > > support for exit-code checks on both Windows and posix systems. This
> > > makes
> > > > > the test suite require Python version >= 2.4.
> > > > >
> > > > >
> > > > >
> > > >
> > > > Ping. Has anyone reviewed the binary_mode changes? Could someone verify
> > > that the patch works on Windows too?
> > > >
> > > >
> > >
> > > Oh sorry. Looks good to me. I can't test on Windows these days, sorry.
> > >
> > > -- Brane
> > >
> > >
> >

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org