| Message ID | 20180202130336.24719-10-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Modularize generated QAPI code | expand |
On 02/02/2018 07:03 AM, Markus Armbruster wrote: > Error messages print absolute filenames of included files even gave a s/even gave/even when given/ > relative one on the command line: > > PYTHONPATH=scripts python -B tests/qapi-schema/test-qapi.py tests/qapi-schema/include-cycle.json > In file included from tests/qapi-schema/include-cycle.json:1: > In file included from /work/armbru/qemu/tests/qapi-schema/include-cycle-b.json:1: > /work/armbru/qemu/tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json > > Improve this to > > In file included from tests/qapi-schema/include-cycle.json:1: > In file included from tests/qapi-schema/include-cycle-b.json:1: > tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json Nice, and makes developing new qapi tests a little less painful since it's less modification to qapi-schema/*.err additions. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi/common.py | 12 ++++++------ > tests/qapi-schema/include-no-file.err | 2 +- > 2 files changed, 7 insertions(+), 7 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 02/02/2018 07:03 AM, Markus Armbruster wrote: >> Error messages print absolute filenames of included files even gave a > > s/even gave/even when given/ I meant to write "even if the user gave". Is that okay? >> relative one on the command line: >> >> PYTHONPATH=scripts python -B tests/qapi-schema/test-qapi.py tests/qapi-schema/include-cycle.json >> In file included from tests/qapi-schema/include-cycle.json:1: >> In file included from /work/armbru/qemu/tests/qapi-schema/include-cycle-b.json:1: >> /work/armbru/qemu/tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json >> >> Improve this to >> >> In file included from tests/qapi-schema/include-cycle.json:1: >> In file included from tests/qapi-schema/include-cycle-b.json:1: >> tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json > > Nice, and makes developing new qapi tests a little less painful since > it's less modification to qapi-schema/*.err additions. Probably not, as our make rule strips off $(SRC_PATH): @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err - I've kept that, because it might also occur in stack backtraces. I think. >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> scripts/qapi/common.py | 12 ++++++------ >> tests/qapi-schema/include-no-file.err | 2 +- >> 2 files changed, 7 insertions(+), 7 deletions(-) >> > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster <armbru@redhat.com> wrote: > Error messages print absolute filenames of included files even gave a > relative one on the command line: > > PYTHONPATH=scripts python -B tests/qapi-schema/test-qapi.py tests/qapi-schema/include-cycle.json > In file included from tests/qapi-schema/include-cycle.json:1: > In file included from /work/armbru/qemu/tests/qapi-schema/include-cycle-b.json:1: > /work/armbru/qemu/tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json > > Improve this to > > In file included from tests/qapi-schema/include-cycle.json:1: > In file included from tests/qapi-schema/include-cycle-b.json:1: > tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json > > Signed-off-by: Markus Armbruster <armbru@redhat.com> most of the necessary refactoring/split is done in previous patch, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > scripts/qapi/common.py | 12 ++++++------ > tests/qapi-schema/include-no-file.err | 2 +- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index be0fcc548a..6c6962a364 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -255,9 +255,8 @@ class QAPIDoc(object): > class QAPISchemaParser(object): > > def __init__(self, fp, previously_included=[], incl_info=None): > - abs_fname = os.path.abspath(fp.name) > self.fname = fp.name > - previously_included.append(abs_fname) > + previously_included.append(os.path.abspath(fp.name)) > self.incl_info = incl_info > self.src = fp.read() > if self.src == '' or self.src[-1] != '\n': > @@ -288,7 +287,7 @@ class QAPISchemaParser(object): > if not isinstance(include, str): > raise QAPISemError(info, > "Value of 'include' must be a string") > - self._include(include, info, os.path.dirname(abs_fname), > + self._include(include, info, os.path.dirname(self.fname), > previously_included) > elif "pragma" in expr: > self.reject_expr_doc(cur_doc) > @@ -321,7 +320,8 @@ class QAPISchemaParser(object): > % doc.symbol) > > def _include(self, include, info, base_dir, previously_included): > - incl_abs_fname = os.path.join(base_dir, include) > + incl_fname = os.path.join(base_dir, include) > + incl_abs_fname = os.path.abspath(incl_fname) > # catch inclusion cycle > inf = info > while inf: > @@ -333,9 +333,9 @@ class QAPISchemaParser(object): > if incl_abs_fname in previously_included: > return > try: > - fobj = open(incl_abs_fname, 'r') > + fobj = open(incl_fname, 'r') > except IOError as e: > - raise QAPISemError(info, '%s: %s' % (e.strerror, include)) > + raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname)) > exprs_include = QAPISchemaParser(fobj, previously_included, info) > self.exprs.extend(exprs_include.exprs) > self.docs.extend(exprs_include.docs) > diff --git a/tests/qapi-schema/include-no-file.err b/tests/qapi-schema/include-no-file.err > index d5b9b22d85..e42bcf4bc1 100644 > --- a/tests/qapi-schema/include-no-file.err > +++ b/tests/qapi-schema/include-no-file.err > @@ -1 +1 @@ > -tests/qapi-schema/include-no-file.json:1: No such file or directory: include-no-file-sub.json > +tests/qapi-schema/include-no-file.json:1: No such file or directory: tests/qapi-schema/include-no-file-sub.json > -- > 2.13.6 >
On 02/03/2018 03:08 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 02/02/2018 07:03 AM, Markus Armbruster wrote: >>> Error messages print absolute filenames of included files even gave a >> >> s/even gave/even when given/ > > I meant to write "even if the user gave". Is that okay? Yes, that works too. > >>> relative one on the command line: >>> >>> PYTHONPATH=scripts python -B tests/qapi-schema/test-qapi.py tests/qapi-schema/include-cycle.json >>> In file included from tests/qapi-schema/include-cycle.json:1: >>> In file included from /work/armbru/qemu/tests/qapi-schema/include-cycle-b.json:1: >>> /work/armbru/qemu/tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json >>> >>> Improve this to >>> >>> In file included from tests/qapi-schema/include-cycle.json:1: >>> In file included from tests/qapi-schema/include-cycle-b.json:1: >>> tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json >> >> Nice, and makes developing new qapi tests a little less painful since >> it's less modification to qapi-schema/*.err additions. > > Probably not, as our make rule strips off $(SRC_PATH): > > @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err - > > I've kept that, because it might also occur in stack backtraces. I > think. I still recall having to hand-edit .err files when doing a naive "run the test to get the failure, then 'mv' the bad files into the expected filenames, then rerun the tests"; so I'm not sure if our make rule was properly munging absolute names out of the file in the first place. I'm not too fussed about it, though, as adding new tests is less frequent and it's still fairly easy to learn if you did it right or not by whether 'make check' succeeds. > >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> scripts/qapi/common.py | 12 ++++++------ >>> tests/qapi-schema/include-no-file.err | 2 +- >>> 2 files changed, 7 insertions(+), 7 deletions(-) >>> >> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Thanks! >
Eric Blake <eblake@redhat.com> writes: > On 02/03/2018 03:08 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 02/02/2018 07:03 AM, Markus Armbruster wrote: >>>> Error messages print absolute filenames of included files even gave a >>> >>> s/even gave/even when given/ >> >> I meant to write "even if the user gave". Is that okay? > > Yes, that works too. > >> >>>> relative one on the command line: >>>> >>>> PYTHONPATH=scripts python -B tests/qapi-schema/test-qapi.py tests/qapi-schema/include-cycle.json >>>> In file included from tests/qapi-schema/include-cycle.json:1: >>>> In file included from /work/armbru/qemu/tests/qapi-schema/include-cycle-b.json:1: >>>> /work/armbru/qemu/tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json >>>> >>>> Improve this to >>>> >>>> In file included from tests/qapi-schema/include-cycle.json:1: >>>> In file included from tests/qapi-schema/include-cycle-b.json:1: >>>> tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json >>> >>> Nice, and makes developing new qapi tests a little less painful since >>> it's less modification to qapi-schema/*.err additions. >> >> Probably not, as our make rule strips off $(SRC_PATH): >> >> @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err - >> >> I've kept that, because it might also occur in stack backtraces. I >> think. > > I still recall having to hand-edit .err files when doing a naive "run > the test to get the failure, then 'mv' the bad files into the expected > filenames, then rerun the tests"; Whenever the Perl script does something, you can't simply move the .test.err to .err. Annoying. A make target check-accept could automate the work. However, with this patch, the Perl script should do something less often. I guess that's your point. Sorry for being slow on the update :) > so I'm not sure if our make rule was > properly munging absolute names out of the file in the first place. I'm > not too fussed about it, though, as adding new tests is less frequent > and it's still fairly easy to learn if you did it right or not by > whether 'make check' succeeds. > >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> --- >>>> scripts/qapi/common.py | 12 ++++++------ >>>> tests/qapi-schema/include-no-file.err | 2 +- >>>> 2 files changed, 7 insertions(+), 7 deletions(-) >>>> >>> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> Thanks! >>
On 02/06/2018 01:49 AM, Markus Armbruster wrote: >>> Probably not, as our make rule strips off $(SRC_PATH): >>> >>> @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err - >>> >>> I've kept that, because it might also occur in stack backtraces. I >>> think. Ideally, we don't have any stack backtraces ;) But we have indeed historically checked in .err files with a backtrace that a later patch in the series cleans up, so that part of the munging does indeed play a role in a clean 'make check' for documenting an expected weakness in the generator. >> >> I still recall having to hand-edit .err files when doing a naive "run >> the test to get the failure, then 'mv' the bad files into the expected >> filenames, then rerun the tests"; > > Whenever the Perl script does something, you can't simply move the > .test.err to .err. Annoying. We could also munge the original output prior to creating .test.err; but that also has drawbacks if it eats something that would have been useful for debugging the test. > > A make target check-accept could automate the work. Yeah, but I'm not sure how often we'd need to invoke it, or even remember that it exists. But it would at least avoid the issue of having to debug a munged .test.err. > > However, with this patch, the Perl script should do something less > often. I guess that's your point. > > Sorry for being slow on the update :) No problem; I figure you've got a lot on your mind as you are trying to tie down loose ends before some much deserved time off.
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index be0fcc548a..6c6962a364 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -255,9 +255,8 @@ class QAPIDoc(object): class QAPISchemaParser(object): def __init__(self, fp, previously_included=[], incl_info=None): - abs_fname = os.path.abspath(fp.name) self.fname = fp.name - previously_included.append(abs_fname) + previously_included.append(os.path.abspath(fp.name)) self.incl_info = incl_info self.src = fp.read() if self.src == '' or self.src[-1] != '\n': @@ -288,7 +287,7 @@ class QAPISchemaParser(object): if not isinstance(include, str): raise QAPISemError(info, "Value of 'include' must be a string") - self._include(include, info, os.path.dirname(abs_fname), + self._include(include, info, os.path.dirname(self.fname), previously_included) elif "pragma" in expr: self.reject_expr_doc(cur_doc) @@ -321,7 +320,8 @@ class QAPISchemaParser(object): % doc.symbol) def _include(self, include, info, base_dir, previously_included): - incl_abs_fname = os.path.join(base_dir, include) + incl_fname = os.path.join(base_dir, include) + incl_abs_fname = os.path.abspath(incl_fname) # catch inclusion cycle inf = info while inf: @@ -333,9 +333,9 @@ class QAPISchemaParser(object): if incl_abs_fname in previously_included: return try: - fobj = open(incl_abs_fname, 'r') + fobj = open(incl_fname, 'r') except IOError as e: - raise QAPISemError(info, '%s: %s' % (e.strerror, include)) + raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname)) exprs_include = QAPISchemaParser(fobj, previously_included, info) self.exprs.extend(exprs_include.exprs) self.docs.extend(exprs_include.docs) diff --git a/tests/qapi-schema/include-no-file.err b/tests/qapi-schema/include-no-file.err index d5b9b22d85..e42bcf4bc1 100644 --- a/tests/qapi-schema/include-no-file.err +++ b/tests/qapi-schema/include-no-file.err @@ -1 +1 @@ -tests/qapi-schema/include-no-file.json:1: No such file or directory: include-no-file-sub.json +tests/qapi-schema/include-no-file.json:1: No such file or directory: tests/qapi-schema/include-no-file-sub.json
Error messages print absolute filenames of included files even gave a relative one on the command line: PYTHONPATH=scripts python -B tests/qapi-schema/test-qapi.py tests/qapi-schema/include-cycle.json In file included from tests/qapi-schema/include-cycle.json:1: In file included from /work/armbru/qemu/tests/qapi-schema/include-cycle-b.json:1: /work/armbru/qemu/tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json Improve this to In file included from tests/qapi-schema/include-cycle.json:1: In file included from tests/qapi-schema/include-cycle-b.json:1: tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi/common.py | 12 ++++++------ tests/qapi-schema/include-no-file.err | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-)