From: Markus Armbruster <armbru@redhat.com>
To: "Marc-André Lureau" <marcandre.lureau@gmail.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 11:05:26 +0200 [thread overview]
Message-ID: <87o7wr5ew9.fsf@pond.sub.org> (raw)
In-Reply-To: <CAJ+F1CKH5y8SWULvgXWh7PPDTXOMGusYHE6RwZDZWVJoC=m8hQ@mail.gmail.com> ("Marc-André Lureau"'s message of "Thu, 11 Aug 2022 11:11:15 +0400")
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?
* Most of the QObject subsystem
qobject/block-qdict.c is left behind.
* 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?
A bit of HMP support left behind.
* 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.
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.
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.
The actual split seems somewhat arbitrary in places. I suspect more
code will move over time. Invalidating "what is where" knowledge.
I believe a serious think about other ways to accomplish your goals is
called for.
> (fwiw, it's a bit of a shame python-qemu-qmp didn't import git history from
> qemu.. we did better with libslirp. If we ever move code in standalone
> repositories again, we should be careful to keep history with it)
Yes, we should preserve history whenever practical.
[...]
next prev parent reply other threads:[~2022-08-11 9:10 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 [this message]
2022-08-11 10:09 ` Marc-André Lureau
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=87o7wr5ew9.fsf@pond.sub.org \
--to=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=marcandre.lureau@gmail.com \
--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.