py3: fixes for encoding in subprocess.Popen in sage.interfaces

[embray]

The easiest way I've found for dealing with interfaces that use subprocess.Popen is, on Python 3, to simply specify an encoding for the subprocess I/O. Unfortunately this argument doesn't exist on Python 2 (a wrapper for this might be nice but the examples are few enough that I haven't been compelled to bother).

Component: python3

Author: Erik Bray

Branch/Commit: bd63b99

Reviewer: Julian Rüth, Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/24834

[jdemeyer]

comment:2

Why latin-1? That's a strange choice.

[embray]

comment:3

As usual, I explained in my commit messages:

In these cases we go with latin-1, because the interface parsing does not anticipate any non-ASCII output from these programs, but we also allow any non-ASCII garbage that might come out (in which case any exceptions will occur in output parsing, not decoding).

[embray]

comment:4

The reason I might use latin-1 here, but not in some other interfaces, is that these are just very simple programs that output some ASCII text, and aren't likely to involve non-ASCII text. I could be wrong there but looking over how they work it seems unlikely. This is true for most of the interfaces actually, but not quite all (e.g. some could produce error messages that contain system errors in whatever the system's language is, but not so here).

[saraedum]

comment:5

I think it would be nice if you added the comment about latin-1 in the source code where you use it. Sure, it's in the commit history, but once we drop Python 2 support (and remove the if clause,) it will not be the commit message for the last change in that line…and it's certainly something that is very suprising to read there.

Once this is documented, I am very happy to give this a positive review.

[saraedum]

Reviewer: Julian Rüth

[fchapoton]

comment:8

branch does not apply, needs rebase

[sagetrac-git]

Changed commit from be79909 to 88269e0

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6d26d45	py3: in ecm and gfan interfaces, on Python 3 specify an encoding for Popen
6a5cf4f	minor PEP-8 / whitespace cleanup
88269e0	py3: fix a few tests that used subprocess.Popen so that they supply an encoding on Python 3

[fchapoton]

comment:10

Is this "needs_review" ? Looks good to me. Could you please fix the pyflakes plugin complaint ?

[embray]

comment:11

There was some comment about leaving a comment as to why I used latin-1 in one case, but I don't think it's necessary...

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

b6c9be9

remove unused import noted by pyflakes

[sagetrac-git]

Changed commit from 88269e0 to b6c9be9

[sagetrac-git]

Changed commit from b6c9be9 to bd63b99

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

bd63b99

why latin-1 (I think it's a good choice in general for wrapping simple programs that are not locale-aware and only produce ASCII text)

[fchapoton]

comment:15

ok, let it be

[fchapoton]

Changed reviewer from Julian Rüth to Julian Rüth, Frédéric Chapoton

[vbraun]

Changed branch from u/embray/python3/interfaces/popen-encoding to bd63b99