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/765b92604459240bed7fcf0...