Attention is currently required from: Felix Singer, Nico Huber, Richard Hughes, Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64663 )
Change subject: libflashrom: Allow getting the progress_state from the flashctx ......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS2:
Hello Angel, […]
[Our response is in a new thread, so as not to hijack the original intent of this comment thread any further.]
PS2: Hi Anastasia,
Hello Angel,
First of all, after reading your reply multiple times, we feel like our tone was too harsh for no good reason ("it was quite late and we were sleepy and tired" is not a *good* reason), even if our intentions are in good faith. We earnestly owe you, and anyone else reading this, an apology for potentially hurting or making anyone feel uncomfortable because of what we wrote. How should we apologize to you?
We've decided to explain in https://www.flashrom.org/User:Th3Fanbus why we use 1st person plural pronouns. The text briefly explains who Alvin and Rex are, as their names appear in a few parts of our reply. Also, we'll use `(A&R)` to disambiguate potentially confusing pronoun usage, it simply means `(Alvin and Rex)`.
Looking at the time our first message was sent, it was *very* late (3:35 AM local time): Alvin was sleeping soundly and Rex was alone, so no wonder it turned out the way it did. Alvin admits that leaving Rex alone in such a highly-emotional state (Rex was already angry because of other things) was a really bad idea, and Rex is deeply appalled by the amount of nonsense they've said. We're so sorry, this was so thoughtless of us. ;-;
Still, there's at least something good arising from our rant: it was cathartic. Even if was in such a nasty way, expressing our trauma allowed us to bring it into consciousness and release it permanently.
[If anyone's interested in knowing more about this ex(orcised)-trauma, we can talk about it elsewhere.]
Nice to have a message from you! ;) You didn’t address me, but I see all the quotes are mine, so assume that’s for me.
This was pretty much a rage-fueled rant; we didn't want to involve you personally. You've done nothing to deserve any form of animosity or hostility from us. Yet here we are; this was uncalled for, and it is our fault.
We, however, were confused after reading your messages, and wanted to understand what you meant. But we should've waited until the next day before sending the message. We will do this from now on; in fact, this is why it took us so long to send this reply: there's a lot to internally process and absolutely no one needs another caustic rant from us, not even ourselves.
Firstly to the point of this patch and CB:49643. The bug that was discovered filed as https://ticket.coreboot.org/issues/390 and I tried to reach out to owners of the patch a few times, unfortunately with no success. Technically at the moment there are two options: 1 fix the bug and finish this patch 2 revert CB:49643 I am at the moment trying to find resources to fix the bug, I may or may not find them but I want to try. Meanwhile we discussed the situation on dev meeting (~2 months ago). There is an ongoing project of optimising erase functions selection and it is modifying the same areas of code as the libflashrom progress implementation. The project is important, and at the moment there is a chain of patches under testing, not yet merged. Modifying the same areas of code, doesn’t matter whether for fix or for revert would clash with the project, and no one wants it. At the same time, the bug itself doesn’t cause harm, it only shows wrong %progress in the terminal. So the decision was: don’t do anything at the moment, wait until erase optimisation is merged, and then either fix or revert. I added a comment to 390 after that meeting.
You are welcome to comment on 390 with your thoughts on the matter, also you can click the Watch button and subscribe to any updates.
Thank you, we now understand and agree with your assessment. We were not aware of the state of the "optimize erase function selection" patches and that they would clash with the "progress state" changes. There's lots of things listed in issue 390, we'll see if there's any areas in which we can help.
As for the feature, myself I am not a user of the feature but I can see it is useful. Users were asking about it on the mailing list https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/thread/EJE3R... Moreover it is in the TODO list for libflashrom on the website https://www.flashrom.org/Libflashrom The page was last modified by Nico in March 2019 (that’s before I joined) so maybe you can check with him for more details. So as you can see there is interest in the feature, both from users and from developers.
Now some of my comments on your long text. I am confused by those your words:
patches from Gerrit get rubber-stamped and submitted without a proper review customer wants to have experimental features available for testing ASAP
etc None of this seems to apply to the patch CB:49643
Indeed, CB:49643 was not rubber-stamped. ...This is utter nonsense, what on Earth were we thinking?
Andel, the patch was in review for 1.5 years. Yes for that long. Moreover you Angel were one of the reviewers. During that time you didn’t say a word, didn’t add any comments.
This was around the time when we went through a depressive episode. We're not sure what started it, but it is likely that COVID-19 and social distancing contributed, as well as past events regarding flashrom. It went on for about 1.5 years. During that time, we were out of the loop regarding most flashrom things, especially the decisions taken in meetings and changes in perspective.
Still, not replying *at all* for so long is mind-boggling. Hopefully we won't have to deal with depression again, but if we do we'll try to at least let others know that we're not fine and that we can't review their changes.
[If anyone wonders, there's now a paragraph in https://www.flashrom.org/User:Th3Fanbus narrating what happened as seen from inside our brain.]
6 other reviewers have added comments, and all comments have been resolved.
What are all those your words about?
They're an incoherent late-night rant, apologies for having you (and everyone else) have to read them. That we misunderstood the meaning of "customer" did not help, either.
things being done in ways that do not align with flashrom's code review process
Could you please give a link to where said process described?
https://www.flashrom.org/Development_Guidelines#Reviews is the only thing there is, which is extremely short... Hmmm, maybe we extrapolated https://doc.coreboot.org/contributing/gerrit_guidelines.html to flashrom for lack of a better set of guidelines. In any case, there are definitely some "unwritten rules/guidelines" in flashrom which deserve to be written down.
Mini-dissertation about `rule` vs `guideline`: the term `rule` implies, in some way, obligation and/or compliance. This is in stark contrast with the term `guideline`, which is "softer" and a piece of advice to follow unless there's good reasons to do otherwise. In other words, a `guideline` lends itself better to be adapted in the future. We prefer the term `guideline` because of this.
The thing is that our code review process is not clearly documented anywhere, but it should be. Each flashrom developer has a picture in their head of what the process should be, but what we need is one shared picture which is discussed, approved by everyone and documented. I really think this is important, and raised the topic earlier this year on the meeting, and then in April I started a thread on the mailing list, “Code Reviews guidelines discussion” here is the link https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/message/5WXS... You are very welcome to respond to the thread!
Huh, we completely missed that, thank you very much! We support your initiative and we'll reply to your email. If we (flashrom community) reach some form of consensus, we can then expand the information in our Wiki.
A little bit of info about reviews is in our Development Guidelines on the website, but it’s very generic and not sufficient.
Let’s admit that flashrom has no clearly documented code review process, and let’s work together and create and document one. Meanwhile, I don’t think it’s fair to accuse people of not aligning with the process which is not documented. Believe me, contributors, especially new ones, love following clearly defined processes for the project they have joined.
We fully agree, apologies if this felt like a personal accusation. Although things were different several years ago (before you arrived), it's true that things are changing for the better now. And we're glad to have you here. We will work towards having the code review process documented.
I am also interested in what exactly in your opinion was wrong in the process of CB:49643 (let’s collect some tribal knowledge at least). If you choose to answer this question, please answer it together with the question, why you haven’t raised any concerns during 1.5 years while the patch was in review.
Looking at it again, we can't find unresolved comments about the API that we thought were somewhere in there... Did we even *read* those comments? Again, apologies for the noise.
[As to why we said nothing for 1.5 years, you probably already read our reply: it's a few paragraphs earlier.]
This reminds us of one thing we encountered several times (not just in flashrom) that is very confusing: reviewing patches introducing APIs and similar functionality meant to be used in follow-up patches, but we can't find these follow-up patches. Even if the follow-ups are WIP or even just for demonstration purposes, they're useful to understand how things work together. But we just came up with a very good solution for most of these things: write these "examples of use-cases" as tests!
Also speaking about Development guidelines which is our little bit of documentation. I have read them many times this year, and I noticed some clashes with what you are saying. https://www.flashrom.org/Development_Guidelines
In most open-source projects, getting one's code reviewed takes an unspecified amount of time
This would not fit into our Development guidelines that say:
All contributions should receive at least a preliminary review within one week of submission by some flashrom developer (if that doesn't happen in time, please be patient)
Sorry for the ambiguity. By "reviewed" we meant "reviewed and approved". Most of the ranting regarding speed of reviews was under the mistaken assumption that the customers were paying someone to do it. We agree that not giving a first review in a reasonable timeframe will upset new contributors, but it's true that getting certain things submitted can take a significant amount of time.
Also you said
flashrom isn't the kind of project where "regressions in the master branch are fine, people should only use the releases"
However Development guidelines say very differently:
As usual there is no quality promise for the state of the code on the master branch. Even though we will try to keep the regression rate as low as possible, the main purpose of the branch is to merge new commits and make them available to a broader audience for testing.
Hmmm, to be honest we don't have much experience with other open-source projects. However, we found some people (most of them in coreboot, with a corporate background) that were surprised by the "commits landing into the master branch are supposed to pass build-tests individually". Our guess is that merge commits make this more of a non-issue.
But yes, we agree that flashrom's master branch comes with no guarantees. We should still try not to introduce regressions, though. The bug with progress state numbers going down in some cases does not qualify as a regression: it doesn't prevent using other flashrom features. Contrast this with CB:66659 which resulted in flashrom crashing when invoked with no parameters: this breaks existing functionality and needs to be dealt with ASAP. [This should probably be documented somewhere in the Wiki.]
I see that the word “customer” has sent your thoughts in a completely wrong direction (you even said it’s “my customer”). “User” is a better word. Let’s say user from now, and I meant flashrom user. Nico got it right. The User here is fwupd project. Both projects are FOSS so of course there are no payments involved.
All right, thank you for the clarification. Again, apologies if this misunderstanding made this conversation bitterer than it should've been.
In any case, collaboration between fwupd and flashrom started before I joined. So you know the story better than me, you’ve been working on flashrom already at that time.
the result is no longer flashrom
Speaking about what flashrom is and what it is not. I think flashrom is what we collectively create, we as the current group of people who are giving their time, skills, effort and a piece of their heart into running the project. Specifically on your statement of “if such and such mistakes have been done, then it’s not flashrom anymore”. It is still flashrom, with a room for improvement.
*audible blinking; then, the sound of a facepalm followed by noises of someone falling of a chair*
There aren't enough swear-words in the English language, so now we'll have to call ourselves "perkeleen vittupää" (a very not-nice expression in Finnish) just to express our disgust and frustration with our own crap. [This is derived from what Angry Linus Torvalds once wrote on LKML. We don't have any words to describe how infinitely stupid that statement is.]
IMO it is the responsibility of the group of maintainers, or “core developers” to analyse the mistakes and think how to prevent them from happening again. Flashrom is what we create as a result of our actions and words.
Let's do this.
You mentioned “historically”, by the way flashrom is ~20years old. Neither you nor me are here for that long. We can ask the founders what were their vision and goals, and whether flashrom now is still flashrom?
Sounds good! It would be interesting to (maybe?) chat about this in a voice call or similar.
TL;DR: Let's work on finding solutions together.
Your tldr is amazing! Yes please! Yes let’s do it, especially given that “let’s work together” is what I am repeating all this year! I agree, of course. But I want to ask you to speak honestly, without attempts to mislead the readers. Alright?
*energetic nodding*
Definitely! It will be very easy now, if not trivial: that nonsensical rant purged all the nastiness buried inside us that precluded proper collaboration and improvement. Thanks for your understanding and forgiveness.
So:
For code reviews process/guidelines here is the thread, please share your ideas: https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/message/5WXS...
For the bug in showing progress in libflashrom, here it is: https://ticket.coreboot.org/issues/390
Will take a look, thank you.
[Apparently this response is so long that it exceeds Gerrit's limit... Will send part 2 right afterwards.]