[PATCH 09/11] doc: add common maintainer guidelines file, start with drm-intel

Tue Oct 30 15:55:55 UTC 2018

On Tue, Oct 30, 2018 at 03:43:55PM +0200, Jani Nikula wrote:
> On Thu, 25 Oct 2018, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
> > On Thu, Oct 25, 2018 at 06:22:09PM +0300, Jani Nikula wrote:
> >> For starters, add the specific guidelines in their own files, starting
> >> with drm-intel, with the intention of consolidating and pulling in
> >> common guides in the top level file in the long term.
> >> 
> >> Start with adding detailed maintainer tasks for handling drm-intel
> >> branches.
> >> 
> >> v2: some tweaks, use dim rebase for rebases
> >> 
> >> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> ---
> >>  index.rst                 |   1 +
> >>  maintainer-drm-intel.rst  | 141 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  maintainer-guidelines.rst |  12 ++++
> >>  3 files changed, 154 insertions(+)
> >>  create mode 100644 maintainer-drm-intel.rst
> >>  create mode 100644 maintainer-guidelines.rst
> >> 
> >> diff --git a/index.rst b/index.rst
> >> index 88b49edbeece..7049d94ccc18 100644
> >> --- a/index.rst
> >> +++ b/index.rst
> >> @@ -27,6 +27,7 @@ Contents:
> >>     drm-misc
> >>     drm-intel
> >>     committer-guidelines
> >> +   maintainer-guidelines
> >>     commit-access
> >>     getting-started
> >>     dim
> >> diff --git a/maintainer-drm-intel.rst b/maintainer-drm-intel.rst
> >> new file mode 100644
> >> index 000000000000..82a3a739eccb
> >> --- /dev/null
> >> +++ b/maintainer-drm-intel.rst
> >> @@ -0,0 +1,141 @@
> >> +.. _maintainer-drm-intel:
> >> +
> >> +=================================
> >> + drm-intel Maintainer Guidelines
> >> +=================================
> >> +
> >> +This document describes the detailed maintainer tasks for drm-intel.
> >> +
> >> +Maintaining drm-intel Branches
> >> +==============================
> >> +
> >> +The guide assumes a maintainer rotation of one person handling features and
> >> +fixes for one release. Thus for each branch, you take over from the person
> >> +maintaining the branch before you, and leave it in a known state for the person
> >> +after you.
> >> +
> >> +drm-intel-next-queued
> >> +---------------------
> >> +
> >> +Take over when the last drm-intel feature pull has been sent for an upcoming
> >> +merge window, and drm-intel-next-queued starts targeting the next merge window
> >> +after that. This happens around -rc5 of the current development kernel.
> >> +
> >> +Tag drm-intel-next-queued periodically, say every 1-2 weeks, depending on the
> >> +rate of change. Use ``dim update-next``. Write a tag summary detailing the
> >> +changes since the last tag. Send a testing request.
> >> +
> >> +Send drm-intel-next-queued pull requests periodically. Use ``dim
> >> +pull-request-next``. Pull requests are based on the tags generated above. Don't
> >> +send a pull request immediately after tagging, leave some time for
> >> +testing. Typically you would send a pull request for each tag, but the tooling
> >> +allows for accumulation of several tags into one pull request. Dave usually
> >> +doesn't start pulling features until the pull requests for the previous release
> >> +have been merged to Linus' tree, so it's normal to accumulate several tags
> >> +before and during the merge window. Only send your first pull request after
> >> +-rc1.
> >
> > Probably the window -rc1 to -rc5 requires a dedicated section.
> 
> What do you mean?

hmm... what I mean is a block to explain our "work-window" in a way that it gets
clear to anyone. But yeap, I can try to suggest something on top later.
Nevermind ;)

> 
> >
> >> +
> >> +Do backmerges as needed, but also don't let drm-intel-next-queued fall too much
> >> +behind from drm-next. Use ``dim backmerge``.
> >
> > well, I remember being told something like: "only do backmerges when there is a
> > real reason/need and make sure this is documented on backmerge commit"
> >
> > I mean, I'm not against what is written now, but I believe the "fall too much
> > behind" is kind of vague. Should we do at least one backmerge at some specific
> > point in time to avoid getting too behind?
> 
> Maybe I'll just add "Use your discretion" because that's what it will
> boil down to.

yeap, this sounds better for now.

> 
> >
> >> Never rebase or force push. Only do
> >> +backmerges from drm-next. Specifically, don't merge drm-misc branches or
> >> +backmerge Linus' tree directly; they all need to go through drm-next. Request
> >> +drm-misc maintainers to have your dependencies sent to drm-next, and request
> >> +Dave to have Linus' tree backmerged to drm-next.
> >> +
> >> +Pull in gvt pull requests. Use ``dim apply-pull``.
> >
> > I believe we need to clarify that this is talking about gvt pull targeting next.
> 
> Ok.

thanks

> 
> >
> >> +
> >> +Usually committers handle most patches, but look for the patches that fall
> >> +through the cracks otherwise. Ping for reviews, and push reviewed patches.
> >> +
> >> +Track the patches being committed, ensuring the merge criteria and the rest of
> >> +the process is being properly followed.
> >> +
> >> +Remind people about the last feature pull deadline (-rc5 time frame), especially
> >> +when there are known features aiming for a release. It's not the maintainer's
> >> +job to ensure deadlines are met, but to raise awareness. Avoid surprising
> >> +people.
> >
> > Besides moving the -rc5 to the dedicated window -rc1-to--rc5 section I wonder
> > if we shouldn't start this document with a section for maintainers mission
> > or roles and responsibilities that would cover the rest of this phrase here.
> 
> Hey, I'm just trying hard to get some of these notes merged before they
> got forgotten again. It's been eons since I lost my first notes, and
> even since I pasted these for comments. We (and you! ;) can build on
> this later!

I know, this is why I gave the rv-b already ;)

> 
> >
> >> +
> >> +When sending the last feature pull request, pass the torch to the next in
> >> +rotation to maintain drm-intel-next-queued. Move on to drm-intel-next-fixes to
> >> +follow through with the features you handled.
> >> +
> >> +drm-intel-next-fixes
> >> +--------------------
> >> +
> >> +After the last drm-intel feature pull request has been merged to drm-next,
> >> +somewhere around -rc6 or -rc7 time frame, rebase drm-intel-next-fixes on top of
> >> +drm-next, and push. Use ``dim rebase``.
> >> +
> >> +Cherry pick fixes from drm-intel-next-queued periodically. Use ``dim
> >> +cherry-pick-next-fixes``. The cherry-picks are automated based on Fixes: and Cc:
> >> +stable tags. Double check that they make sense. Use ``dim push-next-fixes`` to
> >> +push. Stop cherry-picking during the merge window, and only pick the fixes that
> >> +really need to make it to -rc1.
> >
> > I believe it is worth to explicitly recommend at least a compilation check.
> 
> Now that's one of those things that rubs me in the wrong way. It should
> go without saying you don't push without building. But if you write it
> down, it becomes the norm that's enough, and relieves you of anything
> beyond that...

hm... it makes sense...

> 
> The more you add here, the less likely it is that it'll get read.
> 
> > Another thing that I miss here is an explicit mention
> > to push branch with ``dim push-next-fixes```
> 
> It's right there...

ops, indeed.

> 
> >
> >> +
> >> +Note: ``dim cherry-pick-next-fixes`` and ``dim cherry-pick-fixes`` (see below)
> >> +use the same logic to pick patches. The former catches everything that the
> >> +latter catches. Both skip commits that have already been picked. Basically
> >> +there's a race condition where a commit could be picked for next-fixes when it
> >> +should really be headed for fixes. Communicate with the current -fixes
> >> +maintainer.
> >> +
> >> +Send drm-intel-next-fixes pull requests as needed. Check the CI results, and use
> >> +``dim pull-request-next-fixes``. The fixes will be merged to drm-next, on top of
> >> +the features heading for the next merge window.
> >> +
> >> +You can send the last pull also during the merge window, even after the main drm
> >> +pull request has been sent to Linus. Either there will be a follow-up fixes
> >> +pull, or it will be sent and merged after -rc1 as fixes.
> >> +
> >> +Pull in the (relatively rare) gvt next-fixes pull requests. Use ``dim
> >> +apply-pull``.
> >> +
> >> +Do occasional fast-forward rebases on top of drm-next as needed.
> >> +
> >> +Make sure everything you've picked has been merged upstream. If not, request
> >> +Dave to merge the missing pull requests to drm-fixes after -rc1.
> >> +
> >> +After -rc1, move on to drm-intel-fixes.
> >> +
> >> +drm-intel-fixes
> >> +---------------
> >> +
> >> +When you take over, drm-intel-fixes should be at the latest upstream kernel
> >> +release tag. After -rc1, rebase drm-intel-fixes on top of it. Rebase on the tags
> >> +only, don't rebase on arbitrary commits in Linus' master. Use ``dim rebase
> >> +drm-intel-fixes vX.Y-rc1``.
> >> +
> >> +Adopt a weekly routine for -rc fixes.
> >> +
> >> +On Monday, if the the last fixes pull has been merged to Linus' tree, rebase
> >> +drm-intel-fixes on top as a fast-forward rebase. Again, use ``dim rebase
> >> +drm-intel-fixes vX.Y-rcN``. Skip or abort the rebase if it's not a fast-forward
> >> +rebase, i.e. some of the cherry-picks have not been merged upstream. Try again
> >> +next week.
> >> +
> >> +After the rebase, give CI time to run the plain upstream tag, to detect issues
> >> +introduced by Linus' upstream.
> >
> > s/After the rebase, give CI/After the rebase, push ``dim push-fixes``. Then, give CI/
> >
> >> +
> >> +Before Wednesday,
> >
> > on my timezone ideal is tuesday end of the day or wed first thing on the morning.
> 
> i.e. *before* wednesday ;)
> 
> >
> >> cherry pick fixes from drm-intel-next-queued. Use ``dim
> >> +cherry-pick-fixes``. The cherry-picks are automated based on Fixes: and Cc:
> >> +stable tags. Double check that they make sense. Be more and more critical toward
> >> +the higher -rc, nearing stable kernel rules for commits, and drop commits that
> >> +are not needed using ``git rebase -i``. Remember, this is the fastest path to
> >> +Linus' upstream. Use ``dim push-fixes`` to push.
> >
> > Oh, it is here. I think we should move above....
> 
> What is here?
> 
> >
> >> +
> >> +If cherry-picks fail, look into the failures, and see if the commits are
> >> +needed. Send mail to intel-gfx and the authors to ask for help and/or backports
> >> +as needed.
> >> +
> >> +Pull in the gvt fixes pull requests. Use ``dim apply-pull``.
> >> +
> >> +Again, let CI crunch through the branch. If everything looks good, send the
> >> +fixes pull request Thursday at the latest. Due to time zones, a pull request
> >> +sent on Friday might not make it, missing the Sunday -rc. Use ``dim
> >> +pull-request-fixes``.
> >
> > On my timezone the ideal is Wed by end of day. Thursday first thing on the day
> > as the last resource.
> >
> >> +
> >> +After vX.Y release, rebase drm-intel-fixes one last time on that. Use ``dim
> >> +rebase drm-intel-fixes vX.Y``.
> >> +
> >> +It's time to take a break from maintaining branches,
> >
> > I just started this sweet spot :)
> >
> > From all my comments above I believe the critical ones are:
> >
> > - s/After the rebase, give CI/After the rebase, push ``dim push-fixes``. Then, give CI/
> 
> It's there.

ops, and I noticed it was there and forgot here... nevermind...

> 
> > - mention to dim push-fixes-next
> 
> It's there.
> 
> > - explicit gvt next for pull request on next-queued.
> 
> Okay.
> 
> >
> > With this in place:
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

a- more-confident-Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

> >
> > Secondary:
> >
> > - probably a mention/warn to different timezones (but no idea how to do it)
> > - rc1 to rc5 window exclusive section
> 
> I don't know what should be there.
> 
> >
> > All rest is bikesheds that could or couldn't be done later.
> >
> > Thanks for this great doc!
> >
> > Rodrigo.
> >
> >> until you start over with
> >> +drm-intel-next-queued.
> >> diff --git a/maintainer-guidelines.rst b/maintainer-guidelines.rst
> >> new file mode 100644
> >> index 000000000000..335d29a6e2bb
> >> --- /dev/null
> >> +++ b/maintainer-guidelines.rst
> >> @@ -0,0 +1,12 @@
> >> +.. _maintainer-guidelines:
> >> +
> >> +=======================
> >> + Maintainer Guidelines
> >> +=======================
> >> +
> >> +This document gathers together maintainer guidelines.
> >> +
> >> +.. toctree::
> >> +   :maxdepth: 2
> >> +
> >> +   maintainer-drm-intel
> >> -- 
> >> 2.11.0
> >> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center