Re: [Qemu-devel] [PATCH] tests: Force Python I/O encoding for check-qapi-schema

[Eduardo Habkost]

On Wed, May 08, 2019 at 03:04:43PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
> 
> > On Tue, May 07, 2019 at 03:13:45PM +0100, Daniel P. Berrangé wrote:
> >> On Mon, May 06, 2019 at 06:38:17PM -0300, Eduardo Habkost wrote:
> >> > test-qapi.py doesn't force a specific encoding for stderr or
> >> > stdout, but the reference files used by check-qapi-schema are in
> >> > UTF-8.  This breaks check-qapi-schema under certain circumstances
> >> > (e.g. if using the C locale and Python < 3.7).
> >> > 
> >> > We need to make sure test-qapi.py always generate UTF-8 output
> >> > somehow.  On Python 3.7+ we can do it using
> >> > `sys.stdout.reconfigure(...)`, but we need a solution that works
> >> > with older Python versions.
> >> > 
> >> > Instead of trying a hack like reopening sys.stdout and
> >> > sys.stderr, we can just tell Python to use UTF-8 for I/O encoding
> >> > when running test-qapi.py.  Do it by setting PYTHONIOENCODING.
> >> > 
> >> > Reported-by: Thomas Huth <address@hidden>
> >> > Tested-by: Thomas Huth <address@hidden>
> >> > Signed-off-by: Eduardo Habkost <address@hidden>
> >> > ---
> >> >  tests/Makefile.include | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > 
> >> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> > index 7c8b9c84b2..af88ab6f8b 100644
> >> > --- a/tests/Makefile.include
> >> > +++ b/tests/Makefile.include
> >> > @@ -1103,7 +1103,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) $(SRC_PATH)/tests/qapi-schema/test-qapi.py \
> >> > +                PYTHONIOENCODING=utf-8 $(PYTHON) 
> >> > $(SRC_PATH)/tests/qapi-schema/test-qapi.py \
> >> 
> >> I see PYTHONIOENCODING exists since 2.6 which is nice.
> >> 
> >> How about we actually change $(PYTHON) so that it always includes
> >> PYTHONIOENCODING=utf-8 ?
> >> 
> >> That way we avoid continuing to play whack-a-mole with more utf-8
> >> bugs in future.
> >> 
> >> It would also let us revert this:
> >> 
> >>   commit de685ae5e9a4b523513033bd6cadc8187a227170
> >>   Author: Markus Armbruster <address@hidden>
> >>   Date:   Mon Jun 18 19:59:57 2018 +0200
> >> 
> >>     qapi: Open files with encoding='utf-8'
> >> 
> >> which had to provide separate logic for py2 vs py3 :-(
> 
> The separate logic will soon be history.  I'd welcome getting rid of the
> remainder anyway.

Which remainder?  Do you mean the encoding='utf-8' arguments to
open()?

> 
> > Not every Python script in the QEMU tree is run by our makefiles
> > and scripts using $(PYTHON).  We need to ensure our scripts and
> > modules won't break when run directly from the command line, too.
> > Setting PYTHONIOENCODING everywhere would just hide these bugs
> > from us.
> 
> I agree for Python scripts that are meant to be run that way (assuming
> such scripts exist).  [...]

All scripts inside ./scripts are meant to be run directly from
the command line, aren't they?


>                [...]  For all the others (including all the QAPI-related
> scripts), I'd be quite fine with
> 
> 1. Our build system runs all Python scripts with the
> PYTHONIOENCODING=utf-8
> 
> 2. If you run a Python script yourself, you get to specify the
> PYTHONIOENCODING=utf-8, or use a suitable locale.  Enabling UTF-8 mode
> with PYTHONUTF8=1 or -X utf8 could also work.

I'm OK if we don't actively try to fix those bugs and just expect
people to set PYTHONIOENCODING.  But I don't think we should
reject patches that make the Python code work with non-utf8
locales if it's an easy fix.

-- 
Eduardo