Attention is currently required from: Felix Singer, Nico Huber, Richard Hughes, Edward O'Callaghan, Angel Pons, Daniel Campello.
Anastasia Klimchuk 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:
(1 comment)
Patchset:
PS2: Hello Angel,
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.
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.
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 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. 6 other reviewers have added comments, and all comments have been resolved.
What are all those your words about?
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?
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!
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.
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.
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)
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.
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. 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. 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.
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?
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?
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
Also you had a remark about code quality which has dropped below expected levels in your opinion. Code quality is an important subject too, if you could start a thread on the mailing list about it? You could explain how you measure it, what are metrics? What is expected level? (especially in light of Development guidelines saying “As usual there is no quality promise for the state of the code on the master branch”) Ideally if you give examples of quality drops so that we can fix those and prevent from happening again.
Thank you Angel, let’s work together.