Hello,

But I am worried that a self-review is rarely as good as a peer review

On practice, unfortunately, some TF-M tasks are waiting weeks and even months for review and following approvals. If I were a maintainer & owner of my own TFM area, I do not want to wait & push & remind somebody else. Better to have a post-merge review for these cases, which does not limit and slow down the development.

Thanks, Andrej Butok

-----Original Message----- From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Sandrine Bailleux via TF-M Sent: Thursday, March 26, 2020 10:28 AM To: Christian Daudt Christian.Daudt@cypress.com; tf-a tf-a@lists.trustedfirmware.org; tf-m@lists.trustedfirmware.org; tsc@lists.trustedfirmware.org; op-tee@linaro.org Cc: nd@arm.com Subject: Re: [TF-M] Project Maintenance Proposal for tf.org Projects

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.

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

Regards, Sandrine -- TF-M mailing list TF-M@lists.trustedfirmware.org https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.trus...