[S] Change in flashrom[master]: libflashrom: Allow getting the progress_state from the flashctx

[Angel Pons (Code Review)]

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:

(1 comment)

Patchset:

PS2: 
Hi,

Apologies if our comment^wrant sounds harsh and/or rude, our head hurts from too much confused head scratching. It is also quite late. We have noticed some problems about things being done in ways that do not align with flashrom's code review process and we would like to find solutions to these problems.

Also, clarification about using 1st person plural: it is because of recent discoveries regarding our mind. It does not refer to the flashrom project or any other individual involved or not with the project. Sorry if it sounds confusing, it is.
> I can tell you my thoughts. We have a customer for this feature (fwupd), the feature already introduced in the previous patch CB:49643 , and this patch just makes it available for the customer. So we have someone who is waiting this feature, and if we merge this patch they will start using it straight away. Which means, if there are bugs, they will discover them and fix them, because they need the feature to work.

Can't said customer just apply patches from Gerrit? Or do they not want to do that because having to fetch changes from Gerrit makes them "less good" for some reason? If patches from Gerrit get rubber-stamped and submitted without a proper review, the code will still be the same thing one would get from Gerrit, but any mistakes (bugs in the code, but also dumb typos) will be immortalized forever in git history. And flashrom isn't the kind of project where "regressions in the master branch are fine, people should only use the releases". It's precisely this kind of attitude that resulted in us being unable to make a new release for years: the master branch got swarmed by many patches that couldn't be reviewed thoroughly, and code quality (there was a lot of new code whose quality was unknown) of the master branch dropped below the expected quality levels to release a new flashrom version.

The point of code review is to have other people look at the code to find problems and suggest improvements; it's to collaborate. In most open-source projects, getting one's code reviewed takes an unspecified amount of time because the people doing code reviews do so voluntarily, and most do not get paid for doing so. Trying to "expedite" things by throwing more people into the project usually doesn't work: these people still need to learn how to contribute to the project to be trusted with submit rights, and if the process of gaining the trust of existing contributors is bypassed for some reason, there will be conflicts and everyone involved will feel miserable about it. Moreover, if our understanding is correct, flashrom has always been a rather slow-paced project, mainly because the consequences of regressions could result in many users ending up with bricked hardware.

Another very important thing about code reviews is that they allow knowledge sharing, i.e. teaching things to others. When we started contributing to coreboot, we had no clue about things, but others welcomed us helped us get started, and we feel that our duty is to give back this "welcome wagon" to other newcomers.

If the customer wants to have experimental features available for testing ASAP, please have them pull patches from Gerrit or provide a branch and/or a downstream fork. We could consider bringing back the "staging" branch or something like this to mess around with work-in-progress features without compromising the stability of the master branch.

However, if the customer wants new features with quality on par with existing code, it is fundamental to follow flashrom's review process. Otherwise, the result is no longer flashrom: it may resemble flashrom, but it may not be the same, so it's hard to make new releases due to the fear that users may notice the difference (imagine Coca-Cola that ends up tasting like Pepsi, or like bubbly rainwater).
> So, what's the plan: it would be awesome to keep the feature and fix the bug. The feature is already merged and this patch makes it available to external users, it makes it available to the whole world in other words. When the whole world is using the feature, it very important to take the responsibility for known issues.

If a flashrom feature is being used by the whole world, it better be something that has been tried and tested. Personally, we do not want to be held liable for issues regarding a flashrom feature that was not properly reviewed.

We did not take a look at CB:49643 in detail, but we saw that others commented about API design decisions. Fixing such fundamental issues is best achieved by reverting the problematic implementation and re-introducing the feature so that it can be reviewed using Gerrit as it was meant to be used. Even something simple like CB:66659 was much easier to fix by undoing it and re-doing it again
> Is it possible to merge this patch before the bug is fixed?

Sorry, we'd rather not. "Upstream first" does not mean "test/review later". The progress percentage glitch is not too bad, but it indicates something else might be wrong with the code. If your customer really wants the buggy feature anyway, they should consider having their own downstream fork of flashrom to deal with things like this. Having a downstream fork is not mutually exclusive with "upstream first": the feature is developed upstream, and cherry-picked downstream for use.

While we won't block this change, we'd appreciate if you (or anyone else reading this) could share your thoughts on how to satisfy both your customer and the flashrom project's needs. We're pretty sure there's a way.
> Not ideal, but yes it is possible if the bug is clearly assigned, accepted, estimated and started.

The thing is, we probably wouldn't need to treat this as a bug had things been tested / reviewed beforehand. The procedures to deal with bugs seem significantly less efficient than the review process, although it might be because we're much more familiar with review processes than with bug report processes. Reverting things is not inherently bad: we've approved or even created reverts of our own commits, because it was the most sensible thing to do. It's admitting that mistakes were made and prepares the code to try again.

TL;DR: Let's work on finding solutions together.

Best regards,
Angel