On Mon, 6 Dec 2021 at 14:14, Alex Bennée <alex.ben...@linaro.org> wrote: > > Experience has shown that getting new functionality up-streamed can be > a somewhat painful process. Lets see if we can collect some of our > community knowledge into a blog post describing some best practices > for getting code accepted. > > Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
(I shan't bother with a scan for typos etc for the moment.) > +From time to time I hear of frustrations from potential new > +contributors who have tried to get new features up-streamed into the > +QEMU repository. After having read [our patch > +guidelines](https://qemu.readthedocs.io/en/latest/devel/submitting-a-patch.html) > +they post them to [qemu-devel](https://lore.kernel.org/qemu-devel/). > +Often the patches sit there seemingly unread and unloved. The > +developer is left wandering if they missed out the secret hand shake > +required to move the process forward. My hope is that this blog post > +will help. > + > +New features != Fixing a bug > +---------------------------- > + > +Adding a new feature is not the same as fixing a bug. For an area of > +code that is supported for Odd Fixes or above there will be a > +someone listed in the > +[MAINTAINERS](https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS) > +file. A properly configured `git-send-email` will even automatically > +add them to the patches as they are sent out. The maintainer will > +review the code and if no changes are requested they ensure the > +patch flows through the appropriate trees and eventually makes it into > +the master branch. > + > +This doesn't usually happen for new code unless your patches happen to > +touch a directory that is marked as maintained. Without a maintainer > +to look at and apply your patches how will it ever get merged? I think there is a distinction here between "new feature added to something that we already have" and "new feature that isn't part of something we already have". In the former category for instance you have things like "support virtio-mem-pci on the virt board". That's definitely not a bug fix, but it ties into existing and hopefully maintained in-tree things (the virt board, the x86 virtio-mem-pci support) whose maintainers are probably[*] interested and informed enough to help with review. On the other hand "add support for new target architecture foo" is more distinct from what we have already. [*] There is of course the elephant-trap the project sets for new submitters of having parts of the codebase that according to MAINTAINERS have a maintainer but in practice that maintainer is AWOL or overstretched. There is also the spectrum from "clearly fits neatly into QEMU's current design" through to "proposing something very new". "New board" and "new target architecture" may not have a current maintainer, but they do fit very obviously into our existing structure and design. Nobody is likely to object on principle, and the submitter's problem here is merely to attract the attention of somebody to get it reviewed. On the other hand, a new feature like "support plugging QEMU into a SystemC emulation" is massively impactful and poses serious structural questions. A submitter would have a massive uphill job to gain consensus about supporting the feature at all, even before getting into the specifics of a patchset. > + > +Adding new code to a project is not a free activity. Code that isn't > +actively maintained has a tendency to [bit > +rot](http://www.catb.org/jargon/html/B/bit-rot.html) and become a drag > +on the rest of the code base. The QEMU code base is quite large and > +none of the developers are knowledgeable about the all of it. If > +features aren't > +[documented](https://qemu.readthedocs.io/en/latest/devel/submitting-a-patch.html) > +they tend to remain unused as users struggle to enable them. If an > +unused feature becomes a drag on the rest of the code base by preventing > +re-factoring and other clean ups it is likely to be deprecated. > +Eventually deprecated code gets removed from the code base never to be > +seen again. > + > +Fortunately there is a way to avoid the ignominy of ignored new features > +and that is to become a maintainer of your own code! I think this is oversimplifying to the point of being misleading. Some kind of commitment to staying around (ie that this is not a "drop the code and run away" manoeuvre) is definitely helpful, but it doesn't by itself do anything to address the primary problem, which is "you need to persuade somebody to care enough about your new feature to put in the work to review it". > +A practically perfect set of patches > +------------------------------------ > + > +I don't want to repeat all the valuable information from the > +submitting patches document but For a new feature, I think the thing I would most emphasise is the importance of a really good cover letter. It gets more critical as you move along the spectrum from "new feature in existing subsystem" to "new feature but which sits cleanly within QEMU" to "new and unusual feature". The cover letter is your opportunity to explain what you're doing and why somebody else should care about it enough to code review it. It needs to explain the work to an audience who hasn't spent the last six weeks developing it, and why it would be useful to the project to accept it. If the feature looks on the surface like it's "odd thing that we haven't done before" but is really "quite similar to existing thing if you look at it closer", the cover letter is a good place to explain that. (This all applies still for v2 etc; the v2 cover letter shouldn't assume that the reader was paying attention and looked at the v1 cover letter or patches.) > +there is still the > +problem of getting reviews for your brand new code. Fortunately there > +is no approved reviewer list for QEMU. Something of a side-tangent, but I'm not sure about "fortunately". There's an analogy to be drawn here with some of the points made in Jo Freeman's _Tyranny of Structurelessness_ essay (https://www.jofreeman.com/joreen/tyranny.htm) about how if you don't have official structures and hierarchy you get a de-facto unofficial one anyway -- we do not have a formal approved reviewer list, and therefore instead we have an informal and unacknowledged set of "trusted" reviewers. The latter situation is not self-evidently better than the former. -- PMM
