Hi,
could someone tell me how I'm supposed to deal with unresponsive reviewers on gerrit? Clearly I must be doing something wrong as the patches have been gathering dust for around three years now.
https://review.coreboot.org/q/topic:%22macronix-spi-bp%22
--Daniel
Hi Daniel,
thanks for the constructive feedback. The coreboot community is a very active community. We review hundreds of patches every day and I am sorry that we missed to review your patches.
I guess the best idea here is to ping people on either IRC, Matrix or Slack. Also a friendly question if someone has time to review your patches via the mailing list is for sure feasible. Especially if you point out that you have a high interest in getting the patches in.
A good idea is also to review patches on gerrit in order to take more work load _off_ the coreboot reviewers so they eventually have more time for your patches. As always, you get back what you put in.
Hope that helps!
Chris
On 9/29/23 11:41, Daniel Gröber wrote:
Hi,
could someone tell me how I'm supposed to deal with unresponsive reviewers on gerrit? Clearly I must be doing something wrong as the patches have been gathering dust for around three years now.
https://review.coreboot.org/q/topic:%22macronix-spi-bp%22
--Daniel _______________________________________________ coreboot mailing list --coreboot@coreboot.org To unsubscribe send an email tocoreboot-leave@coreboot.org
Hello Daniel,
Thank you for contributing to the coreboot project. Each contribution is important and should be taken care of.
could someone tell me how I'm supposed to deal with unresponsive
reviewers on gerrit?
Typically patches are reviewed in few days from assigning reviewers assuming they are available at the time. If they are not, then emails might get buried in their mailbox. If that case you can simply ping reviewers by adding general patch comment with "@Name" for their attention. If you have problems with finding maintainers in MAINTAINERS file you can use ./util/scripts/get_maintainer.pl script to find potential candidates.
Clearly I must be doing something wrong as the patches have been
gathering dust for around three years now.
There might be few reasons for that. First, for patch to be eligible for review it cannot have merge conflicts as resolving them might change patch contents and will most certainly drop all CR+ scores and invalidate some comments. You should try keep patches in mergable state. Second, submitting long patch trains usually makes reviewers to halt until previous patches are reviewed, as changes in patches earlier in the train might invalidate their time put into reviewing. I'd suggest focusing on earlier patches first, merging them and gradually moving to later ones.
Best Regards, Jakub
On Fri, Sep 29, 2023 at 2:15 PM Christian Walter < christian.walter@9elements.com> wrote:
Hi Daniel,
thanks for the constructive feedback. The coreboot community is a very active community. We review hundreds of patches every day and I am sorry that we missed to review your patches.
I guess the best idea here is to ping people on either IRC, Matrix or Slack. Also a friendly question if someone has time to review your patches via the mailing list is for sure feasible. Especially if you point out that you have a high interest in getting the patches in.
A good idea is also to review patches on gerrit in order to take more work load _off_ the coreboot reviewers so they eventually have more time for your patches. As always, you get back what you put in.
Hope that helps!
Chris On 9/29/23 11:41, Daniel Gröber wrote:
Hi,
could someone tell me how I'm supposed to deal with unresponsive reviewers on gerrit? Clearly I must be doing something wrong as the patches have been gathering dust for around three years now. https://review.coreboot.org/q/topic:%22macronix-spi-bp%22
--Daniel _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
-- *Christian Walter* *Head of Firmware Development / Cyber Security *
9elements GmbH, Kortumstraße 19-21, 44787 Bochum, Germany Email: christian.walter@9elements.com Phone: *+49 234 68 94 188 <+492346894188>* Mobile: *+49 176 70845047 <+4917670845047>*
Sitz der Gesellschaft: Bochum Handelsregister: Amtsgericht Bochum, HRB 17519 Geschäftsführung: Sebastian Deutsch, Eray Basar
Datenschutzhinweise nach Art. 13 DSGVO https://9elements.com/privacy _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Hi Jakub,
On Fri, Sep 29, 2023 at 03:17:23PM +0200, Jakub Czapiga wrote:
Clearly I must be doing something wrong as the patches have been
gathering dust for around three years now.
There might be few reasons for that. First, for patch to be eligible for review it cannot have merge conflicts as resolving them might change patch contents and will most certainly drop all CR+ scores and invalidate some comments. You should try keep patches in mergable state.
My patches all depend on one another, Gerrit seems to treat this as "conflict" when this is entirely normal so I don't see how I can do what you suggest.
I (used to) frequently rebase against master to make sure actual conflicts don't stay there for long FYI.
I push these using HEAD:refs/for/main as was documented last time I checked (3y ago) not sure what else is expected of me.
Second, submitting long patch trains usually makes reviewers to halt until previous patches are reviewed, as changes in patches earlier in the train might invalidate their time put into reviewing. I'd suggest focusing on earlier patches first, merging them and gradually moving to later ones.
Another proplem here being that gerrit makes it very hard to figure out what the "first" patch is anyway. They get shuffled into a random order every time I push the series.
Am I as the submitter supposed to draw the reviewer's attention to which is the first patch or what? That can't possibly be right. The gerrit interface sure makes it seem like reviewers are notified of the fact that they're supposed to do something. UI lies I suppose.
Honestly this gerrit thing may work OK for companies where people are properly incentivized to do their assigned reviews or people in the in-group that know who to poke to get things moving, but for outside contributors the experience is even more fundamentally broken than the email/ML patch workflow and that's saying something.
Consider this as a point of project self-interest. I would have probably been busy writing/reviewing coreboot patches for the past three years if my contributions didn't end up getting dropped on the cherry-pickin room floor.
As I see it: I could have just bought a 50c Winbond flash chip and called it a day, yet here I am refactoring code, adding vendor support and writing tests for security critical code. Isn't that exactly the type of highly motivated individual contributor the coreboot project should be trying to attract?
Well your infrastructure/processes are getting in the way of that.
Please do better <3
--Daniel
29. September 2023 15:52, "Daniel Gröber" dxld@darkboxed.org schrieb:
My patches all depend on one another, Gerrit seems to treat this as "conflict" when this is entirely normal so I don't see how I can do what you suggest.
I (used to) frequently rebase against master to make sure actual conflicts don't stay there for long FYI.
I push these using HEAD:refs/for/main as was documented last time I checked (3y ago) not sure what else is expected of me.
That's the normal flow, yes. The merge conflict messages appear because we use a cherry-pick strategy, so every change _could_ be applied to HEAD directly (even if that's not normally done in patch trains like yours) - as such it makes sense to do the merge conflict detection the same way, too.
So: nothing else to do here.
All the best, Patrick
Hi Daniel, Let me start this overly large email by saying that I appreciate you saying something. We don't get enough feedback about issues and what we need to improve. That said, not getting reviewers on all patches is unfortunately somewhat of a known issue that we're actively working to correct, though we could definitely use help in that regard. Here are some thoughts:
# New Contributor Hashtag One thing we're doing now that we didn't do three years ago is identifying new users so that we can pay attention to helping people who aren't as familiar with the community. You can see those patches here:
https://review.coreboot.org/q/repo:coreboot+status:open+hashtag:new_contribu...
# Greeting new users I'm also sending a greeting to new users (people with fewer than 5 commits), which unfortunately wouldn't have helped your series of SPI patches.
Here's a sample greeting: ``` Hi, It looks like you haven't contributed to the coreboot project before.
Welcome, and thank you for the patch. We hope that this is just the first of many.
Please let us know if there's anything we can do to help get your first sets patches merged as you get used to the contribution process.
The coreboot project has a hands-off policy regarding other people's patches so nobody here is going to update the contents without your permission. If you would you like someone to take over your patches at any point, please just post a comment to that effect on the specific patch.
You might find the coding style guide and the gerrit guidelines useful to read.
https://doc.coreboot.org/contributing/coding_style.html https://doc.coreboot.org/contributing/gerrit_guidelines.html
If you want to talk with anyone, you can talk to developers on one of the many options we have:
https://doc.coreboot.org/community/forums.html Again, please let us know if you have any questions, or if there's anything we can do to help. ``` If you have any thoughts on how to improve the greeting, I'd appreciate any feedback.
# Search for non-corporate users We also have a list of corporate users so that we can search for non-corporate users. This is to help people who don't have coworkers backing them up to ask for reviews.
https://review.coreboot.org/q/repo:coreboot+status:open+-ownerin:corporate
# Maintainers list As mentioned previously, we have a list of maintainers for various areas.
Unfortunately, we don't have a maintainer for the SPI code, so we don't have anyone who has said they'll take responsible for reviewing a number of your patches.
Additionally, the people who are on the maintainers list frequently maintain a LOT of things simply because we don't have enough really experienced people to review everything that gets pushed. I'm honestly overwhelmed by the number of reviews I get added to. I think I have over 300 in my queue right now.
# Gerrit as an issue You mention that you see gerrit as a problem, but don't think the issue is gerrit. My guess is that you're simply not familiar enough with it to use the features it offers properly - This isn't meant to be a slight towards you, there is definitely a learning curve.
If you click on any of your patches, you should see a section called "Relationship Chain" generally in the upper middle right of the page. That's the patch train, in order. The first page that you see is simply ordered as the list of patches most recently touched in some fashion. It's not meant to show any sort of relationship.
# We need ideas on *how* to improve It's fine to say that the coreboot project should "do better", and you're not wrong, but without something concrete that we can actually do, that's not something we can just agree to do.
The coreboot project does not have any paid employees to do the work, and without getting significantly more contributions to support someone, I don't see that happening anytime soon. There are a number of companies supporting work for coreboot, but they're all supporting engineers responsible for their particular sections. Any work done in other areas for the community good, is generally done by volunteers. Asking unpaid volunteers to be strictly responsible for things is difficult. This is a problem that many open source projects have.
# coreboot Leadership Meeting I'd really like to ask you to attend the leadership meeting - we hold it every two weeks. You can find the time and how to join on the coreboot calendar: https://coreboot.org/calendar.html
Thanks again for saying something about the problem. We do want to improve and make things better for everyone.
Martin
Sep 29, 2023, 07:53 by dxld@darkboxed.org:
Hi Jakub,
On Fri, Sep 29, 2023 at 03:17:23PM +0200, Jakub Czapiga wrote:
Clearly I must be doing something wrong as the patches have been
gathering dust for around three years now.
There might be few reasons for that. First, for patch to be eligible for review it cannot have merge conflicts as resolving them might change patch contents and will most certainly drop all CR+ scores and invalidate some comments. You should try keep patches in mergable state.
My patches all depend on one another, Gerrit seems to treat this as "conflict" when this is entirely normal so I don't see how I can do what you suggest.
I (used to) frequently rebase against master to make sure actual conflicts don't stay there for long FYI.
I push these using HEAD:refs/for/main as was documented last time I checked (3y ago) not sure what else is expected of me.
Second, submitting long patch trains usually makes reviewers to halt until previous patches are reviewed, as changes in patches earlier in the train might invalidate their time put into reviewing. I'd suggest focusing on earlier patches first, merging them and gradually moving to later ones.
Another proplem here being that gerrit makes it very hard to figure out what the "first" patch is anyway. They get shuffled into a random order every time I push the series.
Am I as the submitter supposed to draw the reviewer's attention to which is the first patch or what? That can't possibly be right. The gerrit interface sure makes it seem like reviewers are notified of the fact that they're supposed to do something. UI lies I suppose.
Honestly this gerrit thing may work OK for companies where people are properly incentivized to do their assigned reviews or people in the in-group that know who to poke to get things moving, but for outside contributors the experience is even more fundamentally broken than the email/ML patch workflow and that's saying something.
Consider this as a point of project self-interest. I would have probably been busy writing/reviewing coreboot patches for the past three years if my contributions didn't end up getting dropped on the cherry-pickin room floor.
As I see it: I could have just bought a 50c Winbond flash chip and called it a day, yet here I am refactoring code, adding vendor support and writing tests for security critical code. Isn't that exactly the type of highly motivated individual contributor the coreboot project should be trying to attract?
Well your infrastructure/processes are getting in the way of that.
Please do better <3
--Daniel _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Hi Martin,
On Fri, Sep 29, 2023 at 08:27:43PM +0200, Martin Roth wrote:
# New Contributor Hashtag # Greeting new users
Sounds good. Not sure I can add anything here other than:
Here's a sample greeting:
If you would you like someone to take over your patches at any point, please just post a comment to that effect on the specific patch.
Would a comment like that on a random patch actually get noticed by anyone?
# Maintainers list
As mentioned previously, we have a list of maintainers for various areas.
Unfortunately, we don't have a maintainer for the SPI code, so we don't have anyone who has said they'll take responsible for reviewing a number of your patches.
Additionally, the people who are on the maintainers list frequently maintain a LOT of things simply because we don't have enough really experienced people to review everything that gets pushed. I'm honestly overwhelmed by the number of reviews I get added to. I think I have over 300 in my queue right now.
I like to philosophize about incentives. So what incentives does a contributor have to get themselves added to that maintainers list?
The cynic in me says: none. It's just hard work nobody wants to do, but that doesn't explain the fact that there are in fact plenty of mostly-volunteer projects with a thriving development community.
My current theory is, this happens when people see some kind of social reward for doing that hard work.
As I see it most review systems designed for use by companies dehumanize the experience too much. Remaining an employee is usually plenty of motivation for people to put up with these systems. However when you want to attract contributors from outside that context this friction clogs up your funnel.
The email-based git workflow (which GH copied fairly well) does things right here by having the "default" be for people to get notified of all project activity and sending reviews being as easy as an email reply.
This paves the way to anyone becoming a maintainer/reviewer simply by being one without needing any "permission".
# Gerrit as an issue
You mention that you see gerrit as a problem, but don't think the issue is gerrit. My guess is that you're simply not familiar enough with it to use the features it offers properly - This isn't meant to be a slight towards you, there is definitely a learning curve.
If you click on any of your patches, you should see a section called "Relationship Chain" generally in the upper middle right of the page. That's the patch train, in order. The first page that you see is simply ordered as the list of patches most recently touched in some fashion. It's not meant to show any sort of relationship.
Oh I've gotten plenty familiar with it ;)
The random order I'm talking about is on the "topic" search results page I've linked to before. I just find it problematic that a potential reviewer wouldn't even really easily know where to start with my series without jumping through hoops or me telling them :/
Say what you want about the gitlab/github model, but at least they display patch series in-order :]
Were I a committer I'd probably also be peeved by not being able to merge a whole series at once, but I assume there's a mechanism for that?
# We need ideas on *how* to improve
It's fine to say that the coreboot project should "do better", and you're not wrong, but without something concrete that we can actually do, that's not something we can just agree to do.
It just so happens I have opinions here, but frankly I'm the worst person to ask as I don't know the project or community intimately.
The coreboot project does not have any paid employees to do the work, and without getting significantly more contributions to support someone, I don't see that happening anytime soon. There are a number of companies supporting work for coreboot, but they're all supporting engineers responsible for their particular sections. Any work done in other areas for the community good, is generally done by volunteers. Asking unpaid volunteers to be strictly responsible for things is difficult. This is a problem that many open source projects have.
A common problem indeed, more so in projects where commercial entities participate directly through their employees rather than mainly through "independent" individual contributors in my observation.
cf. Debian or Linux. DDs/Maintainers have quite a bit of leverage against employers in the "my way or the highway" kind of sense.
# coreboot Leadership Meeting
I'd really like to ask you to attend the leadership meeting - we hold it every two weeks. You can find the time and how to join on the coreboot calendar: https://coreboot.org/calendar.html
I'm not sure that's the right venue for this discussion at this point. Perhaps more of a hallway track discussion for the next conference (37C3/FOSDEM anyone?).
--Daniel
Hi Daniel,
On Fri, Sep 29, 2023 at 6:53 AM Daniel Gröber dxld@darkboxed.org wrote:
Honestly this gerrit thing may work OK for companies where people are properly incentivized to do their assigned reviews or people in the in-group that know who to poke to get things moving, but for outside contributors the experience is even more fundamentally broken than the email/ML patch workflow and that's saying something.
I am curious how is gerrit review worflow is more broken than the emai/ML patch worflow, i.e. what do you think is better in the latter?
-vb
Hi Vadim,
On Tue, Oct 03, 2023 at 04:50:27PM -0700, Vadim Bendebury wrote:
On Fri, Sep 29, 2023 at 6:53 AM Daniel Gröber dxld@darkboxed.org wrote:
Honestly this gerrit thing may work OK for companies where people are properly incentivized to do their assigned reviews or people in the in-group that know who to poke to get things moving, but for outside contributors the experience is even more fundamentally broken than the email/ML patch workflow and that's saying something.
I am curious how is gerrit review worflow is more broken than the emai/ML patch worflow, i.e. what do you think is better in the latter?
I expanded on this in my reply to Martin:
The email-based git workflow (which GH copied fairly well) does things right here by having the "default" be for people to get notified of all project activity and sending reviews being as easy as an email reply.
This paves the way to anyone becoming a maintainer/reviewer simply by being one without needing any "permission".
Don't get me wrong, the email workflow without any additional tooling around it (patchwork or lists.sr.ht's patch support etc.) has it's own problems but in my mind community building is more important for FLOSS projects than a lot of other cosiderations when it comes to collaboration tools/infrastructure.
I find gerrit uniquely lacking here. The fact that it just isn't a tool that's commonly used as opposed to say GH/GL/G☕ doesn't help there but isn't the only problem.
Another is this: Something I do when I get to know a project is subscribe to the ML or equivalent and passively watch patches, reviews and banter to get a feel for how things are done here. I have no idea how you would do this with gerrit other than by actively adding yourself to CC on patches (which I feel is too visible to really be an equivalent) or actively deathscrolling through the patch lists, which I'm (personally) not going to remember to do periodically.
Perhaps I'm just exceptionally gerrit challanged?
--Daniel
Hi Daniel,
I admittedly am very loosely familiar with the email based reveiw flow, my experience if from many years back, and I don't have fond memories of it :)
On Thu, Oct 5, 2023 at 10:28 AM Daniel Gröber dxld@darkboxed.org wrote:
The email-based git workflow (which GH copied fairly well) does things right here by having the "default" be for people to get notified of all project activity and sending reviews being as easy as an email reply.
Isn't triggering an email as easy in gerrit? All you need to do is to add reviewer(s) in the 'Reply' popup and click 'send'. Or is your concern that the email is not sent to the wide audience by default?
Another is this: Something I do when I get to know a project is subscribe
to the ML or equivalent and passively watch patches, reviews and banter to get a feel for how things are done here. I have no idea how you would do this with gerrit other than by actively adding yourself to CC on patches (which I feel is too visible to really be an equivalent) or actively deathscrolling through the patch lists, which I'm (personally) not going to remember to do periodically.
Just in case this is not known: in gerrit you can subscribe to receive emails about all patches sent to a git repo: you click on the cog in the top right, then on 'Notifications' on the next screen, and then you can configure what emails on what repositories you want to receive. Would this address your concern?
Cheers, -vb
5. Oktober 2023 20:18, "Vadim Bendebury" vbendeb@chromium.org schrieb:
Just in case this is not known: in gerrit you can subscribe to receive emails about all patches sent to a git repo: you click on the cog in the top right, then on 'Notifications' on the next screen, and then you can configure what emails on what repositories you want to receive. Would this address your concern?
There's also the coreboot-gerrit mailing list (archive, although it doesn't thread emails properly, at https://mail.coreboot.org/hyperkitty/list/coreboot-gerrit@coreboot.org/lates...) that can be subscribed to.
We used to have these emails end up on the main mailing list (for familiarity after we moved from a mailing list + svn based flow to Gerrit + git in 2011), but people complained that there's too much noise on the list.
Patrick
Hi Vadim, Patrick,
On Thu, Oct 05, 2023 at 11:18:19AM -0700, Vadim Bendebury wrote:
I admittedly am very loosely familiar with the email based reveiw flow, my experience if from many years back, and I don't have fond memories of it :)
IMO sr.ht has innovated quite nicely here. Their ML manager (lists.sr.ht) detects patches and shows things like status APPLIED/PENDING/NACKed (based on git activity IIRC) and CI build status. Removing some of the legacy objections to the git email workflow.
(Side note: The APIs are all open and built to be modular so jenkins integration would likely not be hard)
Anyone can reply from the webinterface through their email client with a mailto:// link or The "Forward this thread to me" button, which hast to be the best ML invention since sliced bread eer I mean threads.
Notice: no account necessary. You just go ahead and do that thing you want to do.
You can prepare/send patchsets from the webinterface (with an account) to lower the entry barrier for not-yet git-email afficionados.
Until git(ea) PR federation through activitypub comes to fruition and is widely adopted I'm putting my decentralized eggs into the git-email basket.
That being said I'm not sure this particular tool is right for coreboot. I'm just saying in some aspects (barrier to contribute, community building) this is perhaps the gold standard now.
Just in case this is not known: in gerrit you can subscribe to receive emails about all patches sent to a git repo: you click on the cog in the top right, then on 'Notifications' on the next screen, and then you can configure what emails on what repositories you want to receive. Would this address your concern?
I didn't know that but IMO what gerrit sends out is absolutely not equivalent to a normal patch ML.
There's also the coreboot-gerrit mailing list (archive, although it doesn't thread emails properly, at https://mail.coreboot.org/hyperkitty/list/coreboot-gerrit@coreboot.org/lates...) that can be subscribed to.
We used to have these emails end up on the main mailing list (for familiarity after we moved from a mailing list + svn based flow to Gerrit
- git in 2011), but people complained that there's too much noise on the
list.
I mean, look at the emails gerrit sends they're almost pure spam, IMO that's another dimension of the problem with gerrit.
I'm overwhelemed enought by the gerrit emails I get from just my own patches. When humans send patches and humans send replies to those patches we don't get useless and unactionable mails like "attention is required by" and "comments were posted" without the actual comments. I might want to read those comments, Gerrit, grrrr!
I think this problem is multiplied because in gerrit everything is per-patch rather than per-series. I've never had this much of a problem with large series in gitlab even though it can be chatty too.
Something I've seen, but that may not work for coreboot is to keep gerrit for project members/teams working closeley together, but allow [PATCH] submission on the ML for outsiders.
idk. just a baseline for discussion.
--Daniel
-
Christian Walter -
Daniel Gröber -
Jakub Czapiga -
Martin Roth -
Patrick Georgi -
Vadim Bendebury