Re: [Qemu-devel] [PATCH v4 1/2] qapi: Open files with encoding='utf-8'

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> On Tue, Jun 19, 2018 at 08:28:08AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <address@hidden> 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 <address@hidden>
>> >> Reported-by: Matthias Maier <address@hidden>
>> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >> ---
>> >>  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.
>
> That's the whole point: python-future allows us to not worry
> about Python 2 support in the code anymore because it exposes the
> Python 3 string API (and others) even if we're running Python 2.
>
> After we stop supporting Python 2, we can simply delete the "from
> __future__ import .*" and "from builtins import .*" lines.

You're right, __future__ is one of the least annoying ways to keep
Python 2 working.  But is the improvement over my stupid, ugly solution
worth your while?  You decide.

> Anyway, I will send a RFC series demonstrating that, and then we
> can discuss if it's worth it.  My main worry is not the extra
> imports in Python code, but the introduction of a new build
> dependency only for a few (one?) releases.

The sane extra dependency to add would be Python 3.  Not worth arguing
again; time's on my side ;)

>> > Reviewed-by: Eduardo Habkost <address@hidden>
>> > Acked-by: Eduardo Habkost <address@hidden>
>> 
>> Uh, what does "Acked-by" add over "Reviewed-by"?
>
> It was supposed to indicate that I agree it can be merged through
> other maintainers.  But it looks like this is not part of the
> original definition of "Acked-by"?

I'll drop the Acked-by then.