[TF-TSC] [TF-M] Project Maintenance Proposal for tf.org Projects

Joakim Bech joakim.bech at linaro.org
Wed Apr 1 08:08:26 UTC 2020


Hi Christian, Sandrine, all,

On Thu, Mar 26, 2020 at 10:27:14AM +0100, Sandrine Bailleux wrote:
> Hi Christian,
> 
> Thanks a lot for the read and the comments!
> 
> On 3/25/20 7:05 PM, Christian Daudt wrote:
> >  �The maintenance proposal looks great ! I have some feedback on
> > specific portions:
> >  �1. maintainer/owner/author patches. " Note that roles can be
> > cumulative, in particular the same individual can be both a code owner
> > and a maintainer. In such a scenario, the individual would be able to
> > self-merge a patch solely affecting his module, having the authority to
> > approve it from both a code owner and maintainer's point of view.": I'm
> > always leery of people self-approving their patches. At a minimum, all
> > self-patches should be published and a minimum wait time provided for
> > feedback. Or preferably that another maintainer does the merge (it does
> > not need to be mandated but should be suggested).
> 
> Yes, actually this is something that generated some disagreement inside Arm
> as well and I am glad you're bringing this up here, as I'd like to hear more
> opinions on this.
> 
> I too have concerns about allowing self-reviewing. I am not so much
> concerned about people potentially abusing of this situation to silently
> merge patches, as I think we should trust our maintainers. But I am worried
> that a self-review is rarely as good as a peer review, simply because it is
> so easy to miss things when it's your own work. I believe several pair of
> eyes is always better, as different people think differently, have different
> perspectives and backgrounds, and are able to catch different issues.
> 
> But to pull this off, we need enough people to do all these reviews. The
> proposal currently allows self-review because some of us feared that
> mandating 2 reviewers for every patch (especially pure platform patches)
> would be impractical and too heavyweight, especially for the TF-M project in
> its current contributors organization, as I understand. It would be great to
> get more feedback from the TF-M community as to whether they think it could
> work in the end.
> 
> It's a difficult balance between having the best possible code review
> practices, and realistically getting all the review work done in a timely
> manner, avoiding bottlenecks on specific people and keeping the flow of
> patches smooth.
> 
> I like your idea of a minimum wait time provided for feedback. I think it
> could be a good middle ground solution.
> 
+1 for that, after silence for X weeks it should be OK to merge the
patch. X would need to be number that is high enough for people to have
a chance to find it and look into it, but shouldn't be too high, since
there is a risk that it'll force the contributor to pile up things that
might be dependent on this patch. To throw something out, I'd say ~2
weeks sounds like a good number to me.

> Your other suggestion of having a different maintainer doing the merge would
> work as well IMO but requires more workforce. Again this comes down to
> whether this can realistically be achieved for each project. This solution
> was actually suggested within Arm as well (and even called out at the end of
> the proposal ;) ).
> 
> Bottom line is, in an ideal world I would like to condemn self-review
> because I consider this as bad practice
+1 

> , but I do not know whether this will
> be practical and will work for TF-M as well.
> 
> >  �2. 'timely manner': This expectation should be more explicit - when
> > the author can start requesting other maintainers to merge on assumption
> > that silence == approval (or not). Such timeliness expectations are
> > probably best set per project however.
> 
> Yes, "timely manner" is definitely too vague and was actually left that way
> on purpose at this stage to avoid touching upon what I think is a sensitive
> subject! I am aware that some patches sometimes spend a long time in review,
> definitely longer than they should and it understandably generates some
> frustration. This is something we absolutely need to improve on IMO and
> hopefully a bigger pool of maintainers will help solve this issue. But I
> agree that the expected review timeline should be clearly established and it
> is probably best to let each project decides theirs.
> 
> >  �3. The proposal does not address branching strategies. i.e. will
> > there be separate maintainers for dev/master/stable branches? I don't
> > think it needs to address it yet - keep it simpler for a start. But a
> > todo saying something like "in the future this project maintenance
> > proposal might be expanded to address multi-branch maintainership" would
> > be good.
> 
> Good point. A todo sounds good, I will add one in the last section of the
> document.
> 
> >  �4. The platform lifecycle state machine has too many transitions.
> > "Fully maintained" <-> "orphan" -> "out" seems sufficient to me.
> 
> Hmm OK. There might be too many transitions but I feel we need something
> between fully maintained and out, i.e. the limited support one.
> 
> Julius Werner also pointed out on Thursday that orphan might be misplaced,
> as all these other stages deal with some degrees of feature support (what's
> known to work), whereas orphan is an orthogonal topic that is not directly
> related to the level of supported features. For example, a platform could
> have recently become orphan but all features and tests still work for some
> time.
> 
At one point in time in the OP-TEE project we tried to keep track of
maintained platforms, by simply saying maintained "Yes" if they are
maintained. However they're not maintained, we indicated that by stating
the last known version where a platform was maintained. People can still
find that information here [1] (not up-to-date). The intention was to
give future users of an old platform a chance to know if it ever has
been supported and what version that was. That could serve as a starting
point in case someone is interested in bring a device/platform back to
life.

How that works in practice is that all OP-TEE maintainers are adding
their "Tested-by" (see example [2]) tag for the platform they maintain
when we're doing a release. If there are platforms with no "Tested-by"
tag, then they simply end up with the "last known version".

However, to keep that up-to-date, it requires some discipline from the
people maintaining such a table ... something that we in the OP-TEE
project haven't been very good at :)

So, I'm not proposing something, it's just that I wanted to share what
we've tried and it "works", but not easy to maintain (a release
checklist could fix that).

[1] https://optee.readthedocs.io/en/latest/general/platforms.html
[2] https://github.com/OP-TEE/optee_os/pull/3309/commits/765b92604459240bed7fcf051f9621a89934032e

-- 

Regards,
Joakim


More information about the TSC mailing list