[Libreoffice] breathing master

[Norbert Thiebaud]

On Sat, May 21, 2011 at 7:02 AM, Bjoern Michaelsen
<bjoern.michaelsen at canonical.com> wrote:
> Hi all,
>
> here is a proposal to improve our handling of patching and commits on
> master.
> LibreOffice is aiming for a welcoming and inviting culture to aloow
> newcomer get invaolved fast. This is why we have a very lax requirement
> on commits to master and are inviting people to post patches to the
> dev ML too.
> However, this has shown to be problematic in many ways:
> - Patches and reviews are making the dev-mailing list very noisy.
> - Newcomers are distracted by this.
> - Oldtimers elude the idea on the mailing list being integrative,
>  by filtering out patches (thus having effectively a separate patch
>  mailing list).
> - Patches are hard to keep track of in the mailing list, esp. the
>  "needs-how-many-more-reviews" question.
> - Patches wait way too long to be reviewed and pushed because of this.

I think a lot of these valid points could be help by a tool like
reviewboard you proposed.

> - Stability of master is way too low: Newcomers are driven away, if they
>  need to search and find for the commit which builds master and
>  survives basic testing.

That is a tough one. On one hand master should ideally never 'break'.
Practically there are legitimate limitations that make it happens nonetheless.
1/ Local testing is limited to your own box... even with a local
success there can be issue, on a different paltform or the same
platform with different options
2/ There is the issue that by the time you're done build-testing a
version stuff have been pushed, and that could trigger undetected
problem when you pull -r (and if you rebuild if the pull brought back
anything then you'll risk being in the same situation one build-test
later...

Sure we should aim to no-master-breakage goal... but even with the
best of intention there is no practical way to guarantee it.
(of course that is not a license to be sloppy)

> - Response time to "you broke the master"-emails by tinderboxes are way
>  too low -- the default assumption seems to be: somebody else broke the
>  master. This is having further implications as per
>  http://en.wikipedia.org/wiki/Broken_windows_theory
>
The onegit conversion should help a bit with that. There are instance
when buildbot fail due to unlucky pull-timing
False positive induce a 'cry-wolf' syndrome.
Another issue is that breakage sometime cover multiple commit from
multiple committer. that will be improved with onegit as well,
as it will be possible to do bisection.
We also need to find a way to tell people that were alerted of a
breakage when that breakage is solved. in other word no-news is _not_
good news

Finally, although I haven't thought out the details yet, I think there
should be a quarantine mechanism, where newly pushed commit are staged
until they pass tinderbox test.
That can only work if we have fairly stable tinderbox, with some level
of redundancy, so that a centralized tool can determine if a give
commit has passed validation from
the minimum requirement. Once a commit level is validated as not
'breaking', then it could be pushed/picked/merged/whatever to the
'True master(tm)'
Note that this concept is somewhat similar to your proposed
'unreviewed branch', except that I would suggest that this apply to
_every_ commit not just email submitted patches.

Another route is the 'sub-system maintainer' route. where patch and
commit are channeled to sub-system maintainer that regularly but in a
controlled fashion push batch of commits to the 'official' master.
For instance 'calc'-centric patch could transit via a tree managed by
kohei, he would make sure that he tree is stable (via builtbot among
other tings) and weekly - for instance - would merge his tree into the
'official' master.
Note that a subsystem tree can, and probably' have more than one
maintainer to insure continuity... It is just that maintainers of a
common tree should communicate and trust each-other well enough...
In that model there could be a loosely/commonly managed 'catch-all'
tree that would correspond to the 'unreviewed' branch you mention
below.

Note: to give an idea of the volume for the tinderbox: last week (from
the 14 to now), my Gentoo tinderbox did 137 build. taking into account
for commit dump (like dtardon do once in a while), that would have
been around 200 build a week. that box alone can manage about
500-600/week
Of course Windows build is a different story... but with MinGW and
enough box in // there is still a way. beside we could validate
multiple patch at once... the need for bisection is only if there is a
breakage.

> Here is a proposal on how to solve this:
> - Create an branch named "unreviewed" branching off from master on
>  every Tuesday 00:00 UTC.
> - Any submitted patch is pushed to that branch as is without review from
>  Tuesday until Saturday, the only condition is a proper license.
How do you deal with patch that do not apply ?
How do one do to submit a patch that conflict with a previously pushed
'unreviewed' patch.
If you are second and don;t take it into account your patch won't apply.
If you do take it into account, the acceptance of your patch becomes
dependent on the decision
on the previous patch. if the later get dropped during review, the
former won't apply to master anymore
What do we do then ? send the otherwise good patch to the author for
tweaking ? ask the maintainers (who-ever cherry pick) to fix the patch
?
(I would favor dropping it. 1/ if will encourage people to avoid big
spread-out patches 2/ the author of the patch is presumably the best
qualified
to fix a conflict his patch has)

>  Patches from Sunday and Monday will wait to get pushed on Tuesday.
> - The master branch will be open for commiters from Monday to Saturday
That is a problem. if people that send patch to the ML create these
patches based on master... these may not apply
to the 'unreviewed' branch that was branched out Tuesday. if they base
it on the 'unreviewed' branch then these
commit might to apply to master the next monday by the virtue of
master having changed between tuesday to monday...


>  UTC for all commits, Sunday UTC is reserved for stabilzation: Only
There is not that many people active on Sunday...

>  commits fixing build breakers and test breakers are allowed. On
>  Sunday evening, every tinderbox should be shiny green. If you
>  absolutely need to commit something else on Sunday, use a feature
>  branch.
> - Tinderboxes should also do one build of the "unreviewed" branch on
>  Sunday, to identify obvious build or test breakers.
> - On Monday, the commits on the "unreviewed" branch are being
>  collectively reviewed and discussed on IRC. Accepted patches are

And the reciprocal problem of Sunday.... a lot of email submitted
patch are posted
by volunteers, that may not have the liberty to hang around IRC on weekday...
Not to mention timezone issues.

>  cherrypicked to master, results are posted to the mailing list, the
>  "unreviewed" branch is deleted. Rejected patches can be fixed an
>  resubmitted.
>
> This would:
> - Prevent patches to get lost in space on the ML.
> - Prevent patches to hang in a "needs one more review" cycle.
> - Limit the waiting time for a patch review to maximum one week.
> - Patch submitters can be around on IRC at review time and clarify
>  questions and receive feedback directly. Otherwise little changes for
>  them.
> - It is easier to identify and find a domain expert when doing a "group
>  review"
> - Reduce the noise on the dev mailing list.
> - Newcomers can be sure that a Sunday evening checkout of master is
>  stable.
> - The "learning experience" by patch reviews will actually be improved:
>  There will be a good summary of all the reviews done on Monday,
>  instead of lots of tiny bits sprinkled here and there over the mailing
>  list.
> - The only drawback is the limitation to direct commits on Sunday, but
>  there is little activity anyway on Sunday. (And if it is not a
>  buildbreaker/test fix, how can it be so important that it cant wait
>  for the next day?)
>
> This workflow gives our review process a bit more stucture without the
> need for new tooling (with new accounts etc.) like reviewboard, but
> instead really uses the power of our existing tooling.
>
> Opinions?

I'd really like to get a better idea on the viability of the 'extra'
tooling like reviewboard.
That kind of tool, I hope, could remove a lot of chaos out of the
mailing list (especially the multiple-review things)
Then, we can review and refine the process to see what other
'structure' we need to put
in place to achieve the delicate balance between
stability/productivity/ease-of-access ...
Furthermore, the work-flow you propose would still require some
tooling (albeit homegrown ones)
to at least manage the 'monday' review efficiently and thoroughly.
(making sure they are all reviewed, having means so that people
participating in the review can quickly
and efficently see the patch we are talking about, cherry picking it,
sending email to the author and the ML
indicating the fate of the patch, and in case of rejection, possibly
hints as to why...)


Norbert