You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2013/08/01 13:51:20 UTC
Re: [PATCH] print debug information when subprocess.Popen
subprocess.Popen
Please send patches as text/plain (*.txt extension often does this).
Masaru Tsuchiyama wrote on Sun, Jul 28, 2013 at 20:00:27 +0900:
> +++ subversion/tests/cmdline/svntest/main.py (working copy)
> @@ -412,12 +412,19 @@
> if not stderr:
> stderr = subprocess.PIPE
>
> - p = subprocess.Popen(command,
> + try:
> + p = subprocess.Popen(command,
> bufsize,
> stdin=stdin,
> stdout=stdout,
> stderr=stderr,
> close_fds=not windows)
> + except:
Don't catch everything. Here you can/should catch only CalledProcessError.
> + # catch expeption, print information, and reraise
> + print "current dir:", os.path.abspath(os.getcwd())
> + print command
Makes me twitchy. svntest is a library and it shouldn't print, since
this assumes no caller catches the exception. If possible I'd rather
annotate the exception object and re-raise it.
Also you should use a print() when possible for Python 3 compatibility.
I haven't looked at the context, but I assume this is to address the
failure mode of running tests before the svn* binaries are built?
> + raise
> +
> return p.stdin, p.stdout, p.stderr, (p, command_string)
>
> def wait_on_pipe(waiter, binary_mode, stdin=None):
Cheers,
Daniel
Re: [PATCH] print debug information when subprocess.Popen
subprocess.Popen
Posted by Daniel Shahaf <da...@apache.org>.
On Thu, Aug 22, 2013 at 11:22:59PM +0900, Masaru Tsuchiyama wrote:
> Daniel Shahaf wrote:
> >On Sat, Aug 03, 2013 at 03:52:49PM +0300, Daniel Shahaf wrote:
> >>Masaru Tsuchiyama wrote on Sat, Aug 03, 2013 at 16:06:06 +0900:
> >>>If I run the these scripts at Release\subversion\tests\cmdline, I don't
> >>>get a file-not-found error.
> >>
> >>So perhaps the right fix is not your patch, but to look for
> >>atomic-ra-revprop-change up front and bail out if it's not found?
> >
> >Or, more simply,
> >
> > os.chdir(os.path.basename(sys.argv[0]))
> >
>
> Where do you suggest to do that?
Actually, I just realised that using argv[0] like that is wrong, it would break
out-of-tree builds. Sorry for the wrong suggestion.
Re: [PATCH] print debug information when subprocess.Popen subprocess.Popen
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Daniel Shahaf wrote:
> On Sat, Aug 03, 2013 at 03:52:49PM +0300, Daniel Shahaf wrote:
>> Masaru Tsuchiyama wrote on Sat, Aug 03, 2013 at 16:06:06 +0900:
>>> If I run the these scripts at Release\subversion\tests\cmdline, I don't
>>> get a file-not-found error.
>>
>> So perhaps the right fix is not your patch, but to look for
>> atomic-ra-revprop-change up front and bail out if it's not found?
>
> Or, more simply,
>
> os.chdir(os.path.basename(sys.argv[0]))
>
Where do you suggest to do that?
--
Masaru Tsuchiyama <m....@gmail.com>
Re: [PATCH] print debug information when subprocess.Popen
subprocess.Popen
Posted by Daniel Shahaf <da...@apache.org>.
On Sat, Aug 03, 2013 at 03:52:49PM +0300, Daniel Shahaf wrote:
> Masaru Tsuchiyama wrote on Sat, Aug 03, 2013 at 16:06:06 +0900:
> > If I run the these scripts at Release\subversion\tests\cmdline, I don't
> > get a file-not-found error.
>
> So perhaps the right fix is not your patch, but to look for
> atomic-ra-revprop-change up front and bail out if it's not found?
Or, more simply,
os.chdir(os.path.basename(sys.argv[0]))
Re: [PATCH] print debug information when subprocess.Popen
subprocess.Popen
Posted by Daniel Shahaf <da...@elego.de>.
Masaru Tsuchiyama wrote on Sat, Aug 03, 2013 at 16:06:06 +0900:
> > One of your later patches had a *.txt extension but
> application/octet-stream
> > MIME type. It would be good if you could get your client to set text/*
> > MIME type --- this actually changes the way the patch renders in many
> > devs' clients, makes it easier for them to read/review/reply the patch.
>
> My mail client can handle text/plain MIME type, but it always encodes an
> file with uuencode.
The Content-Transfer-Encoding is not as important as the Content-Type, IMO.
> I'll switch my client to Thunderbird. Thunderbird seems to be able to
> handle such situation.
>
That's one way of solving the problem :-)
> > WindowsError is an instance of OSError, and that method is documented as
> > raising CalledProcessError, so you might do:
> >
> > except (OSError, CalledProcessError):
> > > > > + # catch expeption, print information, and reraise
> > > > > + print "current dir:", os.path.abspath(os.getcwd())
> > > > > + print command
>
> It seems to use subprocess.CalledProcessError instead of CalledProcessError.
Yes, I wasn't using the fully-qualified name.
> I got NameError when I used the code.
>
> NameError: global name 'CalledProcessError' is not defined
>
> > > >
> > > > Makes me twitchy. svntest is a library and it shouldn't print, since
> > > > this assumes no caller catches the exception. If possible I'd rather
> > > > annotate the exception object and re-raise it.
> > >
> > > Could you tell me how to 'annotate an exception'?
> > >
> >
> > I'm not completely sure. The 'raise from' syntax is not available. You
> > could set a new attribute on the CalledProcessError or OSError instance,
> > and have callers inspect that attribute _if available_ (hasattr/getattr).
> > Or you could subclass those two and raise an instance of the subclass.
>
> I'll try to find more appropriate method.
>
Okay. Feel free to ask for opinions if needed.
> > > > I haven't looked at the context, but I assume this is to address the
> > > > failure mode of running tests before the svn* binaries are built?
> > >
> > > I used checkout_tests.py, export_tests.py, prop_tests.py, and
> > > update_test.py respectively. At the time, I needed to copy the test
> > > binaries manually to the location which these scripts expected.
> >
> > So... what is a case in which the new information you added will be
> > displayed? Is it "run one test on windows"? Is it "run tests with
> > a typo in the argument of the --bin option"?
>
> I was running these scripts on the root of the working directory.
>
> When using prop_tests.py on the root directory, I got a file-not-found
> error.
> I implemented the patch to find which file is not found.
>
> Then I found it that atomic-ra-revprop-change.exe couldn't be found.
> --bin option can't handle the location of atomic-ra-revprop-change.exe.
> main.py expects that atomic-ra-revprop-change.exe is in the current
> directory,
> and can't change the location by command line option.
>
> If I run the these scripts at Release\subversion\tests\cmdline, I don't
> get a file-not-found error.
So perhaps the right fix is not your patch, but to look for
atomic-ra-revprop-change up front and bail out if it's not found?
IIRC, on unix, if you run the tests as
subversion/tests/cmdline/basic_tests.py
it will create svn-test-work in cwd, which may be argued to be
undesirable.
Re: [PATCH] print debug information when subprocess.Popen subprocess.Popen
Posted by Masaru Tsuchiyama <m....@gmail.com>.
> One of your later patches had a *.txt extension but
application/octet-stream
> MIME type. It would be good if you could get your client to set text/*
> MIME type --- this actually changes the way the patch renders in many
> devs' clients, makes it easier for them to read/review/reply the patch.
My mail client can handle text/plain MIME type, but it always encodes an
file with uuencode.
I'll switch my client to Thunderbird. Thunderbird seems to be able to
handle such situation.
> WindowsError is an instance of OSError, and that method is documented as
> raising CalledProcessError, so you might do:
>
> except (OSError, CalledProcessError):
> > > > + # catch expeption, print information, and reraise
> > > > + print "current dir:", os.path.abspath(os.getcwd())
> > > > + print command
It seems to use subprocess.CalledProcessError instead of CalledProcessError.
I got NameError when I used the code.
NameError: global name 'CalledProcessError' is not defined
> > >
> > > Makes me twitchy. svntest is a library and it shouldn't print, since
> > > this assumes no caller catches the exception. If possible I'd rather
> > > annotate the exception object and re-raise it.
> >
> > Could you tell me how to 'annotate an exception'?
> >
>
> I'm not completely sure. The 'raise from' syntax is not available. You
> could set a new attribute on the CalledProcessError or OSError instance,
> and have callers inspect that attribute _if available_ (hasattr/getattr).
> Or you could subclass those two and raise an instance of the subclass.
I'll try to find more appropriate method.
> > > I haven't looked at the context, but I assume this is to address the
> > > failure mode of running tests before the svn* binaries are built?
> >
> > I used checkout_tests.py, export_tests.py, prop_tests.py, and
> > update_test.py respectively. At the time, I needed to copy the test
> > binaries manually to the location which these scripts expected.
>
> So... what is a case in which the new information you added will be
> displayed? Is it "run one test on windows"? Is it "run tests with
> a typo in the argument of the --bin option"?
I was running these scripts on the root of the working directory.
When using prop_tests.py on the root directory, I got a file-not-found
error.
I implemented the patch to find which file is not found.
Then I found it that atomic-ra-revprop-change.exe couldn't be found.
--bin option can't handle the location of atomic-ra-revprop-change.exe.
main.py expects that atomic-ra-revprop-change.exe is in the current
directory,
and can't change the location by command line option.
If I run the these scripts at Release\subversion\tests\cmdline, I don't
get a file-not-found error.
Regards.
Re: [PATCH] print debug information when subprocess.Popen
subprocess.Popen
Posted by Daniel Shahaf <da...@elego.de>.
Masaru Tsuchiyama wrote on Sat, Aug 03, 2013 at 11:04:30 +0900:
> > Please send patches as text/plain (*.txt extension often does this).
>
> I understand.
>
Thanks.
One of your later patches had a *.txt extension but application/octet-stream
MIME type. It would be good if you could get your client to set text/*
MIME type --- this actually changes the way the patch renders in many
devs' clients, makes it easier for them to read/review/reply the patch.
> > Masaru Tsuchiyama wrote on Sun, Jul 28, 2013 at 20:00:27 +0900:
> > > +++ subversion/tests/cmdline/svntest/main.py (working copy)
> > > @@ -412,12 +412,19 @@
> > > if not stderr:
> > > stderr = subprocess.PIPE
> > >
> > > - p = subprocess.Popen(command,
> > > + try:
> > > + p = subprocess.Popen(command,
> > > bufsize,
> > > stdin=stdin,
> > > stdout=stdout,
> > > stderr=stderr,
> > > close_fds=not windows)
> > > + except:
> >
> > Don't catch everything. Here you can/should catch only CalledProcessError.
>
> I encountered WindowsError exception on Windows Python 2.7.
> I'm not sure which expection is raised on Linux/UNIX.
> So I catch all, and reraise it.
>
Capturing all exceptions is wrong, period. How do you know that on
Linux you aren't capturing an "DoNotCatchMeOrIWillRmDashRFSlashError"
exception?
WindowsError is an instance of OSError, and that method is documented as
raising CalledProcessError, so you might do:
except (OSError, CalledProcessError):
> > > + # catch expeption, print information, and reraise
> > > + print "current dir:", os.path.abspath(os.getcwd())
> > > + print command
> >
> > Makes me twitchy. svntest is a library and it shouldn't print, since
> > this assumes no caller catches the exception. If possible I'd rather
> > annotate the exception object and re-raise it.
>
> Could you tell me how to 'annotate an exception'?
>
I'm not completely sure. The 'raise from' syntax is not available. You
could set a new attribute on the CalledProcessError or OSError instance,
and have callers inspect that attribute _if available_ (hasattr/getattr).
Or you could subclass those two and raise an instance of the subclass.
> > I haven't looked at the context, but I assume this is to address the
> > failure mode of running tests before the svn* binaries are built?
>
> I used checkout_tests.py, export_tests.py, prop_tests.py, and
> update_test.py respectively. At the time, I needed to copy the test
> binaries manually to the location which these scripts expected.
So... what is a case in which the new information you added will be
displayed? Is it "run one test on windows"? Is it "run tests with
a typo in the argument of the --bin option"?
Re: [PATCH] print debug information when subprocess.Popen subprocess.Popen
Posted by Masaru Tsuchiyama <m....@gmail.com>.
> Please send patches as text/plain (*.txt extension often does this).
I understand.
> Masaru Tsuchiyama wrote on Sun, Jul 28, 2013 at 20:00:27 +0900:
> > +++ subversion/tests/cmdline/svntest/main.py (working copy)
> > @@ -412,12 +412,19 @@
> > if not stderr:
> > stderr = subprocess.PIPE
> >
> > - p = subprocess.Popen(command,
> > + try:
> > + p = subprocess.Popen(command,
> > bufsize,
> > stdin=stdin,
> > stdout=stdout,
> > stderr=stderr,
> > close_fds=not windows)
> > + except:
>
> Don't catch everything. Here you can/should catch only CalledProcessError.
I encountered WindowsError exception on Windows Python 2.7.
I'm not sure which expection is raised on Linux/UNIX.
So I catch all, and reraise it.
> > + # catch expeption, print information, and reraise
> > + print "current dir:", os.path.abspath(os.getcwd())
> > + print command
>
> Makes me twitchy. svntest is a library and it shouldn't print, since
> this assumes no caller catches the exception. If possible I'd rather
> annotate the exception object and re-raise it.
Could you tell me how to 'annotate an exception'?
> Also you should use a print() when possible for Python 3 compatibility.
I understand.
> I haven't looked at the context, but I assume this is to address the
> failure mode of running tests before the svn* binaries are built?
I used checkout_tests.py, export_tests.py, prop_tests.py, and
update_test.py respectively. At the time, I needed to copy the test
binaries manually to the location which these scripts expected.
--
Masaru Tsuchiyama <m....@gmail.com>