On 6/16/2026 1:30 AM, Peter Maydell wrote: > On Tue, 16 Jun 2026 at 08:54, Daniel P. Berrangé <[email protected]> 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. 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. 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. 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. > -- PMM Thanks for both of you for the feedback, Pierrick
