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 <arfrever.fta@gmail.com>
Reported-by: Matthias Maier <tamiko@43-1.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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')
         except IOError as e:
             raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname))
         return QAPISchemaParser(fobj, previously_included, info)
@@ -1492,7 +1496,11 @@ class QAPISchemaEvent(QAPISchemaEntity):
 class QAPISchema(object):
     def __init__(self, fname):
         self._fname = fname
-        parser = QAPISchemaParser(open(fname, 'r'))
+        if sys.version_info[0] >= 3:
+            f = open(fname, 'r', encoding='utf-8')
+        else:
+            f = open(fname, 'r')
+        parser = QAPISchemaParser(f)
         exprs = check_exprs(parser.exprs)
         self.docs = parser.docs
         self._entity_list = []
@@ -2006,7 +2014,10 @@ class QAPIGen(object):
                 if e.errno != errno.EEXIST:
                     raise
         fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
-        f = os.fdopen(fd, 'r+')
+        if sys.version_info[0] >= 3:
+            f = open(fd, 'r+', encoding='utf-8')
+        else:
+            f = os.fdopen(fd, 'r+')
         text = (self._top(fname) + self._preamble + self._body
                 + self._bottom(fname))
         oldtext = f.read(len(text) + 1)
-- 
2.17.1

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 <arfrever.fta@gmail.com>
> Reported-by: Matthias Maier <tamiko@43-1.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  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 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.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Acked-by: Eduardo Habkost <ehabkost@redhat.com>

>          except IOError as e:
>              raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname))
>          return QAPISchemaParser(fobj, previously_included, info)
> @@ -1492,7 +1496,11 @@ class QAPISchemaEvent(QAPISchemaEntity):
>  class QAPISchema(object):
>      def __init__(self, fname):
>          self._fname = fname
> -        parser = QAPISchemaParser(open(fname, 'r'))
> +        if sys.version_info[0] >= 3:
> +            f = open(fname, 'r', encoding='utf-8')
> +        else:
> +            f = open(fname, 'r')
> +        parser = QAPISchemaParser(f)
>          exprs = check_exprs(parser.exprs)
>          self.docs = parser.docs
>          self._entity_list = []
> @@ -2006,7 +2014,10 @@ class QAPIGen(object):
>                  if e.errno != errno.EEXIST:
>                      raise
>          fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
> -        f = os.fdopen(fd, 'r+')
> +        if sys.version_info[0] >= 3:
> +            f = open(fd, 'r+', encoding='utf-8')
> +        else:
> +            f = os.fdopen(fd, 'r+')
>          text = (self._top(fname) + self._preamble + self._body
>                  + self._bottom(fname))
>          oldtext = f.read(len(text) + 1)
> -- 
> 2.17.1
> 
> 

-- 
Eduardo

Eduardo Habkost <ehabkost@redhat.com> 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 <arfrever.fta@gmail.com>
>> Reported-by: Matthias Maier <tamiko@43-1.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  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 <ehabkost@redhat.com>
> Acked-by: Eduardo Habkost <ehabkost@redhat.com>

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

On Tue, Jun 19, 2018 at 08:28:08AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> 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 <arfrever.fta@gmail.com>
> >> Reported-by: Matthias Maier <tamiko@43-1.org>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  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.

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.


> 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > Acked-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 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"?

-- 
Eduardo

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Jun 19, 2018 at 08:28:08AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> 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 <arfrever.fta@gmail.com>
>> >> Reported-by: Matthias Maier <tamiko@43-1.org>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  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 <ehabkost@redhat.com>
>> > Acked-by: Eduardo Habkost <ehabkost@redhat.com>
>> 
>> 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.

On Tue, Jun 19, 2018 at 07:50:32AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 19, 2018 at 08:28:08AM +0200, Markus Armbruster wrote:
> > Eduardo Habkost <ehabkost@redhat.com> 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 <arfrever.fta@gmail.com>
> > >> Reported-by: Matthias Maier <tamiko@43-1.org>
> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > >> ---
> > >>  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.
> 
> 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.

Using __future__ doesn't add an build dependancy AFAIK. __future__ is
bundled with core python library rather than being an addon module.

> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > Acked-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > 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"?

Yeah that's what I always thought it meant too.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tue, Jun 19, 2018 at 01:05:10PM +0100, Daniel P. Berrangé wrote:
[...]
> > > > 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.
> > 
> > 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.
> 
> Using __future__ doesn't add an build dependancy AFAIK. __future__ is
> bundled with core python library rather than being an addon module.

The extra dependency is because of the "builtins" module,
provided by python-future in Python 2.7.

-- 
Eduardo

On 06/18/2018 12:59 PM, 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 <arfrever.fta@gmail.com>
> Reported-by: Matthias Maier <tamiko@43-1.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qapi/common.py | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org