Re: [Ksummit-discuss] [RFC PATCH 2/3] MAINTAINERS, Handbook: Subsystem Profile

[Dan Williams]

On Fri, Nov 16, 2018 at 4:04 AM Mauro Carvalho Chehab
<mchehab@xxxxxxxxxx> wrote:
>
> Em Thu, 15 Nov 2018 16:11:59 -0800
> Frank Rowand <frowand.list@xxxxxxxxx> escreveu:
[..]
> > > +Patches or Pull requests
> > > +------------------------
> > > +Some subsystems allow contributors to send pull requests, most require
> > > +mailed patches. State âPatches onlyâ, or âPull requests acceptedâ.
> >
> > This may create some confusion if only "Pull requests accepted" is the
> > contents of this section.  My understanding has been that all changes
> > should be available on a mail list for review before being accepted
> > by the maintainer, even if eventually the final version of the series
> > that is accepted is applied by the maintainer as a pull instead of
> > via applying patches.
> >
> > Is my "must appear on a mail list" understanding correct?
>
> I guess this is true on almost all subsystems that accept pull requests.
>
> It could not be true if some subsystem moves to Github/Gitlab.

Yes. Maybe a "Review Forum" section for subsystems that have
transitioned from email to a web-based tool? There's also the
exception of security disclosures, but the expectations for those
patches are already documented.

> > > +Last -rc for new feature submissions
> > > +------------------------------------
> > > +New feature submissions targeting the next merge window should have
> > > +their first posting for consideration before this point. Patches that
> > > +are submitted after this point should be clear that they are targeting
> > > +the NEXT+1 merge window, or should come with sufficient justification
> > > +why they should be considered on an expedited schedule. A general
> > > +guideline is to set expectation with contributors that new feature
> > > +submissions should appear before -rc5. The answer may be different for
> > > +'Core:' files, include a second entry prefixed with 'Core:' if so.
> >
> > I would expect the -rc to vary based on the patch series size, complexity,
> > risk, number of revisions that will occur, how controversial, number of
> > -rc releases before Linus chooses to release, etc.  You would need a
> > crystal ball to predict much of this.  I could see being able to provide
> > a good estimate of this value for a relatively simple patch.
>
> That's only partially true. I mean, the usual flux is to go up to -rc7.
> After that, the final version is usually merged (except when something
> goes wrong).
>
> Anyway, I guess nobody would complain if the maintainers do an exception
> here and accept more patches when they know that the last rc version
> won't be -rc7, but, instead, -rc8 or -rc9.
>
> This is a general ruleset that describes the usual behavior, telling the
> developers the expected behavior. If the maintainers can do more on some
> particular development cycle, it should be fine.

Yes, and perhaps I should clarify that this is the point at which a
maintainer will start to push back in the typical case, and indicate
to a contributor that they are standing in exceptional territory.
Similar to how later in the -rc series patches get increasing
scrutiny.

> > > +Last -rc to merge features
> > > +--------------------------
> > > +Indicate to contributors the point at which an as yet un-applied patch
> > > +set will need to wait for the NEXT+1 merge window. Of course there is no
> > > +obligation to ever except any given patchset, but if the review has not
> > > +concluded by this point the expectation the contributor should wait and
> > > +resubmit for the following merge window. The answer may be different for
> > > +'Core:' files, include a second entry prefixed with 'Core:' if so.
> >
> > Same comment as for the previous section, with the additional complexity
> > that each unique patch series should bake in -next.

The intent is to try to leave all "should" or prescriptive statements
out of the profile. I'm looking to target other parts of the handbook
to carry advice for integrating with -next and suggested soak times.

> > > +Non-author Ack / Review Tags Required
> > > +-------------------------------------
> >
> > The title of this section seems a bit different than the description
> > below.  A more aligned title would be something like "Maintainer
> > self-commit review requirements".

This is intended to go beyond maintainer self-commits. Consider a pull
request that contains commits without non-author tags.

> > > +Let contributors and other maintainers know whether they can expect to
> > > +see the maintainer self-commit patches without 3rd-party review. Some
> > > +subsystem developer communities are so small as to make this requirement
> > > +impractical. Others may have been bootstrapped by a submission of
> > > +self-reviewed code at the outset, but have since moved to a
> > > +non-author review-required stance. This section sets expectations on the
> > > +code-review economics in the subsystem. For example, can a contributor
> > > +trade review of a maintainer's, or other contributor's patches in
> > > +exchange for consideration of their own.
> >
> > Then another section (which is the one I expected when I slightly
> > mis-read the section title) would be: Review Tags Requirements",
> > which would discuss what type of review is expected, encouraged,
> > or required.

Per the clarification above, I think the whole thing should be called
"Review Tags Requirement".

> > > +Test Suite
> > > +----------
> > > +Indicate the test suite all patches are expected to pass before being
> > > +submitted for inclusion consideration.
> > > +
> > > +
> > > +Resubmit Cadence
> > > +----------------
> > > +Define a rate at which a contributor should wait to resubmit a patchset
> > > +that has not yet received comments. A general guideline is to try to
> > > +meet a deadline of 1 - 2 weeks to acknowledge starting consideration for
> > > +a patch set.
> >
> > Or ping instead of resubmitting?  If you resubmit the same series with
> > an unchanged version then comments could be split across multiple
> > email threads.  If you resubmit the same series with a new version
> > number, the same comment split can occur plus it can be confusing
> > that two versions have the same contents.  I suspect that there may be
> > some maintainers who do prefer a resubmit, but that is just wild
> > conjecture on my part.
>
> That depends on how maintainers handle patches. Those subsystems that
> use patchwork (like media) don't really require anyone to resubmit
> patches. They're all stored there at patchwork database.
>
> My workflow is that, if I receive two patches from the same person with
> the same subject, I simply mark the first one as obsoleted, as usually
> the second one is a new version, and reviewers should need some
> time to handle it.
>
> In other words, re-sending a patch may actually delay its handling, as
> the second version will be later on my input queue, and I'll grant
> additional time for people to review the second version at the community.

Ok, this sounds like a separate policy. How often to follow-up
separated from resend the full series vs send a ping mail.

> > > +Trusted Reviewers
> > > +-----------------
> > > +While a maintainer / maintainer-team is expected to be reviewer of last
> > > +resort the review load is less onerous when distributed amongst
> > > +contributors and or a trusted set of individuals.  This section is
> > > +distinct from the R: tag (Designated Reviewer). Whereas R: identifies
> > > +reviewers that should always be copied on a patch submission, the
> > > +trusted reviewers here are individuals contributors can reach out to if
> > > +a few 'Resubmit Cadence' intervals have gone by without maintainer
> > > +action, or to otherwise consult for advice.
> >
> > This seems redundant with the MAINTAINERS reviewers list.  It seems like
> > the role specified in this section is more of an ombudsman or developer
> > advocate who can assist with the review and/or accept flow if the
> > maintainer is being slow to respond.
>
> Well, on subsystems that have sub-maintainers, there's no way to point to
> it at MAINTAINERS file.
>
> Also, not sure about others, but I usually avoid touching at existing
> MAINTAINERS file entries. This is a file that everyone touches, so it
> has higher chances of conflicts.
>
> Also, at least on media, we have 5 different API sets (digital TV, V4L2, CEC,
> media controller, remote controller). Yet, all drivers are stored at the
> same place (as a single driver may use multiple APIs).
>
> The reviewers for each API set are different. There isn't a good way
> to explain that inside a MAINTANERS file.

Would it be worthwhile to have separate Subsystem Profiles for those
API reviewers? If they end up merging patches and sending them
upstream might we need a hierarchy of profiles for each hop along the
upstream merge path?

> > > +Time Zone / Office Hours
> > > +------------------------
> > > +Let contributors know the time of day when one or more maintainers are
> > > +usually actively monitoring the mailing list.
> >
> > I would strike "actively monitoring the mailing list".  To me, it should
> > be what are the hours of the day that the maintainer might happen to poll
> > (or might receive an interrupt) from the appropriate communications
> > channels (could be IRC, could be email, etc).

Yes, makes sense.

> > For my area, I would want to say something like: I tend to be active
> > between 17:00 UTC (18:00 UTC when daylight savings) and 25:00 (26:00),
> > but often will check for urgent or brief items up until 07:00 (08:00).
> > I interact with email via a poll model.  I interact with IRC via a
> > pull model and often overlook IRC activity for multiple days).
>
> Frankly, for media, I don't think that working hours makes sense. Media
> (sub-)maintainers are spread around the globe, on different time zones
> (in US, Brazil and Europe). We also have several active developers in
> Japan, so we may end by having some day reviewers/sub-maintainers from
> there.

For that case just say:

"the sun never sets on the media subsystem" ;-)

> At max, we can say that we won't warrant to patches on weekends or holidays.

Yeah, maybe:

"outside of weekends or holidays there's usually a maintainer or
reviewer monitoring the mailing list"