Overhaul external process calls#7572
Conversation
|
I wonder if we should have been using our |
tomspur
force-pushed
the
subprocesses
branch
from
6501dd9 to
ffc993c
Compare
Current coverage is 62.03% (diff: 32.94%)@@ master #7572 diff @@
==========================================
Files 109 174 +65
Lines 46633 56048 +9415
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 31049 34770 +3721
- Misses 15584 21278 +5694
Partials 0 0
|
|
Thanks for the pointer to Just as a reference of the benefit of this: |
tomspur
force-pushed
the
subprocesses
branch
from
68a6cc7 to
5208d74
Compare
|
Having access to the stderr seems to work fine: Which points to |
|
The failure seems to be related to an upgrade of freetype on travis... |
|
No, it's just a flaky test; we build in a specific freetype version on CI. |
tomspur
force-pushed
the
subprocesses
branch
from
0e50192 to
10adcfc
Compare
|
Can you rebase and remove the use of |
|
Removing |
|
It's mostly just |
|
Thanks for the pointer. I didn't think about the |
tomspur
force-pushed
the
subprocesses
branch
2 times, most recently
from
c4dac47 to
9fac96b
Compare
|
There are now only two |
tomspur
force-pushed
the
subprocesses
branch
3 times, most recently
from
2aa5cf1 to
0a338f0
Compare
|
You should always import from our compat shim, https://github.com/matplotlib/matplotlib/blob/1a9b4eecff4409b4d9e5502df2ab12b4b16ac1f8/lib/matplotlib/compat/subprocess.py due to issues on early micro versions of 2.7 on mac (see #5314 ) |
examples/tests/backend_driver.py
Outdated
| except ImportError: | ||
| def run(arglist): | ||
| os.system(' '.join(arglist)) | ||
| os.system(arglist) |
There was a problem hiding this comment.
Can just drop this whole section, this is <2.7 compat code.
There was a problem hiding this comment.
Removed this part of the backend driver and used the compat shim also there.
|
Ah, disregard my comment, I see you are using the shim internally but not in the examples. Carry on 🐑 |
tomspur
force-pushed
the
subprocesses
branch
2 times, most recently
from
36b4b44 to
d89c25a
Compare
|
I don't know how to fix the python 2.7 failure. It looks quite similar to #7715, which @tacaswell has tracked down. Any pointers how to fix it here? |
| "%s" > "%s"'% (gs_exe, dpi, device_name, | ||
| paper_option, psfile, tmpfile, outfile) | ||
|
|
||
| command = [gs_exe, "-dBATCH", "-dNOPAUSE", "-r%d" % dpi, |
There was a problem hiding this comment.
This probably needs to be str(gs_exe)
|
Ping @tomspur? You can ignore the build errors; they're just flaky tests. |
tomspur
force-pushed
the
subprocesses
branch
2 times, most recently
from
7a0a653 to
d85947a
Compare
|
I just rebased it to master and cleaned up some parts. Let's see how it looks like now. Things not implemented yet:
|
tomspur
force-pushed
the
subprocesses
branch
3 times, most recently
from
d81ea42 to
bc00092
Compare
This commits starts with moving from `os.system` to the `subprocess` module and captures stderr and includes it in the exceptions. For more information see matplotlibgh-7490.
The full traceback leads to here:
lib/matplotlib/backends/backend_ps.py:1537: in gs_distill
if ps_backend_helper.supports_ps2write: # gs version >= 9
lib/matplotlib/backends/backend_ps.py:106: in supports_ps2write
return self.gs_version[0] >= 9
lib/matplotlib/backends/backend_ps.py:87: in gs_version
s = Popen([self.gs_exe, "--version"], stdout=PIPE)
venv/lib/python2.7/site-packages/subprocess32.py:825: in __init__
restore_signals, start_new_session)
venv/lib/python2.7/site-packages/subprocess32.py:1387: in _execute_child
for exe in executable_list)
venv/lib/python2.7/site-packages/subprocess32.py:1386: in <genexpr>
executable_list = tuple(fs_encode(exe)
venv/lib/python2.7/site-packages/subprocess32.py:1385: in <genexpr>
for dir in path_list)
|
All comments should be implemented - the failing test in |
There was a problem hiding this comment.
LGTM, modulo a few minor questions. Low coverage is probably to be expected given that we probably don't test what happens when subprocess calls fail. If it interests you, you can take a stab at writing some tests for that, but I don't think it needs to hold up this PR.
| from matplotlib.compat.subprocess import Popen, PIPE | ||
| s = Popen(self.gs_exe + " --version", | ||
| shell=True, stdout=PIPE) | ||
| s = Popen([str(self.gs_exe), "--version"], stdout=PIPE) |
| (gs_exe, tmpfile) | ||
| command = '%s -dBATCH -dNOPAUSE -sDEVICE=bbox "%s"' % (gs_exe, tmpfile) | ||
| verbose.report(command, 'debug') | ||
| stdin, stdout, stderr = os.popen3(command) |
There was a problem hiding this comment.
This one could use subprocess.Popen, probably.
There was a problem hiding this comment.
I missed that... Thanks
| mpl.verbose.report(report, 'debug') | ||
| 'string:\n%s\n\n' | ||
| 'Here is the full report generated by LaTeX:\n%s ' | ||
| '\n\n' % (repr(tex.encode('unicode_escape')), |
There was a problem hiding this comment.
Did something require adding the unicode escaping? Windows?
There was a problem hiding this comment.
Good question. This was like this in one of the calls the current master and I took that over in all new subprocess.check_outputs
| ('dvipng was not able to process the following ' | ||
| 'string:\n%s\n\n' | ||
| 'Here is the full report generated by dvipng:\n%s ' | ||
| '\n\n' % (repr(tex.encode('unicode_escape')), |
| ('dvips was not able to process the following ' | ||
| 'string:\n%s\n\n' | ||
| 'Here is the full report generated by dvips:\n%s ' | ||
| '\n\n' % (repr(tex.encode('unicode_escape')), |
|
Thanks for the review.
|
|
Thanks for doing this tedious work! Looking forward to the added test coverage. |
6 participants
This pull request starts to implement the move from
os.systemto thesubprocessmodule and captures stderr and includes it in the exceptions.For more information see #7490.