Quoting Laurent Pinchart (2024-09-06 19:04:39) > Hi Mauro, > > (CC'ing Kieran, for a question below) > > First of all, I'd like to say that I think we agree on more points than > may be apparent from the conversation. I believe this new process will > lower the administrative burden on everybody, which should also come as > a relief. > > On Fri, Sep 06, 2024 at 07:43:10PM +0200, Mauro Carvalho Chehab wrote: > > Em Fri, 6 Sep 2024 16:29:59 +0300 Laurent Pinchart escreveu: > > > On Fri, Sep 06, 2024 at 01:10:30PM +0200, Mauro Carvalho Chehab wrote: > > > > Em Fri, 6 Sep 2024 12:36:58 +0200 Sebastian Fricke escreveu: > > > > > On 06.09.2024 10:11, Mauro Carvalho Chehab wrote: > > > > > > Em Thu, 5 Sep 2024 09:16:27 +0200 Hans Verkuil escreveu: > > > > > > > > > > ... > > > > > > 1. All committers shall use a common procedure for all merges. > > > > > > > > > > > > This is easy said than done. So, IMO, it is needed some common scripts > > > > > > to be used by all committers. On my tests when merging two PRs there, > > > > > > those seems to be the minimal set of scripts that are needed: > > > > > > > > > > > > a) script to create a new topic branch at linux-media/users/<user> > > > > > > The input parameter is the message-ID, e. g. something like: > > > > > > > > > > > > create_media_staging_topic <topic_name> <message_id> > > > > > > > > > > > > (eventually with an extra parameter with the name of the tree) > > > > > > > > > > > > It shall use patchwork REST interface to get the patches - or at least > > > > > > to check if all patches are there (and then use b4). > > > > > > > > > > > > such script needs to work with a single patch, a patch series and a > > > > > > pull request. > > > > > > > > > > > > the message ID of every patch, including the PR should be stored at > > > > > > the MR, as this will be needed to later update patchwork. > > > > > > I'm sure individual committers will want to optimize their workflow > > > using scripts, and possibly share them, but what's wrong with simply > > > using b4 ? With -l it will add a link to lore, so the message ID will be > > > available. > > > > > > I'd rather first focus on what we want to see included in commit > > > messages, and then on how to make sure it gets there. > > > > It is not just running b4. There's the need of preparing a MR message that > > matches the content of patch 0, very likely with the message IDs of patches > > and PRs, and check if everything is in place on patchwork, as this is the > > main tool to track the pending queue. Surely b4 can be called on such script. > > Is that needed, or can we rely on the Link: tag of individual patches to > find the corresponding patch in patchwork ? > > > It doesn't need to be complex, but it should automate the basic steps that > > all committers will use. > > > > > > > > > > > > > > b) once gitlab CI runs, there are two possible outcomes: patches may > > > > > > pass or not. If they pass, a MR will be created and eventually be > > > > > > merged. > > > > > > > > > > > > Either merged or not (because something failed or the patches require > > > > > > more work), the patchwork status of the patch require changes to > > > > > > reflect what happened. IMO, another script is needed to update the > > > > > > patch/patch series/PR on patchwork on a consistent way. > > > > > > > > > > > > This is actually a *big* gap we have here. I have a script that > > > > > > manually check patchwork status and the gap is huge. currently, > > > > > > there are 73 patches that seems to be merged, but patchwork was not > > > > > > updated. > > > > > > > > > > > > From what I noticed, some PR submitters almost never update patchwork > > > > > > after the merges. > > > > > > > > > > Interesting I thought that is how it should be? I mean if I send a PR, > > > > > the one taking the PR must decide whether he wants to pull it, so the > > > > > decision whether the set is accepted should be on the one pulling the > > > > > changes and not on the one pushing them. Right? > > > > > > > > Yes, but you still need to update previous attempts to submit the > > > > work. So, let's see a patch series at v9 got a PR. It is up to you > > > > to cleanup everything on patchwork from v1 to v8. > > > > > > That should be done before though, when v2 is submitted, v1 should be > > > marked as superseded. > > > > Agreed. Still most of the ones sending PRs those days don't do that. > > > > > Isn't there a patchwork bot that can help with this ? > > > > As far as I'm aware, no. > > Kieran, aren't you running such a bot for libcamera ? Yes, but I find it a bit hit-and-miss for superseeding and haven't checked through or updated it lately. It's still running somewhat daily though, and it catches patches being merged quite well - but it's just based on commit title/subject. It's derived from the kernel patchwork bot: - https://korg.docs.kernel.org/patchwork/pwbot.html - https://git.kernel.org/pub/scm/linux/kernel/git/mricon/korg-helpers.git/tree/git-patchwork-bot.py -- Kieran > > > It's not perfect in the sense that it doesn't always match new > > > versions with previous ones, but if it can lower the manual burden by > > > even 80%, it's a big win. > > > > > > > Now, the committer handing PR for v9 should be in charge of updating > > > > the status of it and the patches that belong to it, once the patch > > > > is merged or once he takes a decision otherwise. > > > > > > Today most authors don't have a patchwork account, so they can't update > > > the status themselves. The responsibility mostly falls on you and Hans. > > > > The responsibility is for the ones that submit PRs. Unfortunately, this > > is a process that it is not working right now. > > > > See, if you're reviewing a v2 of a patch series, you know that v1 is > > obsolete. It should be easy to run a script that would take the v1 > > patch series and update patchwork to mark all v1 patches obsoleted. > > In many cases that could be automated, but sometimes locating the > previous version automatically won't work. There will likely be some > amount of manual work. Tooling could help. > > > > I'm fine with moving this to committers, which will make your life > > > easier. The patchwork update when merging a branch should ideally be > > > done automatically by the gitlab instance. If we have lore links in the > > > commit messages, that sounds doable. > > > > With multi-committer, it won't be possible anymore for Hans and I do > > do such updates, as the workflow will be handled by the committers. > > > > So, all committers will need to cleanup patch status on patchwork. > > As I wrote above, that's fine with me, and hopefully that will also make > your life easier (unless you tell me you enjoy updating patchwork > manually :-)). > > > > > > > > > > > > > So another script to solve this gap is needed, doing updates on all > > > > > > patches that were picked by the first script. Something like: > > > > > > > > > > Shouldn't the update happen after the MR has been created instead, > > > > > because only after running the CI we know whether the tests fail or > > > > > pass? From what you say above it sounds like the first script merely > > > > > prepares a topic branch to be tested. > > > > > > > > the first script I mentioned prepares and pushes the topic branch. The > > > > patchwork update script (the second one) can be used on several parts > > > > of the workflow: > > > > > > > > - before MR: if the committer decides to not merge the changes, because > > > > it didn't pass on his review; > > > > - after MR pipeline failures; > > > > - after MR being merged: once it reaches media-staging master. > > > > > > > > > > update_patchwork_from_topic <topic_name> <new_status> > > > > > > > > > > > > This would likely need to use some logic to pick the message IDs > > > > > > of the patch inside the MR. > > > > > > > > > > > > Such script could also check for previous versions of the patch > > > > > > and for identical patches (it is somewhat common to receive identical > > > > > > patches with trivial fixes from different developers). > > > > > > > > > > > > Someone needs to work on such script, as otherwise the multi committers > > > > > > model may fail, and we risk needing to return back to the current model. > > > > > > > > > > Just my personal thought: This sounds a bit extreme to me, I mean we are > > > > > currently in the first test run as preparation for the Media-summit > > > > > where we actually want to discuss policies and further requirements. > > > > > > > > What I'm saying is that multiple-committers model will only work if > > > > all committers follow a common procedure (still variants between > > > > committers is possible, provided that the minimal set is followed). > > > > > > > > If this doesn't happens, maintainers may be forced to do rebases and other > > > > manual fixes, with will actually make life worse for everyone. That's > > > > what I want to avoid by having a common set of scripts for the very basic > > > > tasks: create a topic branch for CI to test and update patchwork. > > > > > > The issues you've listed above are about updating status of patches in > > > patchwork. That's important (or otherwise we should drop patchwork if we > > > think it's not important, but I don't think that's the case), but I > > > hardly see how missing updates to patchwork would call for a rebase :-) > > > > The need for rebase typically won't be due to patchwork[1], but to > > broken processes to validate patches that may happen if the patches > > don't reach first a tree under linux-media/users; if someone uses a wrong > > version of compliance utils; if it has a broken topic branch, etc. > > E. g. it may happen if the agreed process isn't be followed to the > > letter. > > I think we all agree on the need of a clearly documented and simple to > understand process. > > > [1] Still, rebases might still be needed if the developer is not taking > > enough care on patchwork. See, if a developer sends a patch X with a > > change, and some days/weeks later another developer sends a patch Y > > identical to X, except for the author (and eventually description), > > if patch Y is merged, there will be a need to drop it and pick X, to > > properly give credits to the right person. > > No, that doesn't call for a rebase. If we really want to credit patch X > in the git history, we can revert Y and apply X. In most cases the > author of patch X will not mind that Y got applied. > > > > There's a need to make sure that what lands in the shared tree is > > > proper, and for that we need a clearly documented procedure. At this > > > point I don't think it requires a committer tool script, even if in the > > > future developing such a tool could help. > > > > > > > > > 2. The mailbomb script that notifies when a patch is merged at media-stage > > > > > > we're using right now doesn't work with well with multiple committers. > > > > > > > > > > > > Right now, the tree at linuxtv runs it, but it might end sending patches > > > > > > to the author and to linuxtv-commits ML that reached upstream from other > > > > > > trees. It has some logic to prevent that, but it is not bulletproof. > > > > > > > > > > > > A replacement script is needed. Perhaps this can be executed together > > > > > > with the patchwork script (1B) at the committer's machine. > > > > > > This could possibly be done with 'b4 ty'. > > > > Call b4 ty after having patches merged at media staging tree works for me. > > Again placing it on a script that would for instance call pwclient and > > b4 ty afterwards sounds the right thing to do. > > > > > Another option is to rely on patchwork notifications instead. If a patch is > > > marked as merged, the notification e-mail sounds enough to notify the > > > submitter. > > > > Patchwork notification e-mails is about patch reviews, being optional. > > Most authors won't create accounts at patchwork, but are interested only > > at the final result. "b4 ty" or similar sounds more appropriate to me. > > Does patchwork send notification to the author if they don't have an > account ? > > > Also, having such patches c/c to linuxtv-commits help maintainers, > > committers and developers to keep track with tree updates. > > Who uses linuxtv-commits, and for what purpose ? I know I never look at > it, but I'm interested in hearing if anyone uses it. > > > > > The bar for sending stuff to drivers/staging is lower than for > > > > drivers/media. According with Kernel's documentation at > > > > Documentation/process/2.Process.rst: > > > > > > > > "Current rules require that drivers contributed to staging > > > > must, at a minimum, compile properly." > > > > > > > > We actually try to be better than that, but still when new stuff is > > > > added to staging, while we try to also ensure no static code checkers > > > > would fail, we may need to relax the rules, as it is not uncommon to > > > > receive drivers that need extra care to follow Kernel coding style > > > > there. > > > > > > Why can't we tell the submitter to fix the warnings first ? Especially > > > if we can point them to a CI report, that would seem the best course of > > > action to me. drivers/staging/ is meant for code that need more time to > > > be considered stable enough for the kernel. The stabilization doesn't > > > happen by magic, it requires someone actively working on it. If the > > > submitter can't bother to fix the warnings, it doesn't forebode anything > > > good and wouldn't make me confident that the code would ever get out of > > > staging (other than simply being deleted). > > > > Surely we can ask fixes, but sometimes the driver is in so bad shape > > that it may take some time, specially when it is a driver that came > > from a developer without Linux upstream development experience. That > > happened, for instance, when we merged lirc drivers, several USB non-uvc > > camera drivers, atomisp, etc. > > > > So, I'd say this will happen case by case, but, from the media CI PoV, > > an option to relax the checks for stuff under drivers/staging is needed. > > We *may* decide to accept a new driver that comes with warnings, but > that would be a rare operation. Subsequent commits for the driver should > not introduce new warnings. > > > > > > > 4. We need to have some sort of "honour code": if undesired behavior > > > > > > is noticed, maintainers may temporarily (or permanently) revoke > > > > > > committer rights. > > > > > > > > > > That sounds like something to discuss on the media summit, > > > > > > > > Sure. That's why I'm asking to explicitly add it to the topics to be > > > > discussed there ;-) > > > > > > > > It would be nice if someone could come up with a proposal for > > > > it, although the discussions of such proposal should likely happen via > > > > email. > > > > > > > > Also, the ones to be added there likely need to explicitly ack with > > > > such code before being added to gitlab's permission group. > > > > > > This sounds fairly straightforward, I think we'll easily agree on rules. > > > > Sure, but a document with those and an explicit ack sounds an important > > measure to avoid potential future problems. > > Yes, it will be explained in a public document. > > > > > 5. The procedure for fixes wil remain the same. We'll have already enough > > > > things to make it work. Let's not add fixes complexity just yet. > > > > Depending on how well the new multi-committers experimental model > > > > works, we may think using it for fixes as well. > > > > > > I think this last point should still be discussed in Vienna. I want to > > > design a clear workflow that covers -next and -fixes. I'm fine if we > > > decide to then implement part of the workflow only as an initial step, > > > if there are good enough reasons to do so. > > > > I don't see any need. There will be enough things for discussion there > > just for -next, which is where 99% of the patches sit. > > > > Handling -fixes require a different procedure, as maintainers need to > > prepare an special PR to send them. Also, it is up to the maintainers > > to decide to either accept a patch as fixes or postpone to next, > > as sometimes it is not a black and white decision if a patch belongs > > to -fixes or if they will be postponed to the next merge week. > > > > For instance, at -rc7, we usually postpone more complex fixes to > > the merge window, as it is usually not a good idea to do last minute > > complex changes. If a committer write a fix patch during -rc7 and get > > it merged, but the maintainers decide to postpone to the merge window, > > the fix branch will require rebase. > > -next and -fixes are not independent. Even if they are handled through > separate tree and processes, they need to take each other into account, > as the -fixes branch may need to be back-merged in -next from time to > time, and patches should never be applied to both -fixes and -next. We > therefore need to agree on a process that will cover both. It doesn't > mean -fixes will be handled through the shared tree right away if we > decide there are good reasons not to do so. > > -- > Regards, > > Laurent Pinchart
