Hi Andrej,
On 3/26/20 10:54 AM, Andrej Butok via TF-A wrote:
>> 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 for the feedback. That's not good, patches can't realistically
stay in review for weeks and even months, that's just not workable.
Worse, it might discourage developers to contribute to the project.
I can see that cumulating maintainer & owner roles would solve the
problem here but perhaps enlarging the pool of maintainers would as
well? Presumably, the situation is like that today because the current
maintainers of the project are overloaded and cannot get all reviews
done in a timely manner?
I am skeptical about a post-merge review process... Once a patch is
merged there is less urge and motivation (if any) for people to take a
look at it. I am worried that patches might never get reviewed that way.
Regards,
Sandrine
Hi Sandrine,
> Please find the initial proposal here:
>
> https://developer.trustedfirmware.org/w/collaboration/project-maintenance-p…
>
> Please provide any feedback you may have by replying to this email
> thread, keeping all 4 mailing lists in the recipients list.
>
> I will collate comments from the community and try to incorporate them
> in the document, keeping you updated on changes made between revisions.
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).
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.
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.
4. The platform lifecycle state machine has too many transitions. "Fully maintained" <-> "orphan" -> "out" seems sufficient to me.
Thanks,
Christian.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
Hello Pankaj,
Hope you are doing well.
The initial email point to one change, but I see that as the tip of a patch series. I reviewed https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3370
and left some comments, but did not review the complete patch series.
Are you requesting a review of the entire patch series?
-Varun
From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Joanna Farley via TF-A
Sent: Tuesday, March 31, 2020 9:37 AM
To: Pankaj Gupta <pankaj.gupta(a)nxp.com>; tf-a(a)lists.trustedfirmware.org
Subject: Re: [TF-A] [EXT] Patch-set review request: New NXP Platform LX2120ARDB support on TFA
External email: Use caution opening links or attachments
Hi Pankaj,
We will try, one of the arm team has done some +1 reviews and I have just kicked of a CI+2 run on the top of the patch stack. As you can imagine the Arm team is pressed for time the closer to the freeze date.
Other TF-A contributors you all have +1 rights so if folks have time assistance with further +1 reviews would be appreciated as that would help in accelerating confidence is getting +2 and merging.
Traditionally the project has relied on Arm folks but for some time now all contributors have had +1 rights on each other patches and the new project maintenance proposal is set up to take us in the direction of enabling all contributors helping each other.
Thanks
Joanna
From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org<mailto:tf-a-bounces@lists.trustedfirmware.org>> on behalf of Pankaj Gupta via TF-A <tf-a(a)lists.trustedfirmware.org<mailto:tf-a@lists.trustedfirmware.org>>
Reply to: Pankaj Gupta <pankaj.gupta(a)nxp.com<mailto:pankaj.gupta@nxp.com>>
Date: Monday, 30 March 2020 at 20:48
To: "tf-a(a)lists.trustedfirmware.org<mailto:tf-a@lists.trustedfirmware.org>" <tf-a(a)lists.trustedfirmware.org<mailto:tf-a@lists.trustedfirmware.org>>
Subject: Re: [TF-A] [EXT] Patch-set review request: New NXP Platform LX2120ARDB support on TFA
Please find the link to the review request.
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3370
Regards
Pankaj
From: Pankaj Gupta via TF-A
Sent: Monday, 30 March, 22:57
Subject: [EXT] [TF-A] Patch-set review request: New NXP Platform LX2120ARDB support on TFA
To: tf-a(a)lists.trustedfirmware.org<mailto:tf-a@lists.trustedfirmware.org>
Caution: EXT Email
Hi All,
Please pick this review request so that code changes can be merged before code freeze.
Thanks.
Regards
Pankaj
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
Hello team,
Please help review and merge the following bug fixes before v2.3 is released.
* https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3819: include: context_mgmt: include ep_info.h
* https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3820: Tegra: enable EHF for watchdog timer interrupts
* https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3818: Tegra: remove ENABLE_SVE_FOR_NS = 0
Thanks.
From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Varun Wadekar via TF-A
Sent: Monday, March 30, 2020 10:02 AM
To: tf-a(a)lists.trustedfirmware.org
Subject: [TF-A] RFR: Tegra fixes for v2.3
External email: Use caution opening links or attachments
Hello team,
Please help review and merge the following bug fixes before v2.3 is released.
* https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3731: Tegra: fixup GIC init from the 'on_finish' handler
* https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3770: Tegra186: increase memory mapped regions
Thanks.
________________________________
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
________________________________