Re: [PATCH 00/39] MAINTAINERS: Fix F: lines

[Markus Armbruster <armbru@redhat.com>]

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

> On 6/1/2026 3:06 AM, Markus Armbruster wrote:
>> Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com> writes:
>> 
>>> Hi Markus,
>>>
>>> On 5/21/2026 1:04 AM, Markus Armbruster wrote:
>>>> Quite a few F: lines don't match any files.  The quick & dirty check
>>>>
>>>>     $ ls `sed -n 's/^F: *//p' MAINTAINERS ` >/dev/null
>>>>
>>>> finds about fifty.
>>>>
>>>> Philippe Mathieu-Daudé recently posted a few fixes:
>>>>
>>>>   MAINTAINERS: Fix docker/dockerfiles/debian-hexagon-cross.docker path
>>>>   MAINTAINERS: Cover debian-loongarch-cross.docker with LoongArch section
>>>>   MAINTAINERS: Cover debian-xtensa-cross.docker with Xtensa section
>>>>   MAINTAINERS: Cover debian-tricore-cross.docker with TriCore section
>>>>   MAINTAINERS: Cover python.docker with Python library section
>>>>   MAINTAINERS: Fix s390x storage key/attribute device paths
>>>>   MAINTAINERS: Fix tcg/s390x/ path
>>>>   MAINTAINERS: Correct scripts/coverity-model.c path
>>>>   MAINTAINERS: Fix hexagon-linux-user.mak path

[...]

>>> In addition, see the patch attached to this email.
>>> It integrates checking this directly at configure time, so we never run
>>> into any missing entry again in the future.
>>>
>>> I share this here not for a review, but simply to avoid a duplicated
>>> effort, and make sure people know it will be sent after this series.
>>>
>>> I don't believe in adding this in checkpatch, because it's not enforced
>>> systematically unfortunately. Breaking the meson configuration is a good
>>> way to make sure it's enforced by design.
>> 
>> No objection.
>> 
>>> With your series applied, the left entries are:
>> 
>> Most of these are fixed in Philippe's patches mentioned above.
>>
>
> 👍
>
>>> No matching files for /usr2/pbouvier/.work/qemu/MAINTAINERS +258:
>>> configs/targets/hexagon-linux-user/default.mak
>> 
>> MAINTAINERS: Fix hexagon-linux-user.mak path
>> 
>>> No matching files for /usr2/pbouvier/.work/qemu/MAINTAINERS +259:
>>> docker/dockerfiles/debian-hexagon-cross.docker
>> 
>> MAINTAINERS: Fix docker/dockerfiles/debian-hexagon-cross.docker path
>> 
>>> No matching files for /usr2/pbouvier/.work/qemu/MAINTAINERS +2956:
>>> hw/s390x/storage-keys.h
>>> No matching files for /usr2/pbouvier/.work/qemu/MAINTAINERS +2965:
>>> hw/s390x/storage-attributes.h
>> 
>> MAINTAINERS: Fix s390x storage key/attribute device paths
>> 
>>> No matching files for /usr2/pbouvier/.work/qemu/MAINTAINERS +3241:
>>> scripts/coverity-model.c
>> 
>> MAINTAINERS: Correct scripts/coverity-model.c path
>> 
>>> No matching files for /usr2/pbouvier/.work/qemu/MAINTAINERS +3468:
>>> tests/*.py
>> 
>> This is the exception I mentioned above.
>> 
>> The intent is to match *.py below tests/.  It actually matches only in
>> tests/, not in its subdirectories.
>> 
>> Here's a dumb fix:
>> 
>>     F: tests/*.py
>>     F: tests/*/*.py
>>     F: tests/*/*/*.py
>>     F: tests/*/*/*/*.py
>> 
>
> It's acceptable, and nice because it's not ambiguous.
> Also, it shows us the first line is not a correct entry (there is no
> tests/*.py).

It's also brittle: breaks when we add the first .py at level.

>> for however many levels we have.  Same for the scripts/ line next to it.
>> Blech.
>> 
>> The smart fix might be to port N: from the kernel.
>> 
>> 	N: Files and directories *Regex* patterns.
>> 	   N:	[^a-z]tegra	all files whose path contains tegra
>> 	                        (not including files like integrator)
>> 	   One pattern per line.  Multiple N: lines acceptable.
>> 	   scripts/get_maintainer.pl has different behavior for files that
>> 	   match F: pattern and matches of N: patterns.  By default,
>> 	   get_maintainer will not look at git log history when an F: pattern
>> 	   match occurs.  When an N: match occurs, git log history is used
>> 	   to also notify the people that have git commit signatures.
>> 
>> But I wonder: is this section useful at all?
>
> I would prefer to use the manual entries approach above, especially if
> there is only folder that is concerned.
> When we'll have two different use case, maybe we can consider a more
> generic approach.

I'm aware of just two:

    Python scripts
    M: John Snow <jsnow@redhat.com>
    M: Cleber Rosa <crosa@redhat.com>
    S: Odd Fixes
    F: scripts/*.py
    F: tests/*.py

The section was added almost a decade ago (commit ad904f6689f), and
never worked as intended.  I asked John Snow about deleting it to
unblock your work.  He told me he'd be fine with that.  Could add it
back later fixed.

>>> No matching files for /usr2/pbouvier/.work/qemu/MAINTAINERS +4149:
>>> tcg/s390/
>> 
>> MAINTAINERS: Fix tcg/s390x/ path
>> 
>>> Once your current series is pulled, I'll fix the remaining and send the
>>> attached patch. Or feel free to do it directly if you like the idea :)
>>>
>>> Regards,
>>> Pierrick
>> 
>> Two remarks inline.
>> 
>>> From 6c9b49ac7ec06c0159d2b4ba9c9d1081e02ef765 Mon Sep 17 00:00:00 2001
>>> From: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>
>>> Date: Fri, 22 May 2026 12:23:17 -0700
>>> Subject: [PATCH] meson.build: check MAINTAINERS file is consistent with source
>>>  tree
>>>
>>> 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.
>>>
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>
>>> ---
>>>  meson.build                       |  5 +++
>>>  scripts/check-maintainers-file.py | 53 +++++++++++++++++++++++++++++++
>>>  2 files changed, 58 insertions(+)
>>>  create mode 100755 scripts/check-maintainers-file.py
>>>
>>> diff --git a/meson.build b/meson.build
>>> index eeb096c1487..ddfb0b90ca6 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -18,6 +18,11 @@ add_test_setup('thorough',
>>>  
>>>  meson.add_postconf_script(find_program('scripts/symlink-install-tree.py'))
>>>  
>>> +# check our MAINTAINERS file is consistent
>>> +check_maintainers = find_program('scripts/check-maintainers-file.py')
>>> +maintainers_file = files('MAINTAINERS')
>>> +run_command([check_maintainers, maintainers_file], check: true, console: true)
>> 
>> My version of meson (1.8.5) chokes on console: true.  According to
>> https://mesonbuild.com/Reference-manual_functions_run_command.html#run_command_console
>> it's new in 1.11.0.
>>
>
> Our configure script creates a venv with an updated version of meson,
> which is 1.11.1 at the moment.
> If you called meson directly (which uses your host meson), it's not how
> we're supposed to build QEMU.
> How did you try this?

I ran make :)

>> I tested with it deleted.
>> 
>>> +
>>>  ####################
>>>  # Global variables #
>>>  ####################
>>> diff --git a/scripts/check-maintainers-file.py b/scripts/check-maintainers-file.py
>>> new file mode 100755
>>> index 00000000000..b001816a401
>>> --- /dev/null
>>> +++ b/scripts/check-maintainers-file.py
>>> @@ -0,0 +1,53 @@
>>> +#! /usr/bin/env python3
>>> +
>>> +# Check incorrect file entries in MAINTAINERS
>>> +#
>>> +# Author: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>
>>> +#
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +
>>> +import argparse
>>> +import glob
>>> +import sys
>>> +
>>> +
>>> +def check_one_entry(line) -> bool:
>>> +    return True
>>> +
>>> +
>>> +def main() -> None:
>>> +    parser = argparse.ArgumentParser(description="Check MAINTAINERS file")
>>> +    parser.add_argument("maintainers", help="Path to MAINTAINERS file")
>>> +    args = parser.parse_args()
>>> +
>>> +    found_file_entry = False
>>> +    found_incorrect_entries = False
>>> +    line_counter = 0
>>> +
>>> +    with open(args.maintainers) as file:
>>> +        for entry in file:
>>> +            line_counter += 1
>>> +
>>> +            if not entry.startswith("F:"):
>>> +                continue
>>> +            entry = entry[2:].strip()
>>> +            found_file_entry = True
>>> +
>>> +            file_exists = len(glob.glob(entry, recursive=True)) > 0
>> 
>> I'm afraid this matches files not in git, just like my quick & dirty
>> one-liner.  Shouldn't we match against contents of HEAD, say output of
>> "git-ls-tree -r --name-only @"?
>>
>
> I don't think it's needed to restrict to git ls-tree. The only risk is
> that people have a local file they forgot (or didn't want) to add in
> git. It will be caught by CI that won't have such a file, so we're safe.
>
> What do you think?

Yes, we're safe, but the earlier we catch mistakes, the better.  Now,
the juice isn't always worth the squeeze.  How hard would we have to
squeeze here?  We could use fnmatch.filter(names, pat) to test whether
@pat matches anything in @names, where @names is from git-ls-tree.

>>> +            if file_exists:
>>> +                continue
>>> +
>>> +            found_incorrect_entries = True
>>> +            print(
>>> +                f"No matching files for {args.maintainers} +{line_counter}: {entry}",
>>> +                file=sys.stderr,
>>> +            )
>>> +
>>> +    if not found_file_entry:
>>> +        raise Exception("no file entry found - is MAINTAINERS path correct?")
>>> +    if found_incorrect_entries:
>>> +        raise Exception(f"incorrect entries found in {args.maintainers}")
>>> +
>>> +
>>> +if __name__ == "__main__":
>>> +    main()
>> 
>
> Regards,
> Pierrick