Re: [PATCH 2/2] meson.build: check MAINTAINERS file is consistent with source tree

[Markus Armbruster <armbru@redhat.com>]

Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com> writes:

> On 6/16/2026 1:30 AM, Peter Maydell wrote:
>> On Tue, 16 Jun 2026 at 08:54, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>
>>> On Mon, Jun 15, 2026 at 01:17:23PM -0700, Pierrick Bouvier wrote:
>>>> We add a new script: scripts/check-maintainers-file.py, that will run at
>>>> configuration time (and not at build time), to not hurt build time.
>>>> This script runs in 0.2s on my dev VM, which has an old cpu.
>>>>
>>>> We can expect things to be mostly in sync since adding or removing a
>>>> source or test file will trigger a configure step.
>>>> For the rest, like docs, tcg tests, or remaining files, GitLab CI will
>>>> build things from scratch and always run the configure step.
>>>>
>>>> With this, it should be impossible by design to have an upstream
>>>> MAINTAINERS file with non existing file entries.
>>>
>>> Accuracy of the MAINTAINERS file is an upstream-only concern, but
>>> IIUC, this check is going to apply universally to every build of
>>> QEMU which is undesirable. It is irrelevant to end users and not
>>> appropriate to check in downsteam vendors forks.
>>>
>
> I accept the argument about downstream forks even though they are free
> to remove the check since they are... forks.
> However, it's relevant to users I think. It's very easy to move a file,
> and forget to update associated MAINTAINERS entry, and the sooner we
> catch that, the better. Similar, when a new file is added and not
> covered by an entry in MAINTAINERS, it's very easy to miss that.
>
>>> In a "normal" modern project this kind of check would be done in
>>> a CI job on the merge request, since that's the only place it is
>>> relevant. In our case, the nearest fit is the checkpatch.pl file.
>
> It depends how fast you want to catch the problem.
> Adding yet another round trip latency to a project that already has a
> multi day latency for upstream is not a good thing IMHO.

Yes, and ...

> Also, it brings yet another burden on top maintainers that will have to
> deal with this new failure, fix it themselves, or recreate their staging
> set for the next batch and wait for yet another day. More time spent,
> more noise, more iteration, just for a missing line update in
> MAINTAINERS file.

... yes.

We might have conditioned ourselves to consider such loops normal.
They're not.

> All that is nonexistent if no one can build it if it's incorrect. That's
> why we use -Werror by default for instance. And if someone sends a patch
> breaking this, you can be sure they didn't even try to build their own
> code/doc/test.

I don't want make to fail during development just because I haven't
updated MAINTAINERS, yet.  It's a distraction that could snap me out of
the flow.  I'm developing with -Werror disabled for the same reason.

I do want make to fail before I send patches.  I do another compile with
-Werror enabled then, but that's just my personal workflow.

Solutions I like well enough:

* Make it a compile-time warning that obeys --enable-werror.  Could well
  be more trouble than it's worth.

* Make it fail "make check".

Dislike:

* Make it a checkpatch.pl error.  Stays out of my hair during
  development, but then I have to remember to actually run
  checkpatch.pl, or risk having CI bouncing things back to me, wasting
  everybody's time.  Meh

* Make it fail CI in some other way.  Worse, because now I have to
  remember running checkpatch.pl *and* that other way.

* Make it fail "make".  Stay out of my hair!

> Does that open your view on this approach?
>
>> 
>> You could put it in "make check", or (if you want to avoid the
>> downstream-forks issue) in some sub-bit of "make check"
>> that we don't check by default but do check in one of our CI jobs.
>>
>
> If argument above is still rejected, we can just add a CI job (calling
> script directly). I'm not sure any kind of integration is needed. Also,
> not sure where to fit that in any existing test suite to be honest.
>
>> checkpatch isn't a great place for checks that you want to
>> stay consistent on because so much of its output is "this
>> is wrong/merely something to check, ignore".
>>
>
> I agree, checkpatch is not useful to enforce things. It would be
> different if it was strictly enforced in CI, but that's not the case,
> and won't be given the false positives it brings.

CI should only reject things that are actually wrong, or at least
undesirable.

checkpatch.pl warns when you add, move, or delete files without updating
MAINTAINERS.  It's only a warning, because MAINTAINERS may in fact not
need an update.  Not good enough for CI rejects.

The purpose of warnings is to help maintainers and developers find
*potential* issues.

>
>> -- PMM
>
> Thanks for both of you for the feedback,
> Pierrick