BUG: fix checkout_output with non-ascii paths in PATH #7804

Merged
merged 2 commits into from Jan 13, 2017

Projects

None yet

3 participants

@tacaswell
Member

Closes #7715

This ensures that on python2 the values passed into check_output are
bytes. If they are passed in as unicode (due to unicode_literals at
the top of most of our files) they will cause an exception when there
is a path in PATH that has non-ascii values. The reason is that when
locating possible executable our input (as unicode) is concatenated
with the non-ascii path (as bytes) which then fails to encode as ascii
and raises.

The other two places we currently use check_output (setupext.py and
tools/github_stats.py) we do not need this handling as we do not
import unicode_iterals in those files.

@tacaswell tacaswell BUG: fix checkout_output with non-ascii paths in PATH
Closes #7715

This ensures that on python2 the values passed into `check_output` or
`Popen` are bytes.  If they are passed in as unicode (due to
unicode_literals at the top of most of our files) they will cause an
exception when there is a path in PATH that has non-ascii values.  The
reason is that when locating possible executable our input (as
unicode) is concatenated with the non-ascii path (as bytes) which then
fails to encode as ascii and raises.

The other two places we currently use `check_output` (setupext.py and
tools/github_stats.py) we do not need this handling as we do not
import unicode_iterals in those files.
df27722
lib/matplotlib/backends/backend_pgf.py
@@ -179,7 +179,7 @@ def make_pdf_to_png_converter():
tools_available = []
# check for pdftocairo
try:
- check_output(["pdftocairo", "-v"], stderr=subprocess.STDOUT)
+ check_output([str("pdftocairo"), str("-v")], stderr=subprocess.STDOUT)
@anntzer
anntzer Jan 13, 2017 edited Contributor

Not needed around "-v" (I guess).

lib/matplotlib/backends/backend_pgf.py
"-r %d" % dpi, pdffile, os.path.splitext(pngfile)[0]]
# for some reason this doesn't work without shell
- check_output(" ".join(cmd), shell=True, stderr=subprocess.STDOUT)
+ check_output(cmd, shell=True,
+ stderr=subprocess.STDOUT)
@anntzer
anntzer Jan 13, 2017 Contributor

fits on one line?

@tacaswell
tacaswell Jan 13, 2017 Member

Yeah, this went through some thrashing..

@NelleV
NelleV approved these changes Jan 13, 2017 View changes

LGTM 👍
@anntzer comments are valid, but minor so I am happy merging as is.

@NelleV
Contributor
NelleV commented Jan 13, 2017

There seems to be a PEP8 problem.

@tacaswell
Member

This way by far one of the most frustrating bugs to fix I have fixed in a while...not clear why though 🤷‍♂️ .

@anntzer
Contributor
anntzer commented Jan 13, 2017

Because Py2? :) Anyways, LGTM (after squashing I'd say).

@NelleV NelleV changed the title from BUG: fix checkout_output with non-ascii paths in PATH to [MRG+1] BUG: fix checkout_output with non-ascii paths in PATH Jan 13, 2017
@NelleV NelleV merged commit 58fbfbd into matplotlib:v2.x Jan 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tacaswell tacaswell deleted the tacaswell:fix_py2_subprocess branch Jan 13, 2017
@QuLogic QuLogic changed the title from [MRG+1] BUG: fix checkout_output with non-ascii paths in PATH to BUG: fix checkout_output with non-ascii paths in PATH Jan 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment