On 6/16/2026 11:19 PM, Markus Armbruster wrote: > Pierrick Bouvier <[email protected]> writes: > >> 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. > > 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. >
You're the first person I read here saying it's not normal, that's refreshing to see some lucidity instead of "It has always been like that/I have my own tempo, suck it up". >> 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". > This assumes everyone always use make check. Spoiler: a lot of people, including myself and experimented contributors, sometimes forget it. To be honest, I never run any test/check on my own machine, and just let GitHub do it for me, relentlessly and consistently. If we take a step back and consider the recent series to fix MAINTAINERS. It was like 40 patches? I don't know which period of time it covered since last time MAINTAINERS file has been consistent (if it has been one day). But let's say a year at least. So, it means that maybe 3 times in your personal life, when you'll move or remove a file, your flow will be interrupted, and you'll be given a chance to fix things directly instead of hours later. It's very different from -Werror that may hit you more frequently. I'm sure you get more door bells/phone calls interrupting you than this error :). We can't do anything really consistent out of breaking the build, or break the CI. And break the CI adds latency by design. Git pre-commit hooks are a good way to deal with that (with formatting also), but we can't enforce any of that unfortunately. If we choose the CI way, truth is that top maintainers will pay for the cost. I'm not one of them, so can't speak for them, but I feel like solving a problem by narrowing the existing bottleneck is not a good idea. > 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 >
