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

[Markus Armbruster]

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.

> Reviewed-by: Eduardo Habkost <address@hidden>
> Acked-by: Eduardo Habkost <address@hidden>

Uh, what does "Acked-by" add over "Reviewed-by"?