Em Fri, 6 Sep 2024 16:29:59 +0300
Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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.
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.
> 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.
> 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.
> > > >
> > > > 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.
[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.
> 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.
Also, having such patches c/c to linuxtv-commits help maintainers,
committers and developers to keep track with tree updates.
> > 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.
> > > > 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.
> > 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.
Thanks,
Mauro
