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

Sandrine Bailleux sandrine.bailleux at arm.com
Wed Apr 1 09:46:20 UTC 2020


Hi Joakim,

On 4/1/20 10:08 AM, Joakim Bech via TSC wrote:
> 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.

Yes, I think such information can be very useful. It saves some "git 
archeology" effort to try and dig this information afterwards. Also, 
when someone starts looking at a project, I would expect this to be one 
of the first thing they look up, they would want to know in which shape 
the project is for the particular platform they are interested in. 
That's almost as important in my eyes as a "getting started" guide.

We could have such a high-level table that just says whether a platform 
is supported or not (just a yes/no) and have complementary, per-platform 
documentation that goes into the details of what features are supported 
exactly.

> 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".

I think that's a very good idea!

> 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 :)

Can't this be automated, such that it doesn't need to be manually kept 
up-to-date? I imagine we could have some tools generating the platform 
support table out of such a commit message.

> 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
> 


More information about the TSC mailing list