| Message ID | 20231205184503.79769-1-Nikolai.Kondrashov@redhat.com |
|---|---|
| Headers | show |
| Series | MAINTAINERS: Introduce V: entry for tests | expand |
Hi Joe, On 12/5/23 20:55, Joe Perches wrote: > On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote: >> Do not die, but only warn when scripts/get_maintainer.pl is asked to >> retrieve information about a missing file. >> >> This allows scripts/checkpatch.pl to query MAINTAINERS while processing >> patches which are removing files. > > Why is this useful? > > Give a for-instance example please. Sure! Take the ebb0130dad751e88c28ab94c71e46e8ee65427c9 commit for example. It removes a file (and recreates it in another format, but that's besides the point) which belongs to four subsystems. This will work OK: scripts/get_maintainer.pl 0001-dt-bindings-mailbox-convert-bcm2835-mbox-bindings-to.patch But if we try to give the file being removed to get_maintainer.pl directly: scripts/get_maintainer.pl -f Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt It will abort with the following message: scripts/get_maintainer.pl: file 'Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt' not found Even though the state of the source tree we're running this on is *exactly* the same. The latter (using the -f option) is the way checkpatch.pl works to fetch the maintenance status (in is_maintained_obsolete()), and the way I implemented fetching the tests from MAINTAINERS (in get_file_proposed_tests()). This way seems to work better for checkpatch.pl, I suppose, because it can link the error message to a specific file. But in principle, it might be possible to just call get_maintainer.pl on every patch, if we really have to. However, I feel that conceptually it should be possible to query MAINTAINERS without actual *source* files being present in the tree (as opposed to patch files which it needs to read), or even the tree being there at all. I saw that we are printing a warning if the queried file is not there in the git repo (I guess it's helpful?), and thought perhaps we can do the same without the git repo too. Hope that helps! Nick
On 12/5/23 20:02, Nikolai Kondrashov wrote: > Alright, here's a second version, attempting to address as many concerns as > possible. It's likely I've missed something, though. Happy New Year, everyone! Hope your holidays went smoothly and peacefully. Is anyone still interested in this effort? Any more feedback you have in mind? Something to change? Thanks! Nick
Alright, here's a second version, attempting to address as many concerns as possible. It's likely I've missed something, though. Changes from v1: * Make scripts/get_maintainer.pl survive querying missing files, giving a warning instead. This is necessary to enable scripts/checkpatch.pl to query MAINTAINERS about files being deleted. * Start with the minimal change just documenting the V: entry, which accepts test commands directly, and tweaking the tools to deal with that. * However, require the commands accept the -h/--help option so that users have an easier time getting *some* help. The run_checks.py missing that is the reason why the patch proposing it for kunit subsystem is marked "DONOTMERGE" in this version. We can drop that requirement, or soften the language, if there's opposition. * Have a *separate* patch documenting 'Tested-with:' as the next (early) change. Mention that you can add a '#' followed by a results URL, on the end. Adjust the V: docs/checks to exclude '#'. * Have a *separate* patch making scripts/checkpatch.pl propose the execution of the test suite defined in MAINTAINERS whenever the corresponding subsystem is changed. * However, use 'CHECK', instead of 'WARNING', to allow submitters specify the exact (and potentially slightly different) command they used, and not have checkpatch.pl complain too loudly that they didn't run the (exact MAINTAINERS-specified) command. This unfortunately means that unless you use --strict, you won't see the message. We'll try to address that in a new change at the end. * Have a *separate* patch introducing the test catalog and accepting references to that everywhere, with a special syntax to distinguish them from verbatim/direct commands. The syntax is prepending the test name with a '*' (just like C pointer dereference). Make checkpatch.pl handle that. * Drop the recommendation to have the "Docs" and "Sources" fields in test descriptions, as the description text should focus on giving a good introduction and not prompt the user to go somewhere else immediately. They both can be referenced in the text where and how is appropriate. * Generally keep the previous changes adding V: entries and test suite docs, and try to accommodate all the requests, but refine the "Summary" fields to fit the checkpatch.pl messages better. * Have a separate patch cataloguing the complete kunit suite. * Finally, add a patch introducing the "proposal strength" keywords (SUGGESTED/RECOMMENDED/REQUIRED) to the syntax of V: entries, which directly affect which level of checkpatch.pl message missing 'Tested-with:' tags would generate: CHECK/WARNING/ERROR respectively. This allows subsystems to disable checkpatch.pl WARNINGS/ERRORS, and keep their test proposals inobtrusive, if they so wish. E.g. if they expect people to change their commands often. At the same time allow stricter workflows for subsystems with more uniform testing. Or e.g. for subsystems which expect the tests to explain their parameters in their output, and the submitters to upload and link their results in their 'Tested-with:' tags. That seems to be all, but I'm sure I forgot something :D Anyway, send me more corrections and I'll try to address them, but it's likely going to happen next year only. Nick --- Nikolai Kondrashov (9): get_maintainer: Survive querying missing files MAINTAINERS: Introduce V: entry for tests MAINTAINERS: Propose kunit core tests for framework changes docs: submitting-patches: Introduce Tested-with: checkpatch: Propose tests to execute MAINTAINERS: Support referencing test docs in V: MAINTAINERS: Propose kvm-xfstests smoke for ext4 docs: tests: Document kunit in general MAINTAINERS: Add proposal strength to V: entries Mark Brown (1): MAINTAINERS: Propose kunit tests for regmap Documentation/process/index.rst | 1 + Documentation/process/submitting-patches.rst | 46 +++++++ Documentation/process/tests.rst | 96 +++++++++++++++ MAINTAINERS | 17 +++ scripts/checkpatch.pl | 174 ++++++++++++++++++++++++++- scripts/get_maintainer.pl | 23 +++- scripts/parse-maintainers.pl | 3 +- 7 files changed, 355 insertions(+), 5 deletions(-) ---