| Message ID | 20190506213817.14344-1-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests: Force Python I/O encoding for check-qapi-schema | expand |
On 5/6/19 11:38 PM, 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 <thuth@redhat.com> > Tested-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > 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 \ > $^ >$*.test.out 2>$*.test.err; \ > echo $$? >$*.test.exit, \ > "TEST","$*.out") >
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 <thuth@redhat.com> > Tested-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > 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 <armbru@redhat.com> 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 :-( Regards, Daniel
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 <thuth@redhat.com> > > Tested-by: Thomas Huth <thuth@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > 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 <armbru@redhat.com> > 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 :-( 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.
Eduardo Habkost <ehabkost@redhat.com> writes: > 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 <thuth@redhat.com> > Tested-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Tested-by: Alex Bennée <alex.bennee@linaro.org> > --- > 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 \ > $^ >$*.test.out 2>$*.test.err; \ > echo $$? >$*.test.exit, \ > "TEST","$*.out") -- Alex Bennée
Eduardo Habkost <ehabkost@redhat.com> 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 <thuth@redhat.com> >> > Tested-by: Thomas Huth <thuth@redhat.com> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> > --- >> > 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 <armbru@redhat.com> >> 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. > 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). 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.
On Wed, May 08, 2019 at 03:04:43PM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> 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 <thuth@redhat.com> > >> > Tested-by: Thomas Huth <thuth@redhat.com> > >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >> > --- > >> > 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 <armbru@redhat.com> > >> 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 Habkost <ehabkost@redhat.com> writes: > On Wed, May 08, 2019 at 03:04:43PM +0200, Markus Armbruster wrote: >> Eduardo Habkost <ehabkost@redhat.com> 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 <thuth@redhat.com> >> >> > Tested-by: Thomas Huth <thuth@redhat.com> >> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> >> > --- >> >> > 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 <armbru@redhat.com> >> >> 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()? I'd welcome a revert the whole commit. >> > 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? Counter-example: you're welcome to run scripts/qapi-gen.py by hand for whatever purpose, but if you mess up your build that way, you're on your own. >> [...] 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. I'm not going to interfere with easy fixes to code I don't maintain :)
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 \ $^ >$*.test.out 2>$*.test.err; \ echo $$? >$*.test.exit, \ "TEST","$*.out")