[flashrom] New development guidelines after 0.9.7

[Stefan Tauner]

On Fri, 19 Jul 2013 00:30:52 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 18.07.2013 03:21 schrieb Stefan Tauner:
> > I would also like to introduce some kind of stable branch where we can
> > commit important backported patches, if need be, without releasing all
> > non-critical fixes committed in the meantime (as was the case with
> > 0.9.6.1 for example). I would not have a problem doing this with git,
> > but I am not so sure if I want to try that with svn... let's pretend
> > there is a community consensus to continue with svn for now.
> > Is that sensible to do? AFAIK this would produce odd revision numbers
> > (which is one of the main arguments pro svn).
> 
> svn branch management is actually quite simple, and revision numbers are
> still linear (though with gaps in each branch).

that's what I would call odd. it is still better comprehensible than
hash numbers, but I already dislike the missing revisions for tags and
not knowing which branch a commit belongs to makes those numbers pretty
useless to me.

> Quite a few projects have a different version number scheme for stable
> and development branches to make differentiating easier. […]

Why would we care? I don't plan to release anything else than stable
releases. I should have explained that in the mail and it should
probably end up in the wiki.

My suggestion would be to not change the release management too much.
The only difference actually is that I would like to extend the
stabilization phase without prohibiting merging new patches. How would
that work? By branching at the beginning of the stabilization phase
(which is quite similar to what we did with 0.9.5 and 0.9.6 by accident
more or less :) - this should probably be tagged as RC.

Example:
We will eventually tag 0.9.7 and start to merge patches according to
the new rules. Every merged patch is also a candidate for the stable
branch (on top of 0.9.7 to end up in a 0.9.7.1 release if need be). I
have not thought about a process for that, but as conservative as we
are it is rather obvious what we would want to merge there... fixes for
regressions of earlier stable releases for example. I don't think we
need extra rules for that and there will be enough time to discuss such
patches (i.e. no timeout rule needed).

The decision about when to release stable updates can probably be
reached together with the discussion of which individual patches to
merge with stable. If we really need a written rule I would suggest
tagging a release after each patch merged to the stable branch (maybe
with a cool-down phase of 24 hours).

Then after a while (e.g. the usual 6-12 months) we will want to do a
new release. We create a new branch from the current dev branch and tag
it as 0.9.8-rc1. We continue to merge patches according to the rules to
the dev branch, but merge them on top of the rc too if need be. If need
be we tag more rcs on the rc branch and eventually the new stable
release.

Rinse, repeat. No obvious semantic changes to release numbering.

> > I would also like to clean up the commit permissions and name the ones
> > with commit rights in the wiki.
> 
> Listing the active committers in the wiki is an excellent idea from a
> publicity perspective, but will it make breakins easier for attackers?
> There's always svn log to find out the committers, so the security
> argument is probably moot.

Security through obscurity and that from you? I take that as a joke.
BTW how about signed commits and transfers via ssh? :P
Really this shouldn't be about security.

> > IMHO the following ppl should have commit rights:
> > carldani, stefanct, dhendrix, kmalkki, idwer
> 
> Yes. I'd also like to have ruik for IMC stuff, and xvilka for reverse
> engineering stuff, and Mathias Krause and Nico Huber had some good
> patches in the past as well as some still review-pending patches.

I see commit rights to be equivalent to having some kind of maintainer
status. That requires all these points:
- must be active on the ml or IRC on a regular basis (and by active I
  dont mean just being subscribed or lurking in the channel :)
- must know at least parts of the code very well
- must contribute to patch discussions/reviews on a regular basis

Contributing patches alone is not enough for that or not even required
actually. Nico's patches are good no question, but that's all I have
seen from him. Similarly I would not deem Mathias an active member of
the community.

> > I am not sure about uwe and twice11, because they are (sadly) not even
> > active on IRC or the ml.
> 
> From a patch-writing perspective, uwe has been inactive about a year
> longer than twice11. That said, I wouldn't disable their accounts just
> yet. It feels wrong to do that.

As said, patch writing is not so important to me.

> There's also stepan and oxygene. Hm. Neither of them are a security risk
> (it's stepan's server and oxygene is/was doing some maintenance/admin
> stuff on the server as well).

Every unused login is a security risk. But that's not what I am
thinking about. Those with commit rights should be the active members,
that do the reviews, the merging and the ones that can be mailed, asked
and nagged about things, i.e. it is a responsibility too. That's also
why I would like to mention them in the wiki. What should a contributor
do if no one reviews his patch in time to get his patch in? He should
nag someone with commit rights and hence this should be obvious.
And it is not obvious atm... I have no idea who (still) has commit
rights. Everyone who ever did a commit?

> > = Reviews =
> >
> > Contributors without commit rights should receive at least a preliminary review within one week of submission by some committer.
> > At minimum this should include a broad indication of acceptance or rejection of...
> > * the idea/rationale/motivation,
> > * the implementation
> > respectively.
> >
> > In general, reviews should focus on the architectural changes and things that affect flashrom as a whole.
> > This includes (but is by no means limited to) changes in APIs and types, safety, portability, extensibility, and maintainability.
> > The purpose of reviews is not to create perfect patches, but to steer development in the right direction and produce consensus within the community.
> > The goal of each patch should be to improve the state of the project - it does not need to fix all problems of the respective field perfectly.
> 
> Do we also want a no-regression policy or is a patch allowed to break
> some things to improve other things as long as a fixing followup patch
> is in the queue?

I am not sure I want to have too many rules. The reason I put that
sentence in was to remind everyone that perfect patches are hard and
take a long time (and still have bugs).

> > NB: New contributors may need more detailed advices and should be told about minor issues like formatting problems more precisely.
> > The result of a review should either be a 
> >
> > == Acked-by ==
> >
> > * Trivial stuff like...
> > ** compile fixes (that are tested on all or at least some affected and some ''unaffected'' platforms),
> > ** marking boards/chips/programmers as tested, and
> > ** typo and whitespace fixes etc.
> > :do not need an Acked-by line from someone else than the author.
> > * If the patch is something like a board enable, new chip(set) support etc. (i.e. no refactoring, no architectural impact), you can get an ack from someone who is not an experienced developer after he/she has tested the patch, or ack yourself if proof exists that it worked.
> 
> Even if the new code is copy-pasted instead of adding a new switch/ID to
> existing code?

Ask the original author, I just rephrased that point a bit :P
No rule can make common sense obsolete... if copying code is the saner
choice in some situations then it is the right thing to do. And after
all the refactoring I have done I think I have the right to say that :)

> > * However, for more complicated patches you should get a review from someone who knows the code well if possible.
> > ** If no one replies to the patch submission on the mailing list within 2 weeks, you may assume consent by the community and ack yourself.
> 
> While I'm not totally happy about this, it's the only way to remove the
> no-review roadblocks. And with a separate development branch, we can
> always have a stabilization phase shortly before a release where we
> tighten the rules a bit.

No one is happy about the current situation either.
I hope the release management stuff is clear now... 

> > * Committing
> > ** If you ack something and the sender has commit rights, let him commit unless he asks you to commit.
> > *: If the sender does not have commit rights, wait at least 24 hours after sending the ack to give him/her an opportunity to add final refinements or comments.
> 
> A notice to the sender "acked-by <me>, will commit within 24 hours
> unless you want to change or cancel the patch" would be nice.

This should be the semantics the receiver should read when getting an
ack. But it certainly does not hurt if we write that in the ack mail,
yes.


I hope everything is somewhat clear... 3am, gn8
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner