[Qemu-devel] [PATCH v4 0/2] Fix compilation with python-3 if en_US.UTF-8 is unavailable

[Qemu-devel] [PATCH v4 0/2] Fix compilation with python-3 if en_US.UTF-8 is unavailable

[Markus Armbruster]

v4:
 - Rewrite PATCH 1.

v3:
 - reverse order of patches
 - rename second patch to "Revert commit ..."
 - drop "UTF-8" argument in decode()/encode(); bot functions default to
   utf-8 already.

v2:
 - ensure compatibility with python2 by only calling encode()/decode() if
   script is run with a python3 interpreter

Markus Armbruster (1):
  qapi: Open files with encoding='utf-8'

Matthias Maier (1):
  Revert commit d4e5ec877ca

 Makefile               |  6 ++----
 scripts/qapi/common.py | 17 ++++++++++++++---
 tests/Makefile.include |  6 +++---
 3 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.17.1

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

[Markus Armbruster]

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

[Qemu-devel] [PATCH v4 2/2] Revert commit d4e5ec877ca

[Markus Armbruster]

From: Matthias Maier <tamiko@43-1.org>

This commit removes the PYTHON_UTF8 workaround. The problem with setting

  LC_ALL= LANG=C LC_CTYPE=en_US.UTF-8

is that the en_US.UTF-8 locale might not be available. In this case
setting above locales results in build errors even though another UTF-8
locale was originally set [1]. The only stable way of fixing the
encoding problem is by explicitly annotating encoding/decoding in the
python script.

[1] https://bugs.gentoo.org/657766

Signed-off-by: Arfrever Frehtes Taifersar Arahesis <arfrever.fta@gmail.com>
Signed-off-by: Matthias Maier <tamiko@43-1.org>
Message-Id: <20180615220205.6929-3-tamiko@43-1.org>
---
 Makefile               | 6 ++----
 tests/Makefile.include | 6 +++---
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index e46f2b625a..7ed9cc4a21 100644
--- a/Makefile
+++ b/Makefile
@@ -20,8 +20,6 @@ ifneq ($(wildcard config-host.mak),)
 all:
 include config-host.mak
 
-PYTHON_UTF8 = LC_ALL= LANG=C LC_CTYPE=en_US.UTF-8 $(PYTHON)
-
 git-submodule-update:
 
 .PHONY: git-submodule-update
@@ -576,7 +574,7 @@ qga/qapi-generated/qga-qapi-commands.h qga/qapi-generated/qga-qapi-commands.c \
 qga/qapi-generated/qga-qapi-doc.texi: \
 qga/qapi-generated/qapi-gen-timestamp ;
 qga/qapi-generated/qapi-gen-timestamp: $(SRC_PATH)/qga/qapi-schema.json $(qapi-py)
-	$(call quiet-command,$(PYTHON_UTF8) $(SRC_PATH)/scripts/qapi-gen.py \
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-gen.py \
 		-o qga/qapi-generated -p "qga-" $<, \
 		"GEN","$(@:%-timestamp=%)")
 	@>$@
@@ -676,7 +674,7 @@ qapi/qapi-introspect.h qapi/qapi-introspect.c \
 qapi/qapi-doc.texi: \
 qapi-gen-timestamp ;
 qapi-gen-timestamp: $(qapi-modules) $(qapi-py)
-	$(call quiet-command,$(PYTHON_UTF8) $(SRC_PATH)/scripts/qapi-gen.py \
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-gen.py \
 		-o "qapi" -b $<, \
 		"GEN","$(@:%-timestamp=%)")
 	@>$@
diff --git a/tests/Makefile.include b/tests/Makefile.include
index ca91da26cb..88f1bc1242 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -678,13 +678,13 @@ tests/test-qapi-events.c tests/test-qapi-events.h \
 tests/test-qapi-introspect.c tests/test-qapi-introspect.h: \
 tests/test-qapi-gen-timestamp ;
 tests/test-qapi-gen-timestamp: $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(qapi-py)
-	$(call quiet-command,$(PYTHON_UTF8) $(SRC_PATH)/scripts/qapi-gen.py \
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-gen.py \
 		-o tests -p "test-" $<, \
 		"GEN","$(@:%-timestamp=%)")
 	@>$@
 
 tests/qapi-schema/doc-good.test.texi: $(SRC_PATH)/tests/qapi-schema/doc-good.json $(qapi-py)
-	$(call quiet-command,$(PYTHON_UTF8) $(SRC_PATH)/scripts/qapi-gen.py \
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-gen.py \
 		-o tests/qapi-schema -p "doc-good-" $<, \
 		"GEN","$@")
 	@mv tests/qapi-schema/doc-good-qapi-doc.texi $@
@@ -942,7 +942,7 @@ check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh qemu-img$(EXESUF)
 .PHONY: $(patsubst %, check-%, $(check-qapi-schema-y))
 $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: $(SRC_PATH)/%.json
 	$(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts \
-		$(PYTHON_UTF8) $(SRC_PATH)/tests/qapi-schema/test-qapi.py \
+		$(PYTHON) $(SRC_PATH)/tests/qapi-schema/test-qapi.py \
 		$^ >$*.test.out 2>$*.test.err; \
 		echo $$? >$*.test.exit, \
 		"TEST","$*.out")
-- 
2.17.1

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

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH v4 2/2] Revert commit d4e5ec877ca

[Eduardo Habkost]

On Mon, Jun 18, 2018 at 07:59:58PM +0200, Markus Armbruster wrote:
> From: Matthias Maier <tamiko@43-1.org>
> 
> This commit removes the PYTHON_UTF8 workaround. The problem with setting
> 
>   LC_ALL= LANG=C LC_CTYPE=en_US.UTF-8
> 
> is that the en_US.UTF-8 locale might not be available. In this case
> setting above locales results in build errors even though another UTF-8
> locale was originally set [1]. The only stable way of fixing the
> encoding problem is by explicitly annotating encoding/decoding in the
> python script.
> 
> [1] https://bugs.gentoo.org/657766
> 
> Signed-off-by: Arfrever Frehtes Taifersar Arahesis <arfrever.fta@gmail.com>
> Signed-off-by: Matthias Maier <tamiko@43-1.org>
> Message-Id: <20180615220205.6929-3-tamiko@43-1.org>

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

-- 
Eduardo

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

[Markus Armbruster]

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"?

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

[Eduardo Habkost]

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

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

[Eric Blake]

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

Re: [Qemu-devel] [PATCH v4 2/2] Revert commit d4e5ec877ca

[Eric Blake]

On 06/18/2018 12:59 PM, Markus Armbruster wrote:
> From: Matthias Maier <tamiko@43-1.org>
> 
> This commit removes the PYTHON_UTF8 workaround. The problem with setting
> 
>    LC_ALL= LANG=C LC_CTYPE=en_US.UTF-8
> 
> is that the en_US.UTF-8 locale might not be available. In this case
> setting above locales results in build errors even though another UTF-8
> locale was originally set [1]. The only stable way of fixing the
> encoding problem is by explicitly annotating encoding/decoding in the
> python script.
> 
> [1] https://bugs.gentoo.org/657766
> 
> Signed-off-by: Arfrever Frehtes Taifersar Arahesis <arfrever.fta@gmail.com>
> Signed-off-by: Matthias Maier <tamiko@43-1.org>
> Message-Id: <20180615220205.6929-3-tamiko@43-1.org>
> ---

Markus, does this need S-o-b from you?  (Then again, if it goes in 
through your tree, that's a moot point, as you'd add it as maintainer)

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

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

Re: [Qemu-devel] [PATCH v4 2/2] Revert commit d4e5ec877ca

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/18/2018 12:59 PM, Markus Armbruster wrote:
>> From: Matthias Maier <tamiko@43-1.org>
>>
>> This commit removes the PYTHON_UTF8 workaround. The problem with setting
>>
>>    LC_ALL= LANG=C LC_CTYPE=en_US.UTF-8
>>
>> is that the en_US.UTF-8 locale might not be available. In this case
>> setting above locales results in build errors even though another UTF-8
>> locale was originally set [1]. The only stable way of fixing the
>> encoding problem is by explicitly annotating encoding/decoding in the
>> python script.
>>
>> [1] https://bugs.gentoo.org/657766
>>
>> Signed-off-by: Arfrever Frehtes Taifersar Arahesis <arfrever.fta@gmail.com>
>> Signed-off-by: Matthias Maier <tamiko@43-1.org>
>> Message-Id: <20180615220205.6929-3-tamiko@43-1.org>
>> ---
>
> Markus, does this need S-o-b from you?  (Then again, if it goes in
> through your tree, that's a moot point, as you'd add it as maintainer)

I intend to take it through my tree.

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

Thanks!

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

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH v4 2/2] Revert commit d4e5ec877ca

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> From: Matthias Maier <tamiko@43-1.org>
>
> This commit removes the PYTHON_UTF8 workaround. The problem with setting
>
>   LC_ALL= LANG=C LC_CTYPE=en_US.UTF-8
>
> is that the en_US.UTF-8 locale might not be available. In this case
> setting above locales results in build errors even though another UTF-8
> locale was originally set [1]. The only stable way of fixing the
> encoding problem is by explicitly annotating encoding/decoding in the
> python script.

I'd like to tweak the last sentence to

    The only stable way of fixing the encoding problem is by specifying
    the encoding in Python, like the previous commit does.

if there are no objections.

>
> [1] https://bugs.gentoo.org/657766
>
> Signed-off-by: Arfrever Frehtes Taifersar Arahesis <arfrever.fta@gmail.com>
> Signed-off-by: Matthias Maier <tamiko@43-1.org>
> Message-Id: <20180615220205.6929-3-tamiko@43-1.org>

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

[Daniel P. Berrangé]

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 :|

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

[Eduardo Habkost]

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