[S] Change in flashrom[master]: libflashrom: Allow getting the progress_state from the flashctx
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.
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.
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.
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!
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.
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.
*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.
*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.
Will take a look, thank you.
[Apparently this response is so long that it exceeds Gerrit's limit... Will send part 2 right afterwards.]
-
Angel Pons (Code Review)