From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, Eric Blake <eblake@redhat.com>,
Cleber Rosa <crosa@redhat.com>,
qemu-block@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
Xie Yongji <xieyongji@bytedance.com>,
Kyle Evans <kevans@freebsd.org>,
Peter Maydell <peter.maydell@linaro.org>,
John Snow <jsnow@redhat.com>,
Michael Roth <michael.roth@amd.com>, Warner Losh <imp@bsdimp.com>,
Kevin Wolf <kwolf@redhat.com>,
"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>,
Laurent Vivier <laurent@vivier.eu>, Fam Zheng <fam@euphon.net>,
Hanna Reitz <hreitz@redhat.com>
Subject: Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
Date: Thu, 11 Aug 2022 14:09:24 +0400 [thread overview]
Message-ID: <CAJ+F1CKbkTOX7Fh9RvkBvuW_gZqZjYSta=7nEKbzm-OefPE_GQ@mail.gmail.com> (raw)
In-Reply-To: <87o7wr5ew9.fsf@pond.sub.org>
[-- Attachment #1: Type: text/plain, Size: 6442 bytes --]
On Thu, Aug 11, 2022 at 1:05 PM Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi
> >
> > On Thu, Aug 11, 2022 at 10:52 AM Markus Armbruster <armbru@redhat.com>
> > wrote:
> >
> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> >>
> >> > Hi
> >> >
> >> > On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster <armbru@redhat.com>
> >> wrote:
> >> >
> >> >> marcandre.lureau@redhat.com writes:
> >> >>
> >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> >
> >> >> > This is just moving qapi-gen.py and related subdir to qemu-common,
> to
> >> >> > ease review and proceed step by step. The following patches will
> move
> >> >> > related necessary code, tests etc.
> >> >> >
> >> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >>
> >> >> As moved files tend to become low-level annoyances for a long time,
> I'd
> >> >> like to understand why you want to move them. The commit message
> says
> >> >> "to ease review", which I suspect isn't the real reason. Perhaps you
> >> >> explained all that elsewhere already, but I missed it.
> >> >>
> >> >>
> >> >>
> >> > The end goal is to split some projects, such as qemu-ga, to standalone
> >> > meson projects/subprojects. We will be able to build them
> independently
> >> > from the rest of QEMU, and later on perhaps handle them outside of
> QEMU
> >> > main repository. To achieve this, I first introduce a qemu-common
> >> > subproject, where qapi and common units are provided. You can check
> >> > https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak
> peek at
> >> > current end result.
> >>
> >> I worry this move of the QAPI generator code into
> >> subjprojects/common/scripts/qapi/ will be followed by a move into its
> >> own subproject.
> >>
> >
> > Do you mean: it could be moved again to another smaller subproject? not
> > really, see below
> >
> >
> >> Ignorant question: could we turn the QAPI generator into a subproject in
> >> place?
> >>
> >
> > If it's just the generator, probably the target would then be a python
> > project (not meson), similar to python-qemu-qmp.
> >
> > But I don't see much point, since it's not really a standalone python
> > module, it generates code, and that code needs most of what is in
> > qemu-common (see
> >
> https://gitlab.com/marcandre.lureau/qemu/-/tree/qga/subprojects/qemu-common
> ).
> > It's best to have it together imho. Maybe we can consider a different
> > naming or to be more careful not to add stuff that is not strictly needed
> > by qapi?
>
> I had a look at subjprojects/qemu-common in your qga branch. Contents:
>
> * Subproject machinery
>
> * Some common headers (glib-compat.h), but not others (qemu/osdep.h). I
> guess it's whatever subjproject code needs.
>
> Is subprojects/qemu-common/include/block/nvme.h there by accident?
>
It's a header shared with qemu-ga (the commit message explains)
>
> * Most of the QObject subsystem
>
> qobject/block-qdict.c is left behind.
>
It's qemu block specific. Not needed to move at this point, it drags other
stuff iirc.
> * Most of the QAPI subsystem
>
> Some visitors left behind: opts, forward, string input / output. Hmm,
> only the .c, the .h are in the subjproject. Accident?
>
If they are not shared with qemu-ga, I didn't move them yet. They can stay
specific to qemu or specific subprojects, or we can decide to move them (if
that doesn't drag too much stuff along).
>
> A bit of HMP support left behind.
>
Can you be more specific?
>
> * Parts of util/ and include/qemu/
>
> Error reporting, key-value CLI, some C utilities, but not others
> (e.g. qemu/atomic.h, but not qemu/atomic128.h). I guess it's again
> whatever subjproject code needs.
>
>
* Parts of the QAPI Schema subsystem
>
> Aside: MAINTAINERS mostly not updated.
>
>
That needs fixing.
> Your moves tear closely related code apart. This is going to be a drag
> for developers in general and maintainers in particular.
>
> Ergonomics suffer when related code is in multiple places. Having to
> switch between directories and remember where is what will a constant
> low-level pain. Things that used to be simple & quick, like git-grep
> qapi/*.c, become more involved.
>
>
It's inevitable when a project grows. QEMU is already a very large project.
Over the years, we have structured the project, by moving things and
splitting in subdirectories. Imho, this is actually helpful in many ways,
and we got used to it easily hopefully.
Do you see fundamental reasons why qemu-ga or (qemu-img, qemu-nbd,
storage-daemon, virtiofsd, vhost-user-*, *helper, ivshmem* etc) need to be
tight to qemu code, release and management process? I am not saying it's
time to move them out of qemu project, but I believe it's helpful to have
code that is structured and can be compiled indepently.
And by having "standalone" subprojects, we can more easily evolve in new
directions, without touching the rest of the projects.
Hurts even when merely consuming the subsystem: when I see #include
> "qemu/foo.h", the straightforward include/qemu/foo.h may or may not do.
> When it doesn't, I need to know where to look instead.
>
> subprojects/qemu-common/include/ is a lot to type. Sufficiently
> powerful editors mitigate, but not completely.
>
> When changes need to be applied to every instance of an abstraction,
> it's easy to miss instances "elsewhere". There's a reason the QAPI
> visitors are all in one place.
>
I understand it's nice to have all the code in one place. At the same time,
this goes against modularity and composability..
>
> The actual split seems somewhat arbitrary in places. I suspect more
> code will move over time. Invalidating "what is where" knowledge.
>
Are you insinuating that this is obvious today? :) By having smaller
standalone projects, we have better defined scope, test range, reusability
etc. And we avoid creating a jam of dependencies, making code review &
change a bit easier..
> I believe a serious think about other ways to accomplish your goals is
> called for.
>
I don't claim to have the perfect solution. It's evolution, hopefully a
step in a better direction.
thanks
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 9906 bytes --]
next prev parent reply other threads:[~2022-08-11 10:11 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 01/15] error-report: misc comment fix marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 02/15] error-report: introduce "detailed" variable marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 03/15] error-report: simplify print_loc() marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 04/15] error-report: introduce overridable error_is_detailed() marcandre.lureau
2022-07-12 15:02 ` Warner Losh
2022-07-19 7:24 ` Markus Armbruster
2022-07-12 9:35 ` [PATCH v2 05/15] stubs: remove needless error_vprintf_unless_qmp() marcandre.lureau
2022-07-19 7:24 ` Markus Armbruster
2022-07-12 9:35 ` [PATCH v2 06/15] qapi: move QEMU-specific dispatch code in monitor marcandre.lureau
2022-08-02 10:58 ` Markus Armbruster
2022-08-02 11:19 ` Marc-André Lureau
2022-08-02 12:21 ` Markus Armbruster
2022-07-12 9:35 ` [PATCH v2 07/15] scripts/qapi-gen: add -i option marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 08/15] scripts/qapi: add required system includes to visitor marcandre.lureau
2022-07-12 15:08 ` Warner Losh
2022-07-12 9:35 ` [PATCH v2 09/15] util: move 256-by-128 division helpers to int128 marcandre.lureau
2022-08-04 16:17 ` Marc-André Lureau
2022-08-04 17:04 ` Lucas Mateus Martins Araujo e Castro
2022-07-12 9:35 ` [PATCH v2 10/15] qemu-common: introduce a common subproject marcandre.lureau
2022-07-12 14:57 ` Warner Losh
2022-07-15 11:55 ` Marc-André Lureau
2022-07-12 9:35 ` [PATCH v2 11/15] qemu-common: move scripts/qapi marcandre.lureau
2022-08-05 8:02 ` Markus Armbruster
2022-08-05 8:49 ` Marc-André Lureau
2022-08-11 6:52 ` Markus Armbruster
2022-08-11 7:11 ` Marc-André Lureau
2022-08-11 9:05 ` Markus Armbruster
2022-08-11 10:09 ` Marc-André Lureau [this message]
2022-08-11 10:22 ` Peter Maydell
2022-08-11 10:50 ` Marc-André Lureau
2022-08-11 12:15 ` Daniel P. Berrangé
2022-08-11 13:35 ` Markus Armbruster
2022-08-22 8:16 ` Marc-André Lureau
2022-09-02 11:15 ` Markus Armbruster
2022-09-02 13:22 ` Marc-André Lureau
2022-08-11 11:46 ` Markus Armbruster
2022-07-12 9:35 ` [PATCH v2 12/15] qemu-common: move glib-compat.h marcandre.lureau
2022-07-12 15:00 ` Warner Losh
2022-07-12 9:35 ` [PATCH v2 13/15] qemu-common: move error-report marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 14/15] mtest2make.py: teach suite name that are just "PROJECT" marcandre.lureau
2022-08-05 10:35 ` Paolo Bonzini
2022-08-05 11:22 ` Marc-André Lureau
2022-07-12 9:35 ` [PATCH v2 15/15] qemu-common: add error-report test marcandre.lureau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJ+F1CKbkTOX7Fh9RvkBvuW_gZqZjYSta=7nEKbzm-OefPE_GQ@mail.gmail.com' \
--to=marcandre.lureau@gmail.com \
--cc=armbru@redhat.com \
--cc=crosa@redhat.com \
--cc=dgilbert@redhat.com \
--cc=eblake@redhat.com \
--cc=fam@euphon.net \
--cc=hreitz@redhat.com \
--cc=imp@bsdimp.com \
--cc=jsnow@redhat.com \
--cc=kevans@freebsd.org \
--cc=kwolf@redhat.com \
--cc=laurent@vivier.eu \
--cc=michael.roth@amd.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@yandex-team.ru \
--cc=xieyongji@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.