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