[haiku-development] Re: Haiku R1A4 Postmortem
- From: Ingo Weinhold <ingo_weinhold@xxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Mon, 10 Dec 2012 15:17:26 +0100
On 12/10/2012 09:32 AM, Stephan Aßmus wrote:
I think the problem here is how you would enforce the review process. Ingo's
argumentation is based on the assumption that the review of each single commit
going into trunk is absolutely necessary before merging it into the release
branch, regardless of who does the merging. And that is why he thinks your
proposal makes it even harder, even more work for the release manager, or
whoever is responsible for review, if the commits already went into the release
branch before being reviewed.
Well, more or less, though I want to clarify that I don't mean a
line-by-line code review, which I don't think is realistic for us. We
tried that for alpha 2 and it didn't work all that well. I was working
full time on Haiku and I reviewed most of the patches concerning areas I
felt even remotely confident in. There where still quite a few patches
that didn't get any review, or at least not fast enough for the process
(i.e. they got merged very late into the branch).
The kind of "review" I would expect the RM (or RM team) to do is for
each commit to understand what it fixes or what feature it adds -- not
on a technical level, but on a "ticket" level (e.g. hypothetically: not
"register foo of USB OHCI controller isn't initialized correct in
situation blah", but rather "a certain kind of USB mice doesn't work
with certain USB controllers") -- and judge from that whether at the
current point in the release process, integrating the commit is a good idea.
I would argue that the review and cherry picking of commits is already where
our process is flawed. 1) the RM may not be the best person to review patches
depending on which component they affect and
As written above, for the kind of review I think of the RM doesn't need
to be familiar with the affected component, or, in fact, the RM doesn't
necessarily have to a developer at all (though technical knowledge
certainly doesn't harm to be able to judge some commits better). IMO the
important point is that someone who is primarily looking at things with
a release process perspective is deciding what goes into the release branch.
Obviously as a developer you want your fix included. Since it fixes
things, right? The RM's task is to weigh that against the possible
consequences for the release. Is there enough time for testing (e.g. for
finding side effects unthought of)? Is the problem serious enough to
rectify risking possible regressions? Etc.
I think the somewhat biased perspective of the developers and the often
not entirely obvious assessment of commits makes the "laying down rules
and have them followed by the developers" approach impractical.
Also, to beat that undead horse again, having only retroactive opt-out
powers (besides defining the policy) always puts the RM behind. There
would always be X more commits that are already in the release branch
but haven't been cleared by the RM yet. That adds unnecessary pressure.
A RM with proactive opt-in powers does least ensure that at any time
only desired commits are in the release branch, even if there's a backlog.
2) (most importantly) it's just too much work for a single person. Each
previous RM seems to agree with that last point.
Well, except me, apparently. Though, admittedly, I had the luxury of
being available full time.
To me, Ingo's Landon's and Axel's arguments sound directed at an ideal
situation. They seem less targeted at the actual Haiku situation. The arguments
incorporate theoretical problems that we didn't actually face during release
preparation (like developers trying to get half-finished work into the release),
I don't think these are theoretical problems only. And it's not really
about half-finished work, but rather about the situation mentioned
above, developers not having an unbiased "for the good of the release"
perspective.
I totally agree with Axel that a lot less commits should go into the
release branch. The less we commit there the less likely are regressions
and the more effective testing becomes. Which, I hope also helps to ...
and they neglect work power problems that we do have.
... address this problem.
My proposal would be to resurrect something like the old team leader concept. Instead of
having only one RM, we have several people responsible for components. Depending on which
component a commit affects, these people should be the ones to do the review and give
their OK. The people should match up nicely with the default ticket owners in our Trac.
And instead of a single person doing the merging, the original developer should do the
merge, after a component owner has posted "+alpha5" on the commit list,
presumably after having reviewed the commit. How does that sound?
If you mean code review, then that would only take care of the technical
aspect and the not the release aspect. Besides, as already stated, I
don't consider a strict code review policy realistic.
If you mean the RM "review" I described above, this would effectively
mean we'd have to have half a dozen or so people with a "release
mindset". Likely the same ones who produce a good deal of the commits in
the first place. IMO a contradiction.
CU, Ingo
Other related posts:
