Dear fellow flashrom developers,
I've been reflecting lately about written and unwritten rules around our review process and our related privileges on Gerrit. I couldn't identify anything that works well, or let's say well enough. So it might be time we discuss some things and make changes :)
Before the dawn of our Git repository and using Gerrit as a review plat- form, we had a Subversion repository. With very few people, usually one or two I think, that had write access. So practically we had gatekeepers that watched over the code. Eventually, they became a bottleneck so we widened that a little when starting the work on Gerrit. Today, I often catch myself falling back into such a gatekeeper mode. Which is futile anyway because I'm not the only one who can write to the repository. But I still wonder why this happens and if it's necessary.
I guess first everyone needs to understand how we imagined things with Gerrit and how it looks like today. As mentioned above we set up a group "flashrom developers" which has exclusive write access to the reposi- tory. The idea was to only have people in this group who are long-term (I was thinking about years) contributors to flashrom, know each other from real-time communication (IRC) and trust each other. For the review right "+2", we used the "Reviewers" group from coreboot. IIRC, I pushed for the latter. I was hoping to grow the communities a little together that seemed to have gone separate ways in the years before. After all, a lot of coreboot developers use flashrom in their everyday work. Combined with the "flashrom developers" group it shouldn't have been an issue that people with little flashrom knowledge would review.
Since the original setup (about 2017) things went on rather uncon- trolled. I think the gist of it is that people didn't expect that flashrom would need/have different rules compared to coreboot. At some point somebody whom I only knew from their work on coreboot sud- denly merged patches for flashrom. And I guess people pushing patches today pretty much expect that there is nothing more to be done once they see the first +2. No matter if the reviewer knows anything about flashrom.
When reviewers disagree then, it can become frustrating for everyone. So currently, I lean towards aligning our process more with that of coreboot. The big difference seems to be that at coreboot merging patches, i.e. hitting the "Submit" button, is more or less considered an administrative task unrelated to one's opinion on the code. While for us it was supposed to replace the gatekeeping. Changing this would imply that we need our own group of reviewers. With this, a +2 would become both rarer and stronger. If this idea meets some consent, I would propose to copy the "flashrom developers" group for a start. And when we identify somebody over time who shows in-depth knowledge about at least a part of flashrom and good self-judgement if they are experienced enough for a +2, we could add them. Right now, some candidates come to mind already, Peter, Nikolai, and Thomas.
The "flashrom developers" group also grants the privilege to block "-2" a change. I have very conflicting feelings about this. On one hand, this is a tool that is only ever necessary when people in said group can't trust each other. On the other, that seems to be the status quo already. Maybe we should have another group on top for this? This time really only for trusted, established, long-term contributors? Or maybe for those who maintain the codebase beyond what they are paid for?
There is one related idea for a rule that crossed my mind: How about we forbid blocking reverts that fix a regression and apply cleanly (i.e. without needing to revert anything else in advance). Also, let them pass after review without waiting 24 hours. At least for myself this would reduce a lot pressure to fall back to gatekeeping if I knew I could keep things safe and sound without discussion. For maintainers, the patches that are already merged matter the most, cause the most work. This should also be the case for contributors, but alas many don't see it like this and don't help to fix things after a patch is merged. Easing reverts would push the hard work back to the people who break things.
Looks a bit longer than what I intended to write, as usual when one starts writing ;) If we come to a conclusion wrt. Gerrit privileges, we could also discuss more rules for each role. While I'm not a fan of too many rules and would rather keep things to common sense, these things depend much on the people we add to the respective groups.
I hope it's not too much to digest at once ;) Cheers, Nico
Nico, and everyone,
Thanks for many details, and for historical information, always interesting for me to read :)
*“The big difference seems to be that at coreboot mergingpatches, i.e. hitting the "Submit" button, is more or less consideredan administrative task unrelated to one's opinion on the code. Whilefor us it was supposed to replace the gatekeeping. Changing this wouldimply that we need our own group of reviewers. With this, a +2 wouldbecome both rarer and stronger.”*
I didn’t even know that the Reviewers group is inherited from coreboot, I only learned this very recently! :) Probably because I didn’t work on coreboot, so I don’t know people from there. And when I am adding reviewers that I know, they all work on flashrom.
In general, Reviewers and Committers are two groups to go through to merge a patch, and it makes sense that at least one of them knows very well what they are doing. If one of them is doing an “administrative task”, then the other one should do a real task. To me, it makes sense that the real task is for Reviewers. Because their names are immortalised by the “Reviewed-by” tag, so you can always find from commit history who did it.
It makes sense to me to have a Flashrom Reviewers group (and as I said, I thought this is the case already). Although it needs to be very clearly discussed and agreed, what is the responsibility of being a Reviewer. And now my mind immediately starts to generate ideas about what can go wrong. Please don’t see this as a criticism, it is just how an engineering mind works :)
# New group is smaller than the previous group, so some people are losing their +2 on flashrom. People get very upset and offended. # Shortly after, lots of people are added to the new Flashrom Reviewers group, and we get back to square one. # Committing patches is now considered a truly “administrative task” and so lots of people are added to that group because why not # And all of the above can happen together :)
I still think the idea of the Flashrom Reviewers group makes sense, but it needs to be well discussed and agreed upon. Good thing we have a meeting now :)
*“The "flashrom developers" group also grants the privilege to block"-2" a change. I have very conflicting feelings about this. On onehand, this is a tool that is only ever necessary when people in saidgroup can't trust each other. On the other, that seems to be thestatus quo already. Maybe we should have another group on top forthis?”*
With this, I disagree, and the reason is that the real problem is “people don’t trust each other”. We need to fix this real problem! “We” as a team. Merging patches and blocking patches from merging seems to me to be equivalent powers, just with + and - signs. One missing piece of information for me (I simply don’t know): is it possible to remove -2 which is made by someone else? Like one person blocks the patch, another person unblocks? Is it possible?
*“There is one related idea for a rule that crossed my mind: How aboutwe forbid blocking reverts that fix a regression and apply cleanly(i.e. without needing to revert anything else in advance). Also, letthem pass after review without waiting 24 hours.”*Makes sense. I thought “revert first, fix later” is somewhat a general rule?
Especially when you are saying:
*“At least for myselfthis would reduce a lot pressure to fall back to gatekeeping ”* When I hear “gatekeeping” to me it is equivalent to “enormous responsibility”. Such things burn people :( I don’t know how far you are on the “burning” scale, but I don’t want you to get further…
I have some more thoughts about it. I think people can be opposed to reverts because reviews are very long, and so people are waiting for so long for patches to get merged… and then revert?? NO!! These are not my thoughts, I never opposed the revert :) this is what I imagine. So… reviews take long, and people are opposed to reverts. But then, reviews are long because people are opposed to reverts! Vicious circle yes. Which we need to break. “We” as a team. Good thing we have a meeting now :) Oh wait… I am saying this for the second time? :)
On Sun, Mar 6, 2022 at 3:17 AM Nico Huber nico.h@gmx.de wrote:
Dear fellow flashrom developers,
I've been reflecting lately about written and unwritten rules around our review process and our related privileges on Gerrit. I couldn't identify anything that works well, or let's say well enough. So it might be time we discuss some things and make changes :)
Before the dawn of our Git repository and using Gerrit as a review plat- form, we had a Subversion repository. With very few people, usually one or two I think, that had write access. So practically we had gatekeepers that watched over the code. Eventually, they became a bottleneck so we widened that a little when starting the work on Gerrit. Today, I often catch myself falling back into such a gatekeeper mode. Which is futile anyway because I'm not the only one who can write to the repository. But I still wonder why this happens and if it's necessary.
I guess first everyone needs to understand how we imagined things with Gerrit and how it looks like today. As mentioned above we set up a group "flashrom developers" which has exclusive write access to the reposi- tory. The idea was to only have people in this group who are long-term (I was thinking about years) contributors to flashrom, know each other from real-time communication (IRC) and trust each other. For the review right "+2", we used the "Reviewers" group from coreboot. IIRC, I pushed for the latter. I was hoping to grow the communities a little together that seemed to have gone separate ways in the years before. After all, a lot of coreboot developers use flashrom in their everyday work. Combined with the "flashrom developers" group it shouldn't have been an issue that people with little flashrom knowledge would review.
Since the original setup (about 2017) things went on rather uncon- trolled. I think the gist of it is that people didn't expect that flashrom would need/have different rules compared to coreboot. At some point somebody whom I only knew from their work on coreboot sud- denly merged patches for flashrom. And I guess people pushing patches today pretty much expect that there is nothing more to be done once they see the first +2. No matter if the reviewer knows anything about flashrom.
When reviewers disagree then, it can become frustrating for everyone. So currently, I lean towards aligning our process more with that of coreboot. The big difference seems to be that at coreboot merging patches, i.e. hitting the "Submit" button, is more or less considered an administrative task unrelated to one's opinion on the code. While for us it was supposed to replace the gatekeeping. Changing this would imply that we need our own group of reviewers. With this, a +2 would become both rarer and stronger. If this idea meets some consent, I would propose to copy the "flashrom developers" group for a start. And when we identify somebody over time who shows in-depth knowledge about at least a part of flashrom and good self-judgement if they are experienced enough for a +2, we could add them. Right now, some candidates come to mind already, Peter, Nikolai, and Thomas.
The "flashrom developers" group also grants the privilege to block "-2" a change. I have very conflicting feelings about this. On one hand, this is a tool that is only ever necessary when people in said group can't trust each other. On the other, that seems to be the status quo already. Maybe we should have another group on top for this? This time really only for trusted, established, long-term contributors? Or maybe for those who maintain the codebase beyond what they are paid for?
There is one related idea for a rule that crossed my mind: How about we forbid blocking reverts that fix a regression and apply cleanly (i.e. without needing to revert anything else in advance). Also, let them pass after review without waiting 24 hours. At least for myself this would reduce a lot pressure to fall back to gatekeeping if I knew I could keep things safe and sound without discussion. For maintainers, the patches that are already merged matter the most, cause the most work. This should also be the case for contributors, but alas many don't see it like this and don't help to fix things after a patch is merged. Easing reverts would push the hard work back to the people who break things.
Looks a bit longer than what I intended to write, as usual when one starts writing ;) If we come to a conclusion wrt. Gerrit privileges, we could also discuss more rules for each role. While I'm not a fan of too many rules and would rather keep things to common sense, these things depend much on the people we add to the respective groups.
I hope it's not too much to digest at once ;) Cheers, Nico _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
On Sat, Mar 12, 2022 at 12:57 AM Anastasia Klimchuk aklm@chromium.org wrote: ... On Sat, Mar 5, 2022 at 9:17 AM Nico Huber nico.h@gmx.de wrote: ...
“The big difference seems to be that at coreboot merging patches, i.e. hitting the "Submit" button, is more or less considered an administrative task unrelated to one's opinion on the code. While for us it was supposed to replace the gatekeeping. Changing this would imply that we need our own group of reviewers. With this, a +2 would become both rarer and stronger.”
I didn’t even know that the Reviewers group is inherited from coreboot, I only learned this very recently! :) Probably because I didn’t work on coreboot, so I don’t know people from there. And when I am adding reviewers that I know, they all work on flashrom.
I never actually considered merges just administrative and unrelated to the reviews in coreboot. I definitely review every patch before I merge it, though I'm definitely not as picky or thoughtful as some reviewers are. I'm pretty sure it says in the gerrit guidelines that if someone merges a bad patch, they're responsible for helping to fix it.
In general, Reviewers and Committers are two groups to go through to merge a patch, and it makes sense that at least one of them knows very well what they are doing. If one of them is doing an “administrative task”, then the other one should do a real task. To me, it makes sense that the real task is for Reviewers. Because their names are immortalised by the “Reviewed-by” tag, so you can always find from commit history who did it.
Just note that the "Reviewed-by" tag is only for people who gave a +1 or +2 in themerged revision of the patch on Gerrit. Any earlier reviewers are lost.The submitter is of course included in the git history as well.
It makes sense to me to have a Flashrom Reviewers group (and as I said, I thought this is the case already). Although it needs to be very clearly discussed and agreed, what is the responsibility of being a Reviewer. And now my mind immediately starts to generate ideas about what can go wrong. Please don’t see this as a criticism, it is just how an engineering mind works :)
I Absolutely agree. The flashrom project should decide how to decide who gets what rights, and a new set of groups can be created. Even though all of the projects managed at coreboot.org have a similar set of rules, that's not by necessity. For example, the flashrom project doesn't require that all comments be resolved before merge. That can be enabled if you'd like, but currently it isn't.
New group is smaller than the previous group, so some people are losing their +2 on flashrom. People get very upset and offended. Shortly after, lots of people are added to the new Flashrom Reviewers group, and we get back to square one. Committing patches is now considered a truly “administrative task” and so lots of people are added to that group because why not. And all of the above can happen together :)
I still think the idea of the Flashrom Reviewers group makes sense, but it needs to be well discussed and agreed upon. Good thing we have a meeting now :)
I think that the change just needs to be announced, along with the requirements for flashrom +2. I don't see why people would expect to be able to +2 flashrom patches if they haven't submitted anything in a long time. I just removed a few people's +2 rights from coreboot - anyone who hadn't submitted a patch in the past 4 years or so. I'll also occasionally go through and remove submit rights, though I typically talk to the person before doing that.
“The "flashrom developers" group also grants the privilege to block "-2" a change. I have very conflicting feelings about this. On one hand, this is a tool that is only ever necessary when people in said group can't trust each other. On the other, that seems to be the status quo already. Maybe we should have another group on top for this?”
With this, I disagree, and the reason is that the real problem is “people don’t trust each other”. We need to fix this real problem! “We” as a team. Merging patches and blocking patches from merging seems to me to be equivalent powers, just with + and - signs. One missing piece of information for me (I simply don’t know): is it possible to remove -2 which is made by someone else? Like one person blocks the patch, another person unblocks? Is it possible?
It is possible to remove a -2, but it happens very rarely on the coreboot project, and I think that currently only 3 people have that right. If flashrom would like to set different rules, that can be set up.
Depending on how fast you want to be able to get patches merged at flashrom, it might help to have a weekly or bi-weekly meeting *just* to discuss patches face-to-face. It seems like there's some miscommunication, and as we saw at the flashrom meeting last week, people can discuss things better vocally. Even just an impromptu meeting between two reviewers to hash out the points would be better than what's happening now, where people seem to be talking past each other.
“There is one related idea for a rule that crossed my mind: How about we forbid blocking reverts that fix a regression and apply cleanly (i.e. without needing to revert anything else in advance). Also, let them pass after review without waiting 24 hours.”
Makes sense. I thought “revert first, fix later” is somewhat a general rule?
Hopefully, if the review process gets better, this wouldn't need to happen very often. I do think that everyone is really trying to do what they think is best for the project, and that miscommunication may be responsible for a lot of the issues. I definitely don't think that any of the significant contributors are looking to cause friction within the project or codebase.
I hope it's not too much to digest at once ;)
This is all I can digest for now, so I'm ending my response here. To anyone following on, please go back to the previous emails to read the rest of the discussion.
Cheers, Nico
-- Anastasia.
Take care, Martin
On Sat, Mar 12, 2022 at 12:57 AM Anastasia Klimchuk aklm@chromium.org wrote:
Nico, and everyone,
Thanks for many details, and for historical information, always interesting for me to read :)
“The big difference seems to be that at coreboot merging patches, i.e. hitting the "Submit" button, is more or less considered an administrative task unrelated to one's opinion on the code. While for us it was supposed to replace the gatekeeping. Changing this would imply that we need our own group of reviewers. With this, a +2 would become both rarer and stronger.”
I didn’t even know that the Reviewers group is inherited from coreboot, I only learned this very recently! :) Probably because I didn’t work on coreboot, so I don’t know people from there. And when I am adding reviewers that I know, they all work on flashrom.
In general, Reviewers and Committers are two groups to go through to merge a patch, and it makes sense that at least one of them knows very well what they are doing. If one of them is doing an “administrative task”, then the other one should do a real task. To me, it makes sense that the real task is for Reviewers. Because their names are immortalised by the “Reviewed-by” tag, so you can always find from commit history who did it.
It makes sense to me to have a Flashrom Reviewers group (and as I said, I thought this is the case already). Although it needs to be very clearly discussed and agreed, what is the responsibility of being a Reviewer. And now my mind immediately starts to generate ideas about what can go wrong. Please don’t see this as a criticism, it is just how an engineering mind works :)
# New group is smaller than the previous group, so some people are losing their +2 on flashrom. People get very upset and offended. # Shortly after, lots of people are added to the new Flashrom Reviewers group, and we get back to square one. # Committing patches is now considered a truly “administrative task” and so lots of people are added to that group because why not # And all of the above can happen together :)
I still think the idea of the Flashrom Reviewers group makes sense, but it needs to be well discussed and agreed upon. Good thing we have a meeting now :)
“The "flashrom developers" group also grants the privilege to block "-2" a change. I have very conflicting feelings about this. On one hand, this is a tool that is only ever necessary when people in said group can't trust each other. On the other, that seems to be the status quo already. Maybe we should have another group on top for this?”
With this, I disagree, and the reason is that the real problem is “people don’t trust each other”. We need to fix this real problem! “We” as a team. Merging patches and blocking patches from merging seems to me to be equivalent powers, just with + and - signs. One missing piece of information for me (I simply don’t know): is it possible to remove -2 which is made by someone else? Like one person blocks the patch, another person unblocks? Is it possible?
“There is one related idea for a rule that crossed my mind: How about we forbid blocking reverts that fix a regression and apply cleanly (i.e. without needing to revert anything else in advance). Also, let them pass after review without waiting 24 hours.”
Makes sense. I thought “revert first, fix later” is somewhat a general rule?
Especially when you are saying:
“At least for myself this would reduce a lot pressure to fall back to gatekeeping ”
When I hear “gatekeeping” to me it is equivalent to “enormous responsibility”. Such things burn people :( I don’t know how far you are on the “burning” scale, but I don’t want you to get further…
I have some more thoughts about it. I think people can be opposed to reverts because reviews are very long, and so people are waiting for so long for patches to get merged… and then revert?? NO!! These are not my thoughts, I never opposed the revert :) this is what I imagine. So… reviews take long, and people are opposed to reverts. But then, reviews are long because people are opposed to reverts! Vicious circle yes. Which we need to break. “We” as a team. Good thing we have a meeting now :) Oh wait… I am saying this for the second time? :)
On Sun, Mar 6, 2022 at 3:17 AM Nico Huber nico.h@gmx.de wrote:
Dear fellow flashrom developers,
I've been reflecting lately about written and unwritten rules around our review process and our related privileges on Gerrit. I couldn't identify anything that works well, or let's say well enough. So it might be time we discuss some things and make changes :)
Before the dawn of our Git repository and using Gerrit as a review plat- form, we had a Subversion repository. With very few people, usually one or two I think, that had write access. So practically we had gatekeepers that watched over the code. Eventually, they became a bottleneck so we widened that a little when starting the work on Gerrit. Today, I often catch myself falling back into such a gatekeeper mode. Which is futile anyway because I'm not the only one who can write to the repository. But I still wonder why this happens and if it's necessary.
I guess first everyone needs to understand how we imagined things with Gerrit and how it looks like today. As mentioned above we set up a group "flashrom developers" which has exclusive write access to the reposi- tory. The idea was to only have people in this group who are long-term (I was thinking about years) contributors to flashrom, know each other from real-time communication (IRC) and trust each other. For the review right "+2", we used the "Reviewers" group from coreboot. IIRC, I pushed for the latter. I was hoping to grow the communities a little together that seemed to have gone separate ways in the years before. After all, a lot of coreboot developers use flashrom in their everyday work. Combined with the "flashrom developers" group it shouldn't have been an issue that people with little flashrom knowledge would review.
Since the original setup (about 2017) things went on rather uncon- trolled. I think the gist of it is that people didn't expect that flashrom would need/have different rules compared to coreboot. At some point somebody whom I only knew from their work on coreboot sud- denly merged patches for flashrom. And I guess people pushing patches today pretty much expect that there is nothing more to be done once they see the first +2. No matter if the reviewer knows anything about flashrom.
When reviewers disagree then, it can become frustrating for everyone. So currently, I lean towards aligning our process more with that of coreboot. The big difference seems to be that at coreboot merging patches, i.e. hitting the "Submit" button, is more or less considered an administrative task unrelated to one's opinion on the code. While for us it was supposed to replace the gatekeeping. Changing this would imply that we need our own group of reviewers. With this, a +2 would become both rarer and stronger. If this idea meets some consent, I would propose to copy the "flashrom developers" group for a start. And when we identify somebody over time who shows in-depth knowledge about at least a part of flashrom and good self-judgement if they are experienced enough for a +2, we could add them. Right now, some candidates come to mind already, Peter, Nikolai, and Thomas.
The "flashrom developers" group also grants the privilege to block "-2" a change. I have very conflicting feelings about this. On one hand, this is a tool that is only ever necessary when people in said group can't trust each other. On the other, that seems to be the status quo already. Maybe we should have another group on top for this? This time really only for trusted, established, long-term contributors? Or maybe for those who maintain the codebase beyond what they are paid for?
There is one related idea for a rule that crossed my mind: How about we forbid blocking reverts that fix a regression and apply cleanly (i.e. without needing to revert anything else in advance). Also, let them pass after review without waiting 24 hours. At least for myself this would reduce a lot pressure to fall back to gatekeeping if I knew I could keep things safe and sound without discussion. For maintainers, the patches that are already merged matter the most, cause the most work. This should also be the case for contributors, but alas many don't see it like this and don't help to fix things after a patch is merged. Easing reverts would push the hard work back to the people who break things.
Looks a bit longer than what I intended to write, as usual when one starts writing ;) If we come to a conclusion wrt. Gerrit privileges, we could also discuss more rules for each role. While I'm not a fan of too many rules and would rather keep things to common sense, these things depend much on the people we add to the respective groups.
I hope it's not too much to digest at once ;) Cheers, Nico _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
-- Anastasia. _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
Hi Anastasia, Martin,
many thanks for your thoughts on the matter. \o/
On 12.03.22 22:29, Martin L Roth wrote:
On Sat, Mar 12, 2022 at 12:57 AM Anastasia Klimchuk aklm@chromium.org wrote: ... On Sat, Mar 5, 2022 at 9:17 AM Nico Huber nico.h@gmx.de wrote: ...
“The big difference seems to be that at coreboot merging patches, i.e. hitting the "Submit" button, is more or less considered an administrative task unrelated to one's opinion on the code. While for us it was supposed to replace the gatekeeping. Changing this would imply that we need our own group of reviewers. With this, a +2 would become both rarer and stronger.”
I didn’t even know that the Reviewers group is inherited from coreboot, I only learned this very recently! :) Probably because I didn’t work on coreboot, so I don’t know people from there. And when I am adding reviewers that I know, they all work on flashrom.
I never actually considered merges just administrative and unrelated to the reviews in coreboot. I definitely review every patch before I merge it, though I'm definitely not as picky or thoughtful as some reviewers are. I'm pretty sure it says in the gerrit guidelines that if someone merges a bad patch, they're responsible for helping to fix it.
The coreboot guidelines indeed state one is expected to help. We never made this explicit for flashrom. There is definitely room for improve- ment in our guidelines when we agree on something :)
Comparing the two roles there is another important point: One can't review their own commits but a core developer can submit their own commits. So I'd put it like this: The reviewer's task is to make sure the code is sound (that should include the whole result, existing code + patch). It's something everyone can use a second pair of eyes for. The submitter should make sure things generally move into the right direction (which is much less objective than the review) and take responsibility for a commit.
In general, Reviewers and Committers are two groups to go through to merge a patch, and it makes sense that at least one of them knows very well what they are doing. If one of them is doing an “administrative task”, then the other one should do a real task. To me, it makes sense that the real task is for Reviewers. Because their names are immortalised by the “Reviewed-by” tag, so you can always find from commit history who did it.
Just note that the "Reviewed-by" tag is only for people who gave a +1 or +2 in themerged revision of the patch on Gerrit. Any earlier reviewers are lost.The submitter is of course included in the git history as well.
Some background on the mechanics: Git is prepared to distinguish the _author_ and the _committer_ of a commit. The committer is not shown by default in the log. One can make it visible with `git log --pretty=fuller`. Gerrit also shows this information in the diff view of commit messages.
The author information is set when a commit is created and usually never altered again. If a commit is amended or cherry-picked, however, the committer information is updated. Gerrit also does so when somebody clicks "Submit".
Regarding visibility there's a wrinkle: Technically, the committer information is tighter integrated into the commit, compared to the Reviewed-by tag. Only the latter is shown in the log by default, though.
It is possible to remove a -2, but it happens very rarely on the coreboot project, and I think that currently only 3 people have that right. If flashrom would like to set different rules, that can be set up.
There are currently four people set up for flashrom as "owners" (Angel, David, Stefan T. and myself). It's kind of admin access but without being admin for the whole Gerrit instance. I never tried, but I assume we could remove a -2. In any case this group can change the rules, so it's only a question of how convenient it would be to override a -2.
“There is one related idea for a rule that crossed my mind: How about we forbid blocking reverts that fix a regression and apply cleanly (i.e. without needing to revert anything else in advance). Also, let them pass after review without waiting 24 hours.”
Makes sense. I thought “revert first, fix later” is somewhat a general rule?
I would have expected that too. But unfortunately not everybody agrees. And there is also a huge psychological component. When somebody knows they have the time to fix things after a revert, that seems fine. But if one doesn't have the time, it can really hurt to see something reverted.
Hopefully, if the review process gets better, this wouldn't need to happen very often. I do think that everyone is really trying to do what they think is best for the project, and that miscommunication may be responsible for a lot of the issues. I definitely don't think that any of the significant contributors are looking to cause friction within the project or codebase.
With the words above about the reviewer and submitter roles in mind, I'd say the reverts of the past three years are more of a review issue. I mean we only revert when the code is broken. And so far the things that I see to be moving into a wrong direction seem to be cases where this is nobody's intention. It's mostly that the code was written for another project/fork and nobody had the time yet to adapt it for upstream. At least I hope so :)
Nico
On 12.03.22 08:57, Anastasia Klimchuk wrote:
The big difference seems to be that at coreboot mergingpatches, i.e. hitting the "Submit" button, is more or less consideredan administrative task unrelated to one's opinion on the code. Whilefor us it was supposed to replace the gatekeeping. Changing this wouldimply that we need our own group of reviewers. With this, a +2 wouldbecome both rarer and stronger.
I didn’t even know that the Reviewers group is inherited from coreboot, I only learned this very recently! :) Probably because I didn’t work on coreboot, so I don’t know people from there. And when I am adding reviewers that I know, they all work on flashrom.
In general, Reviewers and Committers are two groups to go through to merge a patch, and it makes sense that at least one of them knows very well what they are doing. If one of them is doing an “administrative task”, then the other one should do a real task. To me, it makes sense that the real task is for Reviewers. Because their names are immortalised by the “Reviewed-by” tag, so you can always find from commit history who did it.
It makes sense to me to have a Flashrom Reviewers group (and as I said, I thought this is the case already). Although it needs to be very clearly discussed and agreed, what is the responsibility of being a Reviewer. And now my mind immediately starts to generate ideas about what can go wrong. Please don’t see this as a criticism, it is just how an engineering mind works :)
# New group is smaller than the previous group, so some people are losing their +2 on flashrom. People get very upset and offended.
That's also what I am afraid of. Looking through the recent Git history, I've seen many Reviewed-by tags of people that most likely wouldn't fall into the group if we formalize it now. It might help if we make plans in advance how to bring them into the group if they want to.
# Shortly after, lots of people are added to the new Flashrom Reviewers group, and we get back to square one. # Committing patches is now considered a truly “administrative task” and so lots of people are added to that group because why not
That's probably why I'm so afraid of rules ;) start with one and you'll have to add a dozen more. We'd have to properly formalize everything around the groups to avoid regressions.
There is one related idea for a rule that crossed my mind: How aboutwe forbid blocking reverts that fix a regression and apply cleanly(i.e. without needing to revert anything else in advance). Also, letthem pass after review without waiting 24 hours.
Makes sense. I thought “revert first, fix later” is somewhat a general rule?
Especially when you are saying:
At least for myselfthis would reduce a lot pressure to fall back to gatekeeping
When I hear “gatekeeping” to me it is equivalent to “enormous responsibility”. Such things burn people :( I don’t know how far you are on the “burning” scale, but I don’t want you to get further…
Technically, I'm beyond the scale already. But I don't want to burden you with that story. There is not much to worry about right now. Today, the most unsettling thing seems to me is that we spend a lot of time to address problems that people may have accidentally imported into the project.
I have some more thoughts about it. I think people can be opposed to reverts because reviews are very long, and so people are waiting for so long for patches to get merged… and then revert?? NO!! These are not my thoughts, I never opposed the revert :) this is what I imagine. So… reviews take long, and people are opposed to reverts. But then, reviews are long because people are opposed to reverts!
Well, I'm not sure but something inside of me wants to object. It's hard to put into words, the things I've reverted in the past are not really of a specific kind but still seem to share something. Roughly, when a patch got a decent review there should only be typos left that could cause trouble; it should be easier to fix than to revert.
I've noticed something related in reviews over the years, though. Some- times when reviewers give a lot of comments on Gerrit, among them some critical ones about the overall patch and a lot of nits, the author tends to fix the nits and ignore the critical comments. Sure, when somebody ignores comments and then things get reverted, it's techni- cally their fault. But I can well imagine that such incidents would affect future reviews, also for the reviewers. Somehow, everybody learned that they wasted their time :-/
Reading what I just wrote, it sounds like we'd need more review guidelines ._.
Nico
Vicious circle yes. Which we need to break. “We” as a team. Good thing we have a meeting now :) Oh wait… I am saying this for the second time? :)
I've noticed something related in reviews over the years, though. Some- times when reviewers give a lot of comments on Gerrit, among them some critical ones about the overall patch and a lot of nits, the author tends to fix the nits and ignore the critical comments. Sure, when somebody ignores comments and then things get reverted, it's techni- cally their fault. But I can well imagine that such incidents would affect future reviews, also for the reviewers. Somehow, everybody learned that they wasted their time :-/
In formal software engineering culture, there is a notion of findings being major vs minor, where major is failure to meet specification and minor is something else, usually style.
I think the basic issue is that flashrom, quite reasonably, is trying to achieve a far higher level of software quality than is typical in open source.
It might be reasonable to expect that after a review which had a major finding, to expect it to be re-reviewed before merging.
“For example, the flashrom project doesn't require that all comments be resolved before merge. That can be enabled if you'd like, but currently it isn't.”
Oh this explains! I was wondering where those “patches merged with unresolved comments” are coming from. I am 100% voting for this setting to be enabled. It does not prevent from just clicking Ack on all comments, if needed. But at least, comments won’t be lost unintentionally.
And then, we can for example agree (if we agree) that if the Reviewer wants a response on the comment, they mark it Unresolved (it will be yellow colour). It doesn’t solve all the problems, but at least, it guards unintentional mistakes.
“it might help to have a weekly or bi-weekly meeting *just* to discuss patches face-to-face.”
I would agree with that. This is *another meeting* yes! But looking at how things are going right now, a problematic patch takes so much unnecessary time, it can be weeks or even months, as I said, unnecessary extra time. And it starts involving lots of people, many of those people should not be involved at all. And worst of all, it can cause emotional damage to people involved. This is all wrong. We need to ask others for sure, but for myself, I am happy to have another meeting, because it has a chance to remove those problems. A patch should be a purely technical question.
“Comparing the two roles there is another important point: One can't review their own commits but a core developer can submit their own commits.”
This is an important point. Reviewer is a very important second pair of eyes. I see my previous point about “Reviewed-by” tag is not that strong :) But your one is.
“It's kind of admin access but without being admin for the whole Gerrit instance. I never tried, but I assume we could remove a -2. In any case this group can change the rules, so it's only a question of how convenient it would be to override a -2.”
This is good, so there is a way to remove a -2. I am still in the same position that “merging” and “block from merging” are similar powers with + and - sign, and it makes sense to have it in one group. If this group (very small!) has no trust in each other, let’s solve it.
“And there is also a huge psychological component. When somebody knows they have the time to fix things after a revert, that seems fine. But if one doesn't have the time, it can really hurt to see something reverted.”
But wait, one needs to fix things if a patch breaks something? It is technically a choice between “revert and fix” or “fix”? In both cases, there is a “fix” involved…
“That's probably why I'm so afraid of rules ;) start with one and you'll have to add a dozen more. We'd have to properly formalize everything around the groups to avoid regressions.”
I looked once again in my “what can go wrong” scenarios, and now I think that #2 and #3 can be solved by having clear rules on when someone can be added to Reviewers and Committers (and those clear rules are published so everyone can see). Let’s brainstorm some clear rules? :) The point #1 can be solved by a nice message which explains everything and calms down people ;) So actually, maybe it’s not so bad :)
“Technically, I'm beyond the scale already. But I don't want to burden you with that story. There is not much to worry about right now.”
If I can help by listening, just tell me!
“Today, the most unsettling thing seems to me is that we spend a lot of time to address problems that people may have accidentally imported into the project.”
I think this is a connection to a second topic “Release preparations” that I will respond to next :) But what I think, the problems need to be listed, described and assigned. Lots of these you did already!
“I've noticed something related in reviews over the years, though. Some- times when reviewers give a lot of comments on Gerrit, among them some critical ones about the overall patch and a lot of nits, the author tends to fix the nits and ignore the critical comments.”
We can formalise this. Gerrit has yellow and grey comments. Yellow are supposed to be blocking (major) and grey are non-blocking (minor). We can, at the very least, state very clearly that yellow comments cannot be ignored (and Martin said it can be enforced in Gerrit!). It is still possible to give some random answer, but… better than nothing.
And that’s what Greg said as well, findings can be major and minor. Although in my head I see it as “comments that I expect a reply” and “just comments”. This is why I almost always have yellow comments: I expect a reply :P Unless it is something like “Awesome work thanks!” etc.
And in addition to everything that I said, I have another thought. I hear the words “right direction” and “wrong direction” from time to time (in gerrit comments, and in this thread). And the thing with those words… They are deceptive, because everyone has their own idea in their own head on what is right / wrong direction, and it seems so obvious to the person, and no one thinks of explaining what exactly “right direction” means. But it means different things to different people.
On Mon, Mar 14, 2022 at 1:59 AM Greg Troxel gdt@lexort.com wrote:
I've noticed something related in reviews over the years, though. Some- times when reviewers give a lot of comments on Gerrit, among them some critical ones about the overall patch and a lot of nits, the author tends to fix the nits and ignore the critical comments. Sure, when somebody ignores comments and then things get reverted, it's techni- cally their fault. But I can well imagine that such incidents would affect future reviews, also for the reviewers. Somehow, everybody learned that they wasted their time :-/
In formal software engineering culture, there is a notion of findings being major vs minor, where major is failure to meet specification and minor is something else, usually style.
I think the basic issue is that flashrom, quite reasonably, is trying to achieve a far higher level of software quality than is typical in open source.
It might be reasonable to expect that after a review which had a major finding, to expect it to be re-reviewed before merging.
Hi Anastasia,
On 17.03.22 12:32, Anastasia Klimchuk wrote:
For example, the flashrom project doesn't require that all comments be resolved before merge. That can be enabled if you'd like, but currently it isn't.
Oh this explains! I was wondering where those “patches merged with unresolved comments” are coming from. I am 100% voting for this setting to be enabled. It does not prevent from just clicking Ack on all comments, if needed. But at least, comments won’t be lost unintentionally.
and I vote 100% against it. FWIW, this feature wakes our worst in- stincts. No matter how convinced we are that review is also for our own good, getting a commit merged always feels rewarding. And many people act like it's a necessity to set the resolved tick to gain that reward. I've seen people doing so with every single comment they wrote since day 1 when this feature was activated for coreboot. Let's not repeat every mistake made there.
Also, today Gerrit warns about unresolved comment threads when one tries to submit. Enforcing it brings a marginal convenience but makes the review process less friendly in my experience.
Sometimes comment threads are opened after review. So the `merged` status never implies all-comments-resolved. And that's not bad, IMHO. Sometimes things come up after review and Gerrit makes it easy to continue to discuss changes even after they are merged.
And then, we can for example agree (if we agree) that if the Reviewer wants a response on the comment, they mark it Unresolved (it will be yellow colour). It doesn’t solve all the problems, but at least, it guards unintentional mistakes.
it might help to have a weekly or bi-weekly meeting *just* to discuss patches face-to-face.
I would agree with that. This is *another meeting* yes! But looking at how things are going right now, a problematic patch takes so much unnecessary time, it can be weeks or even months, as I said, unnecessary extra time.
IMO, problematic patches should not be escalated but avoided in the first place. In our wiki it says in multiple places that one should discuss things on the mailing list early so nobody gets frustrated later. Such problematic reviews is exactly why it's stated there. Open-source development is nothing new. Many problems have been fixed in the past already. Please always encourage everybody to try the established ways before experimenting with new ones.
There also seems to be a misunderstanding about patches. Some people seem to believe that every patch should get merged eventually. This is practically impossible. We can't merge every single line that was and will be written about flashrom into one project. Not everything is correct and not everything that is is compatible. Sometimes the most efficient thing to do with a patch is to abandon it.
Sorry if I seem worked up over this. FWIW, such problematic reviews are one thing that's been congesting the project over the past three years. Of course, we can all try to learn from such reviews, but it needs to pay out for the project somehow. Otherwise, it would be more efficient and less frustrating if the reviewers would write everything.
And it starts involving lots of people, many of those people should not be involved at all. And worst of all, it can cause emotional damage to people involved. This is all wrong. We need to ask others for sure, but for myself, I am happy to have another meeting, because it has a chance to remove those problems. A patch should be a purely technical question.
Comparing the two roles there is another important point: One can't review their own commits but a core developer can submit their own commits.
This is an important point. Reviewer is a very important second pair of eyes. I see my previous point about “Reviewed-by” tag is not that strong :) But your one is.
It's kind of admin access but without being admin for the whole Gerrit instance. I never tried, but I assume we could remove a -2. In any case this group can change the rules, so it's only a question of how convenient it would be to override a -2.
This is good, so there is a way to remove a -2. I am still in the same position that “merging” and “block from merging” are similar powers with + and - sign, and it makes sense to have it in one group. If this group (very small!) has no trust in each other, let’s solve it.
And there is also a huge psychological component. When somebody knows they have the time to fix things after a revert, that seems fine. But if one doesn't have the time, it can really hurt to see something reverted.
But wait, one needs to fix things if a patch breaks something? It is technically a choice between “revert and fix” or “fix”? In both cases, there is a “fix” involved…
It's a matter of time and responsibility. Who will fix it and when? Will the one who broke things fix them? or the one who discovers the problem? or maybe even a non-involved maintainer? If we don't revert first, it will create pressure to fix things right now (which may break things again).
That's probably why I'm so afraid of rules ;) start with one and you'll have to add a dozen more. We'd have to properly formalize everything around the groups to avoid regressions.
I looked once again in my “what can go wrong” scenarios, and now I think that #2 and #3 can be solved by having clear rules on when someone can be added to Reviewers and Committers (and those clear rules are published so everyone can see). Let’s brainstorm some clear rules? :) The point #1 can be solved by a nice message which explains everything and calms down people ;) So actually, maybe it’s not so bad :)
Technically, I'm beyond the scale already. But I don't want to burden you with that story. There is not much to worry about right now.
If I can help by listening, just tell me!
Today, the most unsettling thing seems to me is that we spend a lot of time to address problems that people may have accidentally imported into the project.
I think this is a connection to a second topic “Release preparations” that I will respond to next :) But what I think, the problems need to be listed, described and assigned. Lots of these you did already!
I've noticed something related in reviews over the years, though. Some- times when reviewers give a lot of comments on Gerrit, among them some critical ones about the overall patch and a lot of nits, the author tends to fix the nits and ignore the critical comments.
We can formalise this. Gerrit has yellow and grey comments. Yellow are supposed to be blocking (major) and grey are non-blocking (minor). We can, at the very least, state very clearly that yellow comments cannot be ignored (and Martin said it can be enforced in Gerrit!). It is still possible to give some random answer, but… better than nothing.
How about we try education first? A hunch tells me that half-baked rules can't fix someone's attitude towards review. Taking Google for instance, there were some patches by CrOS developers since Edward started that endeavor. I don't know what they are told before they push upstream. But I assume it makes a huge difference if you tell them "Do you want to push that upstream?" compared to something like "Do you want to write that for upstream, there's the development guidelines: https://...".
And that’s what Greg said as well, findings can be major and minor. Although in my head I see it as “comments that I expect a reply” and “just comments”. This is why I almost always have yellow comments: I expect a reply :P Unless it is something like “Awesome work thanks!” etc.
And in addition to everything that I said, I have another thought. I hear the words “right direction” and “wrong direction” from time to time (in gerrit comments, and in this thread). And the thing with those words… They are deceptive, because everyone has their own idea in their own head on what is right / wrong direction, and it seems so obvious to the person, and no one thinks of explaining what exactly “right direction” means. But it means different things to different people.
It can be subjective but doesn't have to be. Unless something is obviously objective, figuring out what is what is impossible without talking about it. That's why we point people to the mailing list. We all should exercise this more, IMHO. If things turn out to be subjective and people disagree, it's definitely something for a meeting, though ;)
Cheers, Nico
Hello!
and I vote 100% against it. FWIW, this feature wakes our worst in- stincts. No matter how convinced we are that review is also for our own good, getting a commit merged always feels rewarding. And many people act like it's a necessity to set the resolved tick to gain that reward. I've seen people doing so with every single comment they wrote since day 1 when this feature was activated for coreboot. Let's not repeat every mistake made there.
Also, today Gerrit warns about unresolved comment threads when one tries to submit. Enforcing it brings a marginal convenience but makes the review process less friendly in my experience.
Sometimes comment threads are opened after review. So the `merged` status never implies all-comments-resolved. And that's not bad, IMHO. Sometimes things come up after review and Gerrit makes it easy to continue to discuss changes even after they are merged.
I have mixed feelings about this. On one hand, I understand what you are saying about worst instincts, especially since you are saying you observed examples how the feature can be workarounded. On the other hand, it seems logical to me to have two types of comments: major ones that need to be resolved before merging the patch, and everything else (non-blocking). Also, I am totally fine to leave as is: my own workflow won’t change. I learned over the years that people have a variety of opinions about yellow and grey comments, my workflow is trying to cover all of that anyway :)
There also seems to be a misunderstanding about patches. Some people seem to believe that every patch should get merged eventually.
I think, you gave a perfect explanation to this just above: “getting a commit merged always feels rewarding”
IMO, problematic patches should not be escalated but avoided in the first place. In our wiki it says in multiple places that one should discuss things on the mailing list early so nobody gets frustrated later. Such problematic reviews is exactly why it's stated there. Open-source development is nothing new. Many problems have been fixed in the past already. Please always encourage everybody to try the established ways before experimenting with new ones.
That’s all correct, for sure! But sometimes, established ways do not work (for whatever reasons). And boom, we have a problematic patch. It should not exist, but here it is. You called this “escalation” but what if it is only reviewers discussing, it is not an escalation, it’s the same people who are on the patch, just talking instead of typing? And yes, if everything goes normally , it’s not needed.
Re: encouragement, I always do what I can!
If we don't revert first, it will create pressure to fix things right now (which may break things again).
Agree with that! I agreed with that from the very beginning :)
something like "Do you want to write that for upstream, there's the development guidelines: https://...".
Believe it or not, guidelines are something that we recently discussed (guidelines for people who send patches upstream). But I need to work on this a bit, I will tell later!
However one topic I am not sure how to guide is: gerrit comments. The only thing I understand by now, there are various opinions on gerrit comments.
Also, returning back to initial questions, an idea of having a Reviewers group for flashrom. Does this look good for everyone? This comes together with a question: what are criteria to add someone to Reviewers and Committers?
This probably needs some brainstorming, so I am going to start, I give it 5 mins, everything that comes to mind:
Number of patches owned [for last X months/years] Number of patches reviewed [for last X months/years] Number of patches merged [for last X months/years] Last time the patch was reviewed Last time patch was send to review Last time patch was merged How long active on the project Any topics on the mailing list Reachable via email or channel …..
On Fri, Mar 18, 2022 at 2:44 AM Nico Huber nico.h@gmx.de wrote:
Hi Anastasia,
On 17.03.22 12:32, Anastasia Klimchuk wrote:
For example, the flashrom project doesn't require that all comments be resolved before merge. That can be enabled if you'd like, but currently it isn't.
Oh this explains! I was wondering where those “patches merged with unresolved comments” are coming from. I am 100% voting for this setting to be enabled. It does not prevent from just clicking Ack on all comments, if needed. But at least, comments won’t be lost unintentionally.
and I vote 100% against it. FWIW, this feature wakes our worst in- stincts. No matter how convinced we are that review is also for our own good, getting a commit merged always feels rewarding. And many people act like it's a necessity to set the resolved tick to gain that reward. I've seen people doing so with every single comment they wrote since day 1 when this feature was activated for coreboot. Let's not repeat every mistake made there.
Also, today Gerrit warns about unresolved comment threads when one tries to submit. Enforcing it brings a marginal convenience but makes the review process less friendly in my experience.
Sometimes comment threads are opened after review. So the `merged` status never implies all-comments-resolved. And that's not bad, IMHO. Sometimes things come up after review and Gerrit makes it easy to continue to discuss changes even after they are merged.
And then, we can for example agree (if we agree) that if the Reviewer wants a response on the comment, they mark it Unresolved (it will be yellow colour). It doesn’t solve all the problems, but at least, it guards unintentional mistakes.
it might help to have a weekly or bi-weekly meeting *just* to discuss patches face-to-face.
I would agree with that. This is *another meeting* yes! But looking at how things are going right now, a problematic patch takes so much unnecessary time, it can be weeks or even months, as I said, unnecessary extra time.
IMO, problematic patches should not be escalated but avoided in the first place. In our wiki it says in multiple places that one should discuss things on the mailing list early so nobody gets frustrated later. Such problematic reviews is exactly why it's stated there. Open-source development is nothing new. Many problems have been fixed in the past already. Please always encourage everybody to try the established ways before experimenting with new ones.
There also seems to be a misunderstanding about patches. Some people seem to believe that every patch should get merged eventually. This is practically impossible. We can't merge every single line that was and will be written about flashrom into one project. Not everything is correct and not everything that is is compatible. Sometimes the most efficient thing to do with a patch is to abandon it.
Sorry if I seem worked up over this. FWIW, such problematic reviews are one thing that's been congesting the project over the past three years. Of course, we can all try to learn from such reviews, but it needs to pay out for the project somehow. Otherwise, it would be more efficient and less frustrating if the reviewers would write everything.
And it starts involving lots of people, many of those people should not be involved at all. And worst of all, it can cause emotional damage to people involved. This is all wrong. We need to ask others for sure, but for myself, I am happy to have another meeting, because it has a chance to remove those problems. A patch should be a purely technical question.
Comparing the two roles there is another important point: One can't review their own commits but a core developer can submit their own commits.
This is an important point. Reviewer is a very important second pair of eyes. I see my previous point about “Reviewed-by” tag is not that strong :) But your one is.
It's kind of admin access but without being admin for the whole Gerrit instance. I never tried, but I assume we could remove a -2. In any case this group can change the rules, so it's only a question of how convenient it would be to override a -2.
This is good, so there is a way to remove a -2. I am still in the same position that “merging” and “block from merging” are similar powers with + and - sign, and it makes sense to have it in one group. If this group (very small!) has no trust in each other, let’s solve it.
And there is also a huge psychological component. When somebody knows they have the time to fix things after a revert, that seems fine. But if one doesn't have the time, it can really hurt to see something reverted.
But wait, one needs to fix things if a patch breaks something? It is technically a choice between “revert and fix” or “fix”? In both cases, there is a “fix” involved…
It's a matter of time and responsibility. Who will fix it and when? Will the one who broke things fix them? or the one who discovers the problem? or maybe even a non-involved maintainer? If we don't revert first, it will create pressure to fix things right now (which may break things again).
That's probably why I'm so afraid of rules ;) start with one and you'll have to add a dozen more. We'd have to properly formalize everything around the groups to avoid regressions.
I looked once again in my “what can go wrong” scenarios, and now I think that #2 and #3 can be solved by having clear rules on when someone can be added to Reviewers and Committers (and those clear rules are published so everyone can see). Let’s brainstorm some clear rules? :) The point #1 can be solved by a nice message which explains everything and calms down people ;) So actually, maybe it’s not so bad :)
Technically, I'm beyond the scale already. But I don't want to burden you with that story. There is not much to worry about right now.
If I can help by listening, just tell me!
Today, the most unsettling thing seems to me is that we spend a lot of time to address problems that people may have accidentally imported into the project.
I think this is a connection to a second topic “Release preparations” that I will respond to next :) But what I think, the problems need to be listed, described and assigned. Lots of these you did already!
I've noticed something related in reviews over the years, though. Some- times when reviewers give a lot of comments on Gerrit, among them some critical ones about the overall patch and a lot of nits, the author tends to fix the nits and ignore the critical comments.
We can formalise this. Gerrit has yellow and grey comments. Yellow are supposed to be blocking (major) and grey are non-blocking (minor). We can, at the very least, state very clearly that yellow comments cannot be ignored (and Martin said it can be enforced in Gerrit!). It is still possible to give some random answer, but… better than nothing.
How about we try education first? A hunch tells me that half-baked rules can't fix someone's attitude towards review. Taking Google for instance, there were some patches by CrOS developers since Edward started that endeavor. I don't know what they are told before they push upstream. But I assume it makes a huge difference if you tell them "Do you want to push that upstream?" compared to something like "Do you want to write that for upstream, there's the development guidelines: https://...".
And that’s what Greg said as well, findings can be major and minor. Although in my head I see it as “comments that I expect a reply” and “just comments”. This is why I almost always have yellow comments: I expect a reply :P Unless it is something like “Awesome work thanks!” etc.
And in addition to everything that I said, I have another thought. I hear the words “right direction” and “wrong direction” from time to time (in gerrit comments, and in this thread). And the thing with those words… They are deceptive, because everyone has their own idea in their own head on what is right / wrong direction, and it seems so obvious to the person, and no one thinks of explaining what exactly “right direction” means. But it means different things to different people.
It can be subjective but doesn't have to be. Unless something is obviously objective, figuring out what is what is impossible without talking about it. That's why we point people to the mailing list. We all should exercise this more, IMHO. If things turn out to be subjective and people disagree, it's definitely something for a meeting, though ;)
Cheers, Nico
There are several questions raised in this thread, but good news is that at the meeting yesterday one of the questions was decided on!
We decided to have a Reviewers group for flashrom.
Martin, can we ask you to help with creating a group? If you could help that would be great! Initial content of the group (as it is suggested in the starter email) is a copy of “flashrom developers”, and then we can add people on the top. Thank you!
Given the existing "flashrom developers" , maybe a new group can be called "flashrom reviewers"?
On Sat, Mar 26, 2022 at 9:11 PM Anastasia Klimchuk aklm@chromium.org wrote:
Hello!
and I vote 100% against it. FWIW, this feature wakes our worst in- stincts. No matter how convinced we are that review is also for our own good, getting a commit merged always feels rewarding. And many people act like it's a necessity to set the resolved tick to gain that reward. I've seen people doing so with every single comment they wrote since day 1 when this feature was activated for coreboot. Let's not repeat every mistake made there.
Also, today Gerrit warns about unresolved comment threads when one tries to submit. Enforcing it brings a marginal convenience but makes the review process less friendly in my experience.
Sometimes comment threads are opened after review. So the `merged` status never implies all-comments-resolved. And that's not bad, IMHO. Sometimes things come up after review and Gerrit makes it easy to continue to discuss changes even after they are merged.
I have mixed feelings about this. On one hand, I understand what you are saying about worst instincts, especially since you are saying you observed examples how the feature can be workarounded. On the other hand, it seems logical to me to have two types of comments: major ones that need to be resolved before merging the patch, and everything else (non-blocking). Also, I am totally fine to leave as is: my own workflow won’t change. I learned over the years that people have a variety of opinions about yellow and grey comments, my workflow is trying to cover all of that anyway :)
There also seems to be a misunderstanding about patches. Some people seem to believe that every patch should get merged eventually.
I think, you gave a perfect explanation to this just above: “getting a commit merged always feels rewarding”
IMO, problematic patches should not be escalated but avoided in the first place. In our wiki it says in multiple places that one should discuss things on the mailing list early so nobody gets frustrated later. Such problematic reviews is exactly why it's stated there. Open-source development is nothing new. Many problems have been fixed in the past already. Please always encourage everybody to try the established ways before experimenting with new ones.
That’s all correct, for sure! But sometimes, established ways do not work (for whatever reasons). And boom, we have a problematic patch. It should not exist, but here it is. You called this “escalation” but what if it is only reviewers discussing, it is not an escalation, it’s the same people who are on the patch, just talking instead of typing? And yes, if everything goes normally , it’s not needed.
Re: encouragement, I always do what I can!
If we don't revert first, it will create pressure to fix things right now (which may break things again).
Agree with that! I agreed with that from the very beginning :)
something like "Do you want to write that for upstream, there's the development guidelines: https://...".
Believe it or not, guidelines are something that we recently discussed (guidelines for people who send patches upstream). But I need to work on this a bit, I will tell later!
However one topic I am not sure how to guide is: gerrit comments. The only thing I understand by now, there are various opinions on gerrit comments.
Also, returning back to initial questions, an idea of having a Reviewers group for flashrom. Does this look good for everyone? This comes together with a question: what are criteria to add someone to Reviewers and Committers?
This probably needs some brainstorming, so I am going to start, I give it 5 mins, everything that comes to mind:
Number of patches owned [for last X months/years] Number of patches reviewed [for last X months/years] Number of patches merged [for last X months/years] Last time the patch was reviewed Last time patch was send to review Last time patch was merged How long active on the project Any topics on the mailing list Reachable via email or channel …..
On Fri, Mar 18, 2022 at 2:44 AM Nico Huber nico.h@gmx.de wrote:
Hi Anastasia,
On 17.03.22 12:32, Anastasia Klimchuk wrote:
For example, the flashrom project doesn't require that all comments be resolved before merge. That can be enabled if you'd like, but currently it isn't.
Oh this explains! I was wondering where those “patches merged with unresolved comments” are coming from. I am 100% voting for this setting to be enabled. It does not prevent from just clicking Ack on all comments, if needed. But at least, comments won’t be lost unintentionally.
and I vote 100% against it. FWIW, this feature wakes our worst in- stincts. No matter how convinced we are that review is also for our own good, getting a commit merged always feels rewarding. And many people act like it's a necessity to set the resolved tick to gain that reward. I've seen people doing so with every single comment they wrote since day 1 when this feature was activated for coreboot. Let's not repeat every mistake made there.
Also, today Gerrit warns about unresolved comment threads when one tries to submit. Enforcing it brings a marginal convenience but makes the review process less friendly in my experience.
Sometimes comment threads are opened after review. So the `merged` status never implies all-comments-resolved. And that's not bad, IMHO. Sometimes things come up after review and Gerrit makes it easy to continue to discuss changes even after they are merged.
And then, we can for example agree (if we agree) that if the Reviewer wants a response on the comment, they mark it Unresolved (it will be yellow colour). It doesn’t solve all the problems, but at least, it guards unintentional mistakes.
it might help to have a weekly or bi-weekly meeting *just* to discuss patches face-to-face.
I would agree with that. This is *another meeting* yes! But looking at how things are going right now, a problematic patch takes so much unnecessary time, it can be weeks or even months, as I said, unnecessary extra time.
IMO, problematic patches should not be escalated but avoided in the first place. In our wiki it says in multiple places that one should discuss things on the mailing list early so nobody gets frustrated later. Such problematic reviews is exactly why it's stated there. Open-source development is nothing new. Many problems have been fixed in the past already. Please always encourage everybody to try the established ways before experimenting with new ones.
There also seems to be a misunderstanding about patches. Some people seem to believe that every patch should get merged eventually. This is practically impossible. We can't merge every single line that was and will be written about flashrom into one project. Not everything is correct and not everything that is is compatible. Sometimes the most efficient thing to do with a patch is to abandon it.
Sorry if I seem worked up over this. FWIW, such problematic reviews are one thing that's been congesting the project over the past three years. Of course, we can all try to learn from such reviews, but it needs to pay out for the project somehow. Otherwise, it would be more efficient and less frustrating if the reviewers would write everything.
And it starts involving lots of people, many of those people should not be involved at all. And worst of all, it can cause emotional damage to people involved. This is all wrong. We need to ask others for sure, but for myself, I am happy to have another meeting, because it has a chance to remove those problems. A patch should be a purely technical question.
Comparing the two roles there is another important point: One can't review their own commits but a core developer can submit their own commits.
This is an important point. Reviewer is a very important second pair of eyes. I see my previous point about “Reviewed-by” tag is not that strong :) But your one is.
It's kind of admin access but without being admin for the whole Gerrit instance. I never tried, but I assume we could remove a -2. In any case this group can change the rules, so it's only a question of how convenient it would be to override a -2.
This is good, so there is a way to remove a -2. I am still in the same position that “merging” and “block from merging” are similar powers with + and - sign, and it makes sense to have it in one group. If this group (very small!) has no trust in each other, let’s solve it.
And there is also a huge psychological component. When somebody knows they have the time to fix things after a revert, that seems fine. But if one doesn't have the time, it can really hurt to see something reverted.
But wait, one needs to fix things if a patch breaks something? It is technically a choice between “revert and fix” or “fix”? In both cases, there is a “fix” involved…
It's a matter of time and responsibility. Who will fix it and when? Will the one who broke things fix them? or the one who discovers the problem? or maybe even a non-involved maintainer? If we don't revert first, it will create pressure to fix things right now (which may break things again).
That's probably why I'm so afraid of rules ;) start with one and you'll have to add a dozen more. We'd have to properly formalize everything around the groups to avoid regressions.
I looked once again in my “what can go wrong” scenarios, and now I think that #2 and #3 can be solved by having clear rules on when someone can be added to Reviewers and Committers (and those clear rules are published so everyone can see). Let’s brainstorm some clear rules? :) The point #1 can be solved by a nice message which explains everything and calms down people ;) So actually, maybe it’s not so bad :)
Technically, I'm beyond the scale already. But I don't want to burden you with that story. There is not much to worry about right now.
If I can help by listening, just tell me!
Today, the most unsettling thing seems to me is that we spend a lot of time to address problems that people may have accidentally imported into the project.
I think this is a connection to a second topic “Release preparations” that I will respond to next :) But what I think, the problems need to be listed, described and assigned. Lots of these you did already!
I've noticed something related in reviews over the years, though. Some- times when reviewers give a lot of comments on Gerrit, among them some critical ones about the overall patch and a lot of nits, the author tends to fix the nits and ignore the critical comments.
We can formalise this. Gerrit has yellow and grey comments. Yellow are supposed to be blocking (major) and grey are non-blocking (minor). We can, at the very least, state very clearly that yellow comments cannot be ignored (and Martin said it can be enforced in Gerrit!). It is still possible to give some random answer, but… better than nothing.
How about we try education first? A hunch tells me that half-baked rules can't fix someone's attitude towards review. Taking Google for instance, there were some patches by CrOS developers since Edward started that endeavor. I don't know what they are told before they push upstream. But I assume it makes a huge difference if you tell them "Do you want to push that upstream?" compared to something like "Do you want to write that for upstream, there's the development guidelines: https://...".
And that’s what Greg said as well, findings can be major and minor. Although in my head I see it as “comments that I expect a reply” and “just comments”. This is why I almost always have yellow comments: I expect a reply :P Unless it is something like “Awesome work thanks!” etc.
And in addition to everything that I said, I have another thought. I hear the words “right direction” and “wrong direction” from time to time (in gerrit comments, and in this thread). And the thing with those words… They are deceptive, because everyone has their own idea in their own head on what is right / wrong direction, and it seems so obvious to the person, and no one thinks of explaining what exactly “right direction” means. But it means different things to different people.
It can be subjective but doesn't have to be. Unless something is obviously objective, figuring out what is what is impossible without talking about it. That's why we point people to the mailing list. We all should exercise this more, IMHO. If things turn out to be subjective and people disagree, it's definitely something for a meeting, though ;)
Cheers, Nico
-- Anastasia.
On 06.05.22 03:52, Anastasia Klimchuk wrote:
There are several questions raised in this thread, but good news is that at the meeting yesterday one of the questions was decided on!
We decided to have a Reviewers group for flashrom.
Technically the attendees agreed on it. Which is a kind of a random selection of people. I would prefer if topics that ask for a decision get their own mailing list thread. Not that I expect anyone to object, but it still would be nice to give busy people a chance to notice it. A little courtesy can't hurt, can it?
Nico
(Generally, when one top-posts it's a sign that the message might deserve its own thread.)
On Sat, 2022-05-07 at 14:39 +0200, Nico Huber wrote:
On 06.05.22 03:52, Anastasia Klimchuk wrote:
There are several questions raised in this thread, but good news is that at the meeting yesterday one of the questions was decided on!
We decided to have a Reviewers group for flashrom.
Technically the attendees agreed on it. Which is a kind of a random selection of people.
I wouldn't call these people "a random selection of people". Looking at the flashrom developers group, these are the ones who are most active in the project. Angel didn't attend the meeting and though he noticed the related commit which does the change [1]. So it's not like people are unable to notice it.
What wouldn't be a random selection of people? Who are the right people if not the active ones?
I would prefer if topics that ask for a decision get their own mailing list thread. Not that I expect anyone to object, but it still would be nice to give busy people a chance to notice it. A little courtesy can't hurt, can it?
How much courtesy does it need to make a decision? Who else do you expect to reply?
Seriously, this thread has been on the mailing list for two months now. If the subject of the thread ("Gatekeeping, ACLs and Review Rules") is not eye-catching enough, well.. Then I don't know. I think people had enough time to read and comment. I can't see that anyone in this thread is objecting on this specific topic.
Then, it was a topic on the meeting agenda, which is public viewable, and it was discussed there. No one objected. Neither before, during the meeting nor after the meeting.
And finally, I also sent an email with the meeting notes (agenda + notes + a somewhat summary of the meeting) to the mailing list. The summary is on the top of the mail and contains "We want a separate reviewers group for flashrom so that reviewers are not mixed across different projects". No one objected.
I think everyone had enough chances to comment and to object, but instead only the same people reply. So why should we wait and use more time on this? The time can be used better elsewhere.
To answer your question: Actually, I think it can hurt. For example, when it slows down or even blocks a decision for an unknown reason. This hurts a lot.
// Felix
On 09.05.22 06:31, Felix Singer wrote:
On Sat, 2022-05-07 at 14:39 +0200, Nico Huber wrote:
On 06.05.22 03:52, Anastasia Klimchuk wrote:
There are several questions raised in this thread, but good news is that at the meeting yesterday one of the questions was decided on!
We decided to have a Reviewers group for flashrom.
Technically the attendees agreed on it. Which is a kind of a random selection of people.
I wouldn't call these people "a random selection of people". Looking at the flashrom developers group, these are the ones who are most active in the project.
Thomas and Nikolai were there. It feels like I could find somebody more active who didn't attend for most if not all the other attendees.
Angel didn't attend the meeting and though he noticed the related commit which does the change [1]. So it's not like people are unable to notice it.
That change[1] is mostly unrelated to what was discussed, though. And doesn't link to anything.
What wouldn't be a random selection of people? Who are the right people if not the active ones?
All people interested. At least those that are registered to the mailing list to take part.
I would prefer if topics that ask for a decision get their own mailing list thread. Not that I expect anyone to object, but it still would be nice to give busy people a chance to notice it. A little courtesy can't hurt, can it?
How much courtesy does it need to make a decision?
I don't know. One email with a fitting subject line shouldn't be too much to ask for, IMO.
Who else do you expect to reply?
Depends on the contents of the email.
Seriously, this thread has been on the mailing list for two months now. If the subject of the thread ("Gatekeeping, ACLs and Review Rules") is not eye-catching enough, well.. Then I don't know. I think people had enough time to read and comment.
Did you? Suddenly you seem concerned about the topic... IMO, it's not that you did anything wrong. That thread was meant to discuss the whole working-together-on-Gerrit topic. Now somebody picked something useful out of it and asked for a decision. That's two different things and it's only natural, IMHO, that more people get more involved with the latter.
I can't see that anyone in this thread is objecting on this specific topic.
Pretty much expected as the thread wasn't about making any final decision.
Then, it was a topic on the meeting agenda, which is public viewable,
Can't see it, must have missed it. It's much easier to miss than emails with proper subject.
and it was discussed there. No one objected. Neither before, during the meeting nor after the meeting.
IIRC, we decided (first meeting?) to keep using the mailing list for decisions. If I'd have to expect things to be set in stone if I don't object during the meeting I would have to object to everything that takes me by surprise. I've seen such behavior and I don't think it would make productive meetings.
And finally, I also sent an email with the meeting notes (agenda + notes + a somewhat summary of the meeting) to the mailing list. The summary is on the top of the mail and contains "We want a separate reviewers group for flashrom so that reviewers are not mixed across different projects". No one objected.
Yes, but it doesn't say much more than this. And I still agree with it. What it doesn't say: We'd take immediate action. Without any plan btw.
I think everyone had enough chances to comment and to object, but instead only the same people reply. So why should we wait and use more time on this? The time can be used better elsewhere.
What chances? It's completely different to discuss the possibility of something or to say we want to do it now. Especially when there are unanswered questions. Like who is going to be in that group? how can people get their +2 rights back that they lose this way?
To answer your question: Actually, I think it can hurt. For example, when it slows down or even blocks a decision for an unknown reason. This hurts a lot.
If it's a stupid decision, slowing down would be a good thing, wouldn't it?
Nico
I am not going to comment on most things of your email, because I think the discussion would be going nowhere. Also, you should know my answer for some things. So there is no point for me in replying.
Just one thing..
On Mon, 2022-05-09 at 14:19 +0200, Nico Huber wrote:
I would prefer if topics that ask for a decision get their own mailing list thread. Not that I expect anyone to object, but it still would be nice to give busy people a chance to notice it. A little courtesy can't hurt, can it?
How much courtesy does it need to make a decision?
I don't know. One email with a fitting subject line shouldn't be too much to ask for, IMO.
Feel free to do so. If you want another thread, you can create one at any time. Nobody is preventing you from doing that.
// Felix
Hello Everyone,
First of all let's summarize what has happened. Today is May 12.
Two months ago (Mar 6) Nico started this thread on the mailing list: "Gatekeeping, ACLs and Review Rules". I recommend everyone [who is interested] re-read the opening email in this thread. It is well written, analyses existing issues on flashrom project, gives historical information, suggests solutions and rationale behind the solutions. There are several ideas, among them the idea to create a "flashrom reviewers" group in gerrit. The group gives rights to +2 patches (but does not give submit rights).
The thread has been active for two months, and various people joined the discussion. People either supported the idea of the "flashrom reviewers" group, or did not comment / did not object. After two months, the topic was raised and discussed at the dev meeting (May 5). "flashrom reviewers" group was approved by all attendees. We made a decision to create this group, and documented the decision (see Meeting notes, Decision summary at 5th May 2022 https://docs.google.com/document/d/18qKvEbfPszjsJJGJhwi8kRVDUG3GZkADzQSH6WFs... ). Just to be clear, Nico was present at the meeting on 5th May.
Few hours after the meeting (that was still 5th May), we sent an email with meeting notes, https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/thread/VGTQI... Email contains meeting notes in full, and if someone is busy and has no time to read notes in full, there is "Decision Summary" at the top.
Next day, 6th May I posted to this thread, once again announcing that we made the decision to create "flashrom reviewers" group and asked Martin to help. He really helped and created a patch https://review.coreboot.org/c/flashrom/+/64101 . The patch is under review, it has some comments.
So I want to confirm that all steps for discussion and decision were done properly and publicly.
Secondly, let's summarize the issues solved by the "flashrom reviewers" group and next goals.
In the absence of "flashrom reviewers", everyone in "coreboot reviewers" has rights to +2 flashrom patches. "coreboot reviewers" is a large group, not all people know flashrom code well, not all people work on flashrom. Which leads to repeated complaints like "Why was this patch merged? It wasn't properly reviewed!" Mostly Nico complained about that, so not surprisingly he described the issue and suggested the solution (see the opening email in the thread).
Another point, having our own group makes it much more clear for patch owners on what the expectations are. A reviewer who has +2 right has enough experience on flashrom and can approve the patch. If a reviewer only says +1 this means someone else needs to approve, or patch needs more work.
What is the initial list of people in the group? Thankfully, Nico has a good and reasonable suggestion in the first email, too:
Changing this would imply that we need our own group of reviewers. With this, a +2 would become both rarer and stronger. If this idea meets some consent, I would propose to copy the "flashrom developers" group for a start. And when we identify somebody over time who shows in-depth knowledge about at least a part of flashrom and good self-judgement if they are experienced enough for a +2, we could add them. Right now, some candidates come to mind already, Peter, Nikolai, and Thomas.
The very important next goal to discuss: the rules and criteria on how to add new people into the "flashrom reviewers" group. We could not enforce any rules on coreboot reviewers, but now once we have our own group, we can have our own rules. Similarly, "flashrom developers" group has no rules defined. We should discuss this too.
Great thing is that now flashrom has a place where core devs and active members of the community can make decisions: we have a meeting! Mailing list and IRC channel have been here since forever, they are good for discussions but don’t work for making decisions. A project needs a place where decisions can be made.
And one more thing, I noticed there are some incorrect statements on this thread. I am sure this was not intentional, very likely just emotional, but still I want things to be very very clear.
That change[1] is mostly unrelated to what was discussed
This is not true. The patch https://review.coreboot.org/c/flashrom/+/64101 is modifying gerrit config and adding "flashrom reviewers" group, which is exactly what was discussed. However, the patch is doing two things in one, so I added a comment and asked if it is technically possible to split.
IIRC, we decided (first meeting?) to keep using the mailing list for decisions
This is not true. Decisions summary for the first meeting has one item: "Will make this meeting bi-weekly until we've worked our way through the issues listed here." I checked the decision summary for all the meetings we had to that time, none of them said we "keep using the mailing list for decisions".
the attendees <...> is a kind of a random selection of people
This is not true. The attendees of the meeting are active members of the flashrom community, who are deeply interested to discuss and solve current issues and improve the project. People who invest a lot of their time into flashrom project. This one, I am actually not sure whether it is an incorrect statement or an insult. Again, most likely unintentional… but it would be great to have some kind of “sorry”.
Hi folks,
On 12.05.22 11:04, Anastasia Klimchuk wrote:
The thread has been active for two months, and various people joined the discussion. People either supported the idea of the "flashrom reviewers" group, or did not comment / did not object. After two months, the topic was raised and discussed at the dev meeting (May 5). "flashrom reviewers" group was approved by all attendees. We made a decision to create this group, and documented the decision (see Meeting notes, Decision summary at 5th May 2022 https://docs.google.com/document/d/18qKvEbfPszjsJJGJhwi8kRVDUG3GZkADzQSH6WFs... ). Just to be clear, Nico was present at the meeting on 5th May.
Few hours after the meeting (that was still 5th May), we sent an email with meeting notes, https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/thread/VGTQI... Email contains meeting notes in full, and if someone is busy and has no time to read notes in full, there is "Decision Summary" at the top.
there were two misunderstandings on my end, both not clear from the notes either. 1. It says "we want a [...] group". To me that says, nobody would waste time if they start planing, making arrangements in that direction. But it doesn't say we are going to make it right now without further planing (to me). 2. I didn't expect that we'd be making final decisions for the project during that meeting. When we say "we want ...", "we decide ..." I assume we decide that for the attendees and non-attendees can still have a word.
Whatever, misunderstandings happen, so I just told myself it will work out anyway. To avoid friction with non-attendees, I suggested to announce it more visibly.
Great thing is that now flashrom has a place where core devs and active members of the community can make decisions: we have a meeting!
tl;dr Doesn't work for me. I'm not a native English speaker and spend most of the time of these meeting concentrating on the language.
I feared people could use the meeting to force decisions at some point. But hoped we could find some balance, a good compromise. I guess it's like the saying, give them an inch and they'll take an ell.
Here's how things look like when we don't compromise:
* I'll not take part in any verbal decision making not in my native language (German). * I'll not accept any decisions when the topic wasn't brought up at least one week in advance with maximum visibility via an unalterable medium (e.g. this mailing list on a thread per topic). * I'll not attend meetings unless the scope of those is clear and we can agree on the scope. For instance, can the meeting make decisions on behalf of the project? * I'll not use meeting infrastructure of companies who's business model is focused on building profiles of people.
Mailing list and IRC channel have been here since forever, they are good for discussions but don’t work for making decisions.
Please, elaborate on this.
A project needs a place where decisions can be made.
And one more thing, I noticed there are some incorrect statements on this thread. I am sure this was not intentional, very likely just emotional, but still I want things to be very very clear.
That change[1] is mostly unrelated to what was discussed
This is not true. The patch https://review.coreboot.org/c/flashrom/+/64101 is modifying gerrit config and adding "flashrom reviewers" group, which is exactly what was discussed. However, the patch is doing two things in one, so I added a comment and asked if it is technically possible to split.
"2 files changed, 7 insertions(+), 23 deletions(-)"
2 of the added lines were about the new group. One changed line was partially about the new group. The rest seemed unrelated.
IIRC, we decided (first meeting?) to keep using the mailing list for decisions
This is not true. Decisions summary for the first meeting has one item: "Will make this meeting bi-weekly until we've worked our way through the issues listed here." I checked the decision summary for all the meetings we had to that time, none of them said we "keep using the mailing list for decisions".
Yeah, decided was the wrong word there. I guess I brought it up. But can't really prove nor remember anything.
the attendees <...> is a kind of a random selection of people
This is not true. The attendees of the meeting are active members of the flashrom community, who are deeply interested to discuss and solve current issues and improve the project. People who invest a lot of their time into flashrom project. This one, I am actually not sure whether it is an incorrect statement or an insult. Again, most likely unintentional… but it would be great to have some kind of “sorry”.
Right, I had the flashrom context in mind, i.e. s/people/people involved with flashrom/. If that offended anyone, sorry. So it's a random subset of the people involved with flashrom. Well, that's what I wanted to say but that's not correct either. It's not a true random selection. It's biased, of course. Based on people's experience with the meeting (i.e. what is usually discussed and if that's worth their time), the date/time of course, how well they speak the language, if they are paid for it etc.
Nico
Hello,
I am also not a native English speaker, and need to always concentrate on the language. Sometimes I can't understand the person who has just spoken, in this case I am trying to clarify for example by asking "do I understand you correctly, do you mean ABCXYZ?". Also I remember a few times when I noticed you were frowning and not saying anything, I asked you "Nico what do you think? Do you agree?" etc.
There is also a Raise Hand button, you can click when someone is talking and you don't want to interrupt. People always pay attention to raised hands, they are never lost.
In any case, if you have any other suggestions/strategies, please share! We will use them. I just looked, for all the meetings we got to the time, native English speakers are less than a half of attendees. So any concrete suggestions on how to improve experience for all of us, very welcome.
By the way,
give them an inch and they'll take an ell
I don't understand this (maybe because I am non-native English). I see this is a metaphor, but I don't understand how to map it into the reality of flashrom project. Who "gives"? Who are "them"? What do you mean by "inch" and what do you mean by "ell"?
Also, in your statement "Here's how things look like when we don't compromise"
There are two things that I don't understand.
First of all, who are "we"?
Secondly, compromise with what? You provided a list of things you won't do, you said "I'll not" 4 times. But what do you do? What is your suggestion for the decision making process, can you describe the complete process?
Just to be clear, my suggestion for decision making process:
#1 Discuss things on the mailing list first (it could be ongoing for months, not a problem - so that everyone interested has time to read and respond) #2 When/if the discussion seems to be settled and people are in agreement, the item can be added to the agenda of the meeting. #3 At the meeting the item can be discussed again, and a decision can be made (or not made, if people disagree). #4 After the meeting we send an email with meeting notes and “Decisions summary” on the top of the email. #5 Since stuff never gets done immediately, there always be some time to react on #4
And I am really interested in what other people think about the suggestion!
Also I am thinking: decisions from all previous meetings were fine, you never protested before. We had 5 meetings already, each one made at least one decision. The number of decisions per meeting: 1, 4, 3, 1, 4. And now you are protesting, when we decided to go ahead with your idea? Your own good idea, which solves real issues on the project, the issues that you regularly complain about? I don't understand your behavior. Can you please explain why you behave like this?
If you have concerns and the software we are currently using, you are welcome to organise a video call with any other software that you prefer. Create a meeting room, write instructions for people how to join, and we will start using it. I am happy to help spread the word and tell everyone when you set up new software, and when everything is ready to switch from current one into a new thing. I set up the software that I am familiar with. Me and Felix spent a lot of time making sure all the use cases work, so that no one needs to spend time troubleshooting during the actual meeting. So my reason for this choice was: I know how it works, I know the features, where are the buttons etc.
Mailing list and IRC channel have been here since forever, they are good for discussions but don’t work for making decisions.
Please, elaborate on this.
Please explain how the decision making process would work on the mailing list. And, specifically, please define the criteria to detect when a decision is "done". Please give an example, a link to a flashrom mailing list thread where the decision was made. Good thing we have archives, so even a thread from some time ago can be linked.
I don't see a realistic way for flashrom to make decisions on the mailing list, and I have never seen that happen. What I see is that issues keep repeating again and again for years, and questions are not answered.
For example, a question that was added to the agenda of the first meeting (which was >2months ago): "how do people get commit rights on flashrom?" No one has an answer. Gerrit groups were created in 2017, which is 5 years ago, and yet for all the years flashrom has no rules and no criteria on how people get commit rights! Mailing list was functioning for all the time, so why the question haven't been answered in 5 years?
Finally, I noticed some statements that can be misleading. Surely not intentional, but I want things to be very very clear.
I feared people could use the meeting to force decisions at some point.
I don't know what you meant to say, but this statement may look as if you are suggesting that decisions are forced at the meeting. This is not true. All the items are discussed and decisions made when all people agree.
It's not a true random selection. It's biased, of course. Based on people's experience with the meeting (i.e. what is usually discussed and if that's worth their time), the date/time of course, how well they speak the language, if they are paid for it etc.
Why "it's biased"? and especially why "of course"? I don't know what you meant to say, but this statement may look as if you are suggesting that the meeting is targeting/preferring some people over the others. This is not true. Meeting is as open as possible, all information is public, everyone can join and no account is needed. Meeting agenda for future and notes from the past are public. I regularly remind people about the meeting both on the mailing list and IRC channel.
Speaking about that: next meeting is this Thursday, 19th May 2022, 6:00 - 7:00 am UTC+0 Everyone interested is very welcome! Please don't forget to convert to your local time. Meeting doc link https://docs.google.com/document/d/18qKvEbfPszjsJJGJhwi8kRVDUG3GZkADzQSH6WFs...
Hi Anastasia,
On 16.05.22 07:15, Anastasia Klimchuk wrote:
I am also not a native English speaker, and need to always concentrate on the language. Sometimes I can't understand the person who has just spoken, in this case I am trying to clarify for example by asking "do I understand you correctly, do you mean ABCXYZ?". Also I remember a few times when I noticed you were frowning and not saying anything, I asked you "Nico what do you think? Do you agree?" etc.
that is right, I often keep some thoughts for myself. Simple reason: I fear the meetings would become much less efficient otherwise.
give them an inch and they'll take an ell
I don't understand this (maybe because I am non-native English). I see this is a metaphor, but I don't understand how to map it into the reality of flashrom project. Who "gives"? Who are "them"? What do you mean by "inch" and what do you mean by "ell"?
Hmmm, I got this out of an German-English dictionary. But I have to admit, I can't recall ever having heard this. There's a similar phrase in German, different nouns, though (pinky and hand). The idea of it is that when you support somebody and they don't realize it (or are just selfish), they might ask for more support without showing gratitude (or exploit you deliberately).
The inch could be having the video meetings. They, the Googlers, because the meetings are a great way to include you folks. The ell would be using these meetings to decide on matters like revoking +2 rights of people. IMHO, too much. Because as much as we can include people who like such meetings, I'm afraid we might leave people behind who don't.
Also, in your statement "Here's how things look like when we don't compromise"
There are two things that I don't understand.
First of all, who are "we"?
All of us. Which includes myself.
Secondly, compromise with what? You provided a list of things you won't do, you said "I'll not" 4 times. But what do you do?
I meant to imply that. If I say without compromise "I'll not", then I mean "I'd do" the same if I keep compromising.
What is your suggestion for the decision making process, can you describe the complete process?
When I find the time, sure.
Just to be clear, my suggestion for decision making process:
#1 Discuss things on the mailing list first (it could be ongoing for months, not a problem - so that everyone interested has time to read and respond) #2 When/if the discussion seems to be settled and people are in agreement, the item can be added to the agenda of the meeting. #3 At the meeting the item can be discussed again, and a decision can be made (or not made, if people disagree). #4 After the meeting we send an email with meeting notes and “Decisions summary” on the top of the email. #5 Since stuff never gets done immediately, there always be some time to react on #4
And I am really interested in what other people think about the suggestion!
Great, then put it into another thread, please, with a matching subject. It's really too hidden here. Especially when people start to squabble, other people won't continue reading a thread.
Also I am thinking: decisions from all previous meetings were fine, you never protested before.
Yes, I didn't. I guess because we only made decisions about the meeting itself, the people who attended, or about some long-term strategy like the Meson transition. AFAIR, nothing that immediately affected non- attendees.
We had 5 meetings already, each one made at least one decision. The number of decisions per meeting: 1, 4, 3, 1, 4. And now you are protesting, when we decided to go ahead with your idea? Your own good idea, which solves real issues on the project, the issues that you regularly complain about? I don't understand your behavior. Can you please explain why you behave like this?
As I already tried to explain there were misunderstandings. And I don't even know what you mean with "behave like this". I merely asked to write a proper email that the affected folks can notice easily. No idea why anybody would want to fight this.
If you have concerns and the software we are currently using, you are welcome to organise a video call with any other software that you prefer. Create a meeting room, write instructions for people how to join, and we will start using it. I am happy to help spread the word and tell everyone when you set up new software, and when everything is ready to switch from current one into a new thing.
I won't. It's not me who wants these meetings.
I set up the software that I am familiar with. Me and Felix spent a lot of time making sure all the use cases work, so that no one needs to spend time troubleshooting during the actual meeting. So my reason for this choice was: I know how it works, I know the features, where are the buttons etc.
Mailing list and IRC channel have been here since forever, they are good for discussions but don’t work for making decisions.
Please, elaborate on this.
Please explain how the decision making process would work on the mailing list. And, specifically, please define the criteria to detect when a decision is "done". Please give an example, a link to a flashrom mailing list thread where the decision was made. Good thing we have archives, so even a thread from some time ago can be linked.
I don't see a reason too. I'm not the one who's making claims. You say it's not working. So please show an example. Then we can figure out how to do better.
I don't see a realistic way for flashrom to make decisions on the mailing list, and I have never seen that happen. What I see is that issues keep repeating again and again for years, and questions are not answered.
Example?
For example, a question that was added to the agenda of the first meeting (which was >2months ago): "how do people get commit rights on flashrom?" No one has an answer. Gerrit groups were created in 2017, which is 5 years ago, and yet for all the years flashrom has no rules and no criteria on how people get commit rights! Mailing list was functioning for all the time, so why the question haven't been answered in 5 years?
Maybe nobody has asked. Maybe nobody had to ask? What do you want me to say? Mailing lists are no magical answering machines that predict questions.
Also, I remember having answered that question lately. Not sure if it was during one of the meetings. People who gained trust and kept contri- buting earned such rights.
Finally, I noticed some statements that can be misleading. Surely not intentional, but I want things to be very very clear.
I feared people could use the meeting to force decisions at some point.
I don't know what you meant to say, but this statement may look as if you are suggesting that decisions are forced at the meeting. This is not true. All the items are discussed and decisions made when all people agree.
When the attendees agree and then it would be set in stone, it's kind of forced upon the non-attendees.
It's not a true random selection. It's biased, of course. Based on people's experience with the meeting (i.e. what is usually discussed and if that's worth their time), the date/time of course, how well they speak the language, if they are paid for it etc.
Why "it's biased"? and especially why "of course"?
I think it's quite human. Everybody has their own reasons to attend or not to attend. There's no lottery that decides who attends.
I don't know what you meant to say, but this statement may look as if you are suggesting that the meeting is targeting/preferring some people over the others.
Not deliberately, but is water wet? For instance, we decided to pick times that suit EU/AU folks.
This is not true. Meeting is as open as possible, all information is public, everyone can join and no account is needed. Meeting agenda for future and notes from the past are public. I regularly remind people about the meeting both on the mailing list and IRC channel.
Sure it's open to everyone. But does it attract everyone? Does everybody always have the time? I'm not saying we do anything wrong having mee- tings like this. It's just not a prefect forum either.
Nico
-
Anastasia Klimchuk -
Felix Singer -
Greg Troxel -
Martin L Roth -
Nico Huber