[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 8/9] scripts/qapi-gen: add -i option
|
From: |
Marc-André Lureau |
|
Subject: |
Re: [PATCH 8/9] scripts/qapi-gen: add -i option |
|
Date: |
Thu, 23 Jun 2022 12:10:11 +0400 |
Hi
On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Replace hard-coded "qemu/osdep.h" include with a qapi-gen option to
> > specify the headers to include. This will allow to substitute QEMU
> > osdep.h with glib.h for example, for projects with different
> > global headers.
> >
> > For historical reasons, we can keep the default as "qemu/osdep.h".
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> I wish we'd all agree on "config.h" (conventional with autoconf). But
> we don't.
>
> Precedence for tweaking generated code with command line options: -p.
>
> I suspect that we'll always specify the same -p and -i for a given
> schema. To me, that suggests they should both go into the schema
> instead. Observation, not demand.
>
> > ---
> > scripts/qapi/commands.py | 15 ++++++++++-----
> > scripts/qapi/events.py | 17 +++++++++++------
> > scripts/qapi/gen.py | 17 +++++++++++++++++
> > scripts/qapi/introspect.py | 11 +++++++----
> > scripts/qapi/main.py | 17 +++++++++++------
> > scripts/qapi/types.py | 17 +++++++++++------
> > scripts/qapi/visit.py | 17 +++++++++++------
> > 7 files changed, 78 insertions(+), 33 deletions(-)
> >
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index 38ca38a7b9dd..781491b6390d 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -294,9 +294,9 @@ def gen_register_command(name: str,
> >
> >
> > class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> > - def __init__(self, prefix: str, gen_tracing: bool):
> > + def __init__(self, prefix: str, include: List[str], gen_tracing: bool):
>
> Ignorant question: why ist this List[str], not str? Do multiple options
> accumulate into a list?
>
> Alright, I'm back from further down: looks like they do.
>
> > super().__init__(
> > - prefix, 'qapi-commands',
> > + prefix, include, 'qapi-commands',
> > ' * Schema-defined QAPI/QMP commands', None, __doc__,
> > gen_tracing=gen_tracing)
> > self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> > @@ -308,7 +308,8 @@ def _begin_user_module(self, name: str) -> None:
> > types = self._module_basename('qapi-types', name)
> > visit = self._module_basename('qapi-visit', name)
> > self._genc.add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "qapi/compat-policy.h"
> > #include "qapi/visitor.h"
> > #include "qapi/qmp/qdict.h"
> > @@ -318,6 +319,7 @@ def _begin_user_module(self, name: str) -> None:
> > #include "%(commands)s.h"
> >
> > ''',
> > + include=self.genc_include(),
> > commands=commands, visit=visit))
> >
> > if self._gen_tracing and commands != 'qapi-commands':
> > @@ -344,7 +346,8 @@ def visit_begin(self, schema: QAPISchema) -> None:
> > ''',
> > c_prefix=c_name(self._prefix, protect=False)))
> > self._genc.add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "%(prefix)sqapi-commands.h"
> > #include "%(prefix)sqapi-init-commands.h"
> >
> > @@ -353,6 +356,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
> > QTAILQ_INIT(cmds);
> >
> > ''',
> > + include=self.genc_include(),
> > prefix=self._prefix,
> > c_prefix=c_name(self._prefix, protect=False)))
> >
> > @@ -404,7 +408,8 @@ def visit_command(self,
> > def gen_commands(schema: QAPISchema,
> > output_dir: str,
> > prefix: str,
> > + include: List[str],
> > gen_tracing: bool) -> None:
> > - vis = QAPISchemaGenCommandVisitor(prefix, gen_tracing)
> > + vis = QAPISchemaGenCommandVisitor(prefix, include, gen_tracing)
> > schema.visit(vis)
> > vis.write(output_dir)
> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > index 27b44c49f5e9..6e677d11d2e0 100644
> > --- a/scripts/qapi/events.py
> > +++ b/scripts/qapi/events.py
> > @@ -175,9 +175,9 @@ def gen_event_send(name: str,
> >
> > class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
> >
> > - def __init__(self, prefix: str):
> > + def __init__(self, prefix: str, include: List[str]):
> > super().__init__(
> > - prefix, 'qapi-events',
> > + prefix, include, 'qapi-events',
> > ' * Schema-defined QAPI/QMP events', None, __doc__)
> > self._event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
> > self._event_enum_members: List[QAPISchemaEnumMember] = []
> > @@ -188,7 +188,8 @@ def _begin_user_module(self, name: str) -> None:
> > types = self._module_basename('qapi-types', name)
> > visit = self._module_basename('qapi-visit', name)
> > self._genc.add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "%(prefix)sqapi-emit-events.h"
> > #include "%(events)s.h"
> > #include "%(visit)s.h"
> > @@ -198,6 +199,7 @@ def _begin_user_module(self, name: str) -> None:
> > #include "qapi/qmp-event.h"
> >
> > ''',
> > + include=self.genc_include(),
> > events=events, visit=visit,
> > prefix=self._prefix))
> > self._genh.add(mcgen('''
> > @@ -209,9 +211,11 @@ def _begin_user_module(self, name: str) -> None:
> > def visit_end(self) -> None:
> > self._add_module('./emit', ' * QAPI Events emission')
> > self._genc.preamble_add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "%(prefix)sqapi-emit-events.h"
> > ''',
> > + include=self.genc_include(),
> > prefix=self._prefix))
> > self._genh.preamble_add(mcgen('''
> > #include "qapi/util.h"
> > @@ -246,7 +250,8 @@ def visit_event(self,
> >
> > def gen_events(schema: QAPISchema,
> > output_dir: str,
> > - prefix: str) -> None:
> > - vis = QAPISchemaGenEventVisitor(prefix)
> > + prefix: str,
> > + include: List[str]) -> None:
> > + vis = QAPISchemaGenEventVisitor(prefix, include)
> > schema.visit(vis)
> > vis.write(output_dir)
> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > index 113b49134de4..54a70a5ff516 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -17,6 +17,7 @@
> > from typing import (
> > Dict,
> > Iterator,
> > + List,
> > Optional,
> > Sequence,
> > Tuple,
> > @@ -45,6 +46,12 @@ def gen_special_features(features:
> > Sequence[QAPISchemaFeature]) -> str:
> > return ' | '.join(special_features) or '0'
> >
> >
> > +def genc_include(include: List[str]) -> str:
> > + return '\n'.join(['#include ' +
> > + (f'"{inc}"' if inc[0] not in ('<', '"') else inc)
> > + for inc in include])
>
> This maps a list of file names / #include arguments to C code including
> them, mapping file names to #include arguments by enclosing them in "".
>
> Option arguments of the form <foo.h> and "foo.h" need to be quoted in
> the shell. The latter can be done without quoting as foo.h.
>
> Somewhat funky wedding of generality with convenience.
>
> > +
> > +
> > class QAPIGen:
> > def __init__(self, fname: str):
> > self.fname = fname
> > @@ -228,16 +235,21 @@ def ifcontext(ifcond: QAPISchemaIfCond, *args:
> > QAPIGenCCode) -> Iterator[None]:
> > class QAPISchemaMonolithicCVisitor(QAPISchemaVisitor):
> > def __init__(self,
> > prefix: str,
> > + include: List[str],
> > what: str,
> > blurb: str,
> > pydoc: str):
> > self._prefix = prefix
> > + self._include = include
> > self._what = what
> > self._genc = QAPIGenC(self._prefix + self._what + '.c',
> > blurb, pydoc)
> > self._genh = QAPIGenH(self._prefix + self._what + '.h',
> > blurb, pydoc)
> >
> > + def genc_include(self) -> str:
> > + return genc_include(self._include)
> > +
> > def write(self, output_dir: str) -> None:
> > self._genc.write(output_dir)
> > self._genh.write(output_dir)
> > @@ -246,12 +258,14 @@ def write(self, output_dir: str) -> None:
> > class QAPISchemaModularCVisitor(QAPISchemaVisitor):
> > def __init__(self,
> > prefix: str,
> > + include: List[str],
> > what: str,
> > user_blurb: str,
> > builtin_blurb: Optional[str],
> > pydoc: str,
> > gen_tracing: bool = False):
> > self._prefix = prefix
> > + self._include = include
> > self._what = what
> > self._user_blurb = user_blurb
> > self._builtin_blurb = builtin_blurb
> > @@ -262,6 +276,9 @@ def __init__(self,
> > self._main_module: Optional[str] = None
> > self._gen_tracing = gen_tracing
> >
> > + def genc_include(self) -> str:
> > + return genc_include(self._include)
> > +
> > @property
> > def _genc(self) -> QAPIGenC:
> > assert self._current_module is not None
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index 67c7d89aae00..d965d1769447 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -170,9 +170,9 @@ def to_c_string(string: str) -> str:
> >
> > class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
> >
> > - def __init__(self, prefix: str, unmask: bool):
> > + def __init__(self, prefix: str, include: List[str], unmask: bool):
> > super().__init__(
> > - prefix, 'qapi-introspect',
> > + prefix, include, 'qapi-introspect',
> > ' * QAPI/QMP schema introspection', __doc__)
> > self._unmask = unmask
> > self._schema: Optional[QAPISchema] = None
> > @@ -180,10 +180,12 @@ def __init__(self, prefix: str, unmask: bool):
> > self._used_types: List[QAPISchemaType] = []
> > self._name_map: Dict[str, str] = {}
> > self._genc.add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "%(prefix)sqapi-introspect.h"
> >
> > ''',
> > + include=self.genc_include(),
> > prefix=prefix))
> >
> > def visit_begin(self, schema: QAPISchema) -> None:
> > @@ -384,7 +386,8 @@ def visit_event(self, name: str, info:
> > Optional[QAPISourceInfo],
> >
> >
> > def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
> > + include: List[str],
> > opt_unmask: bool) -> None:
> > - vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
> > + vis = QAPISchemaGenIntrospectVisitor(prefix, include, opt_unmask)
> > schema.visit(vis)
> > vis.write(output_dir)
> > diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> > index fc216a53d32a..eba98cb9ace2 100644
> > --- a/scripts/qapi/main.py
> > +++ b/scripts/qapi/main.py
> > @@ -9,7 +9,7 @@
> >
> > import argparse
> > import sys
> > -from typing import Optional
> > +from typing import List, Optional
> >
> > from .commands import gen_commands
> > from .common import must_match
> > @@ -31,6 +31,7 @@ def invalid_prefix_char(prefix: str) -> Optional[str]:
> > def generate(schema_file: str,
> > output_dir: str,
> > prefix: str,
> > + include: List[str],
> > unmask: bool = False,
> > builtins: bool = False,
> > gen_tracing: bool = False) -> None:
> > @@ -48,11 +49,11 @@ def generate(schema_file: str,
> > assert invalid_prefix_char(prefix) is None
> >
> > schema = QAPISchema(schema_file)
> > - gen_types(schema, output_dir, prefix, builtins)
> > - gen_visit(schema, output_dir, prefix, builtins)
> > - gen_commands(schema, output_dir, prefix, gen_tracing)
> > - gen_events(schema, output_dir, prefix)
> > - gen_introspect(schema, output_dir, prefix, unmask)
> > + gen_types(schema, output_dir, prefix, include, builtins)
> > + gen_visit(schema, output_dir, prefix, include, builtins)
> > + gen_commands(schema, output_dir, prefix, include, gen_tracing)
> > + gen_events(schema, output_dir, prefix, include)
> > + gen_introspect(schema, output_dir, prefix, include, unmask)
> >
> >
> > def main() -> int:
> > @@ -75,6 +76,9 @@ def main() -> int:
> > parser.add_argument('-u', '--unmask-non-abi-names',
> > action='store_true',
> > dest='unmask',
> > help="expose non-ABI names in introspection")
> > + parser.add_argument('-i', '--include', nargs='*',
> > + default=['qemu/osdep.h'],
> > + help="top-level include headers")
>
> The option name --include doesn't really tell me what it is about. Is
> it an include path for schema files? Or is it about including something
> in generated C? Where in generated C?
>
> The help text provides clues: "headers" suggests .h, and "top-level"
> suggests somewhere top-like.
>
> In fact, it's about inserting C code at the beginning of generated .c
> files. For the uses we have in mind, the C code is a single #include.
> The patch implements any number of #includes.
>
> More general and arguably less funky: a way to insert arbitrary C code.
>
> Except arbitrary C code on the command line is unwieldy. Better kept it
> in the schema. Pragma?
>
> Thoughts?
Pragmas are global currently. This doesn't scale well, as we would
like to split the schemas. I have a following patch that will allow me
to split/merge the pragmas. This is not optimal either, I would rather
remove/replace them (using annotations).
Imho, global tweaking of compilation is better done from the command line.
>
> >
> > # Option --suppress-tracing exists so we can avoid solving build system
> > # problems. TODO Drop it when we no longer need it.
> > @@ -94,6 +98,7 @@ def main() -> int:
> > generate(args.schema,
> > output_dir=args.output_dir,
> > prefix=args.prefix,
> > + include=args.include,
> > unmask=args.unmask,
> > builtins=args.builtins,
> > gen_tracing=not args.suppress_tracing)
> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > index 477d02700137..9617b7d4edfa 100644
> > --- a/scripts/qapi/types.py
> > +++ b/scripts/qapi/types.py
> > @@ -282,18 +282,20 @@ def gen_type_cleanup(name: str) -> str:
> >
> > class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
> >
> > - def __init__(self, prefix: str):
> > + def __init__(self, prefix: str, include: List[str]):
> > super().__init__(
> > - prefix, 'qapi-types', ' * Schema-defined QAPI types',
> > + prefix, include, 'qapi-types', ' * Schema-defined QAPI types',
> > ' * Built-in QAPI types', __doc__)
> >
> > def _begin_builtin_module(self) -> None:
> > self._genc.preamble_add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "qapi/dealloc-visitor.h"
> > #include "qapi/qapi-builtin-types.h"
> > #include "qapi/qapi-builtin-visit.h"
> > -'''))
> > +''',
> > + include=self.genc_include()))
> > self._genh.preamble_add(mcgen('''
> > #include "qapi/util.h"
> > '''))
> > @@ -302,11 +304,13 @@ def _begin_user_module(self, name: str) -> None:
> > types = self._module_basename('qapi-types', name)
> > visit = self._module_basename('qapi-visit', name)
> > self._genc.preamble_add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "qapi/dealloc-visitor.h"
> > #include "%(types)s.h"
> > #include "%(visit)s.h"
> > ''',
> > + include=self.genc_include(),
> > types=types, visit=visit))
> > self._genh.preamble_add(mcgen('''
> > #include "qapi/qapi-builtin-types.h"
> > @@ -381,7 +385,8 @@ def visit_alternate_type(self,
> > def gen_types(schema: QAPISchema,
> > output_dir: str,
> > prefix: str,
> > + include: List[str],
> > opt_builtins: bool) -> None:
> > - vis = QAPISchemaGenTypeVisitor(prefix)
> > + vis = QAPISchemaGenTypeVisitor(prefix, include)
> > schema.visit(vis)
> > vis.write(output_dir, opt_builtins)
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 380fa197f589..1ff464c0360f 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -318,17 +318,19 @@ def gen_visit_object(name: str) -> str:
> >
> > class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
> >
> > - def __init__(self, prefix: str):
> > + def __init__(self, prefix: str, include: List[str]):
> > super().__init__(
> > - prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
> > + prefix, include, 'qapi-visit', ' * Schema-defined QAPI
> > visitors',
> > ' * Built-in QAPI visitors', __doc__)
> >
> > def _begin_builtin_module(self) -> None:
> > self._genc.preamble_add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "qapi/error.h"
> > #include "qapi/qapi-builtin-visit.h"
> > -'''))
> > +''',
> > + include=self.genc_include()))
> > self._genh.preamble_add(mcgen('''
> > #include "qapi/visitor.h"
> > #include "qapi/qapi-builtin-types.h"
> > @@ -339,11 +341,13 @@ def _begin_user_module(self, name: str) -> None:
> > types = self._module_basename('qapi-types', name)
> > visit = self._module_basename('qapi-visit', name)
> > self._genc.preamble_add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "qapi/error.h"
> > #include "qapi/qmp/qerror.h"
> > #include "%(visit)s.h"
> > ''',
> > + include=self.genc_include(),
> > visit=visit))
> > self._genh.preamble_add(mcgen('''
> > #include "qapi/qapi-builtin-visit.h"
> > @@ -408,7 +412,8 @@ def visit_alternate_type(self,
> > def gen_visit(schema: QAPISchema,
> > output_dir: str,
> > prefix: str,
> > + include: List[str],
> > opt_builtins: bool) -> None:
> > - vis = QAPISchemaGenVisitVisitor(prefix)
> > + vis = QAPISchemaGenVisitVisitor(prefix, include)
> > schema.visit(vis)
> > vis.write(output_dir, opt_builtins)
>
> Patch is mostly plumbing. Looks reasonable.
>
- [PATCH 4/9] error-report: simplify print_loc(), (continued)
- [PATCH 4/9] error-report: simplify print_loc(), marcandre . lureau, 2022/06/16
- [PATCH 6/9] error-report: add a callback to overwrite error_vprintf, marcandre . lureau, 2022/06/16
- [PATCH 5/9] error-report: introduce ErrorReportDetailedFunc, marcandre . lureau, 2022/06/16
- [PATCH 7/9] qapi: move QEMU-specific dispatch code in monitor, marcandre . lureau, 2022/06/16
- [PATCH 9/9] scripts/qapi: add required system includes to visitor, marcandre . lureau, 2022/06/16
- [PATCH 8/9] scripts/qapi-gen: add -i option, marcandre . lureau, 2022/06/16
- Re: [PATCH 0/9] Preliminary patches for subproject split, Paolo Bonzini, 2022/06/16
- Re: [PATCH 0/9] Preliminary patches for subproject split, Marc-André Lureau, 2022/06/28