Eduardo Habkost <[email protected]> writes: > On Mon, Jun 18, 2018 at 07:59:57PM +0200, Markus Armbruster wrote: >> Python 2 happily reads UTF-8 files in text mode, but Python 3 requires >> either UTF-8 locale or an explicit encoding passed to open(). Commit >> d4e5ec877ca fixed this by setting the en_US.UTF-8 locale. Falls apart >> when the locale isn't be available. >> >> Matthias Maier and Arfrever Frehtes Taifersar Arahesis proposed to use >> binary mode instead, with manual conversion from bytes to str. Works, >> but opening with an explicit encoding is simpler, so do that. >> >> Since Python 2's open() doesn't support the encoding parameter, we >> need to suppress it with a version check. >> >> Reported-by: Arfrever Frehtes Taifersar Arahesis <[email protected]> >> Reported-by: Matthias Maier <[email protected]> >> Signed-off-by: Markus Armbruster <[email protected]> >> --- >> scripts/qapi/common.py | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index 2462fc0291..832f11438a 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -16,6 +16,7 @@ import errno >> import os >> import re >> import string >> +import sys >> from collections import OrderedDict >> >> builtin_types = { >> @@ -340,7 +341,10 @@ class QAPISchemaParser(object): >> return None >> >> try: >> - fobj = open(incl_fname, 'r') >> + if sys.version_info[0] >= 3: >> + fobj = open(incl_fname, 'r', encoding='utf-8') >> + else: >> + fobj = open(incl_fname, 'r') > > I dislike the Python version check, but getting rid of it would > require rewriting the QAPI modules to not use the Python 2 str > type (that has different semantics from Python 3 str type).
The version check is ugly, but it has a property I rather like: when we drop support for Python 2, the conditional becomes True, and partial evaluation results in the Python 3 code we actually want. > The python-future package would help us write code for a single > file/string API instead of two different APIs, but it's not a > QEMU build dependency (yet?), so this patch is good enough for > now. Please do not invest more than absolutely necessary in Python 2 support. All such investment will turn into technical debt in less than two years. If you must invest, pick a solution that will result in less technical debt. We can accept local ugliness for that. In my personal opinion, dumb ideas like supporting Python 2 this close to its EOL ought to look ugly. > Reviewed-by: Eduardo Habkost <[email protected]> > Acked-by: Eduardo Habkost <[email protected]> Uh, what does "Acked-by" add over "Reviewed-by"?
