Hi Nico,
Thank you so much for reviewing! Sorry for this HTML mess, I was thinking of making things easier to read… but the end result seems to be the opposite, sorry!
I went through all your comments, and for the first step I took all the pieces that you agreed with. So that was in total 4 small changes, which I will list below. I did only those 4 changes, I didn’t move sections around, and didn’t remove anything. It would be great to continue improvements on Dev guidelines, but I see that the rest of the suggestions needs more discussion. I will send another email later, with a plaintext version, hopefully that would be easier to read.
Here are the changes I made: 1. Update text for GitHub section (the section itself stays where it was). I kept the wording “reviewers do not look at pull requests”, because I don’t think anyone looks at them now? Diffs1 https://www.flashrom.org/index.php?title=Development_Guidelines&type=rev...
2. Strong preference for Gerrit code reviews p.1 of “Sending a patch”. Diffs2 https://www.flashrom.org/index.php?title=Development_Guidelines&type=rev...
3. Updating a text of mailing list reviews p.2 of “Sending a patch”. Diffs3 https://www.flashrom.org/index.php?title=Development_Guidelines&type=rev...
4. Update the links referring to coreboot guidelines. You suggested that we maybe can remove that paragraph, but I was not sure, so as a first step I decided to try to update the links (so that they point to a new website). Diffs4 https://www.flashrom.org/index.php?title=Development_Guidelines&type=rev...
Thank you so much again! I will send another email with remaining suggestions in plaintext, maybe in a few days.
Anastasia.
On Mon, Mar 7, 2022 at 12:42 AM Nico Huber nico.h@gmx.de wrote:
Hi Anastasia,
thank you for getting this started.
For the mailing list, especially reviews, text-only messages (i.e. no HTML) are preferred. If one would export the wiki source (not sure if there is a better way to do it than to copy-paste into a text editor), and after breaking long lines, it can also be run through `diff` quite nicely. Not 100% sure if it would have helped a lot here, moving things around can create rather confusing diffs.
I tried to comment as best as I could inline on what was left of the HTML.
Nico
On 06.03.22 05:04, Anastasia Klimchuk wrote:
*Patch Submission* Coding style (unchanged)
Flashrom generally follows Linux kernel style <
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
.
The notable exception is the line length limit. Our guidelines are:
80-columns soft limit for most code and comments. This is to encourage simple design and concise naming.
112-columns hard limit. Use this to reduce line breaks in cases where they harm grep-ability or overall readability, such as print
statements and
function signatures. Don't abuse this for long variable/function
names or
deep nesting.
Tables are the only exception to the hard limit and may be as long as needed for practical purposes.
GitHub
Why start with GitHub if it's not the preferred way?
The official Flashrom mirror on GitHub is: https://github.com/flashrom/flashrom
Importantly, GitHub repo is only a mirror, so all changes must go through Gerrit on review.coreboot.org in order to be merged, even small patches such as adding support for a flash chip. For this reason, reviewers do
not
look at pull requests. Even if pull requests were automatically
transferred
to Gerrit, requirements such as the #Sign-off Procedure https://www.flashrom.org/Development_Guidelines#Sign-off_Procedure
still
present a problem.
Change seems ok to me. Can't tell if really nobody is looking at PRs, though.
The quickest and best way to get your patch reviewed and merged is by sending it to review.coreboot.org. Conveniently, you can use your
GitHub,
GitLab or Google account as an OAuth2 login method. Please continue reading, the instructions are below.
Please add a link.
Preparing a patch
Before sending a patch for review, put your Signed-off-by line https://www.flashrom.org/Development_Guidelines#Sign-off_Procedure in
the
commit message.
Currently there are two ways to send patches for review:
Our strong preference, especially for large patches, is via Gerrit on review.coreboot.org <https://review.coreboot.org/#/q/project:flashrom , i.e. git push origin HEAD:refs/for/master
SGTM
For small patches, with the reviewer’s agreement, there is an option
to
How should one get the agreement before sending the patch?
send via our mailing list <
https://www.flashrom.org/Contact#Mailing_List%3E.
When sending a patch via the mailing list, send it in-line instead of
as an
attachment so that reviewers can easily comment on specific parts of
it.
However, eventually the reviewer will need to push patch to gerrit
anyway.
*G*errit with upper-case G, please.
Our guidelines borrow heavily from the coreboot development guidelines https://doc.coreboot.org/contributing/index.html, and most of them
apply
That link seems too generic, it also points to coreboot's project ideas and GSoC page. If there is no good link, maybe it's time to just drop this paragraph?
to flashrom as well. The really important part is about the #Sign-off Procedure https://www.flashrom.org/Development_Guidelines#Sign-off_Procedure.
We try to reuse as much code as possible and create new files only if absolutely needed, so if you find a function somewhere in the tree which already does what you want (even if it is for a totally different chip), please use it. See also Command set secrets https://www.flashrom.org/Random_notes#Command_set_secrets.
The patch reviews may sound harsh, but please don't get discouraged. We
try
to merge simple patches after one or two iterations and complicated ones
as
soon as possible, but we have quite high standards regarding code
quality.
If you introduce new features (not flash chips, but stuff like partial programming, support for new external programmers, voltage handling, etc) please discuss your plans on the mailing list https://www.flashrom.org/Contact#Mailing_List first. That way, we can avoid duplicated work and know about how flashrom internals need to be adjusted and you avoid frustration if there is some disagreement about
the
design.
For patches that modify convoluted tables like struct flashchip
flashchips[]
in flashchips.c it may make sense to increase the lines of context to include enough information directly in the patch for reviewers (for
example
to include the chip names when changing other parameters like .voltage).
To
do this with git use git format-patch -U5 where 5 is an example for the number of lines of context you want.
Commit message (unchanged)
Commit messages shall have the following format:
<component>: Short description (up to 72 characters)
This is a long description. Max width of each line in the description
is 72 characters. It is separated from the summary by a blank line. You
may skip the long description if the short description is sufficient,
for example "flashchips: Add FOO25Q128" to add FOO25Q128 chip support.
You may have multiple paragraphs in the long description, but please
do not write a novel here. For non-trivial changes you must explain
what your patch does, why, and how it was tested.
TEST=tests that you performed
Finally, follow the #Sign-off Procedure https://www.flashrom.org/Development_Guidelines#Sign-off_Procedure to
add
your sign-off!
Signed-off-by: Your Name <your(a)domain>
Sign-off Procedure (unchanged)
We employ a similar sign-off procedure as the Linux kernel developers <
http://web.archive.org/web/20070306195036/http://osdlab.org/newsroom/press_r...
do. Add a note such as
Signed-off-by: Random J Developer <random(a)developer.example.org>
to your email/patch if you agree with the Developer's Certificate of
Origin
1.1 printed below. Read this post on the LKML https://lkml.org/lkml/2004/5/23/10 for rationale (spoiler: SCO).
You must use your real name in the Signed-off-by line and in any
copyright
notices you add. Patches without an associated real name lack provenance and cannot be committed!
Developer's Certificate of Origin 1.1:
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I have
the right to submit it under the open source license indicated in the
file;
or
(b) The contribution is based upon previous work that, to the best of my
knowledge, is covered under an appropriate open source license and I have the
right under that license to submit that work with modifications, whether created
in whole or in part by me, under the same open source license (unless I
am
permitted to submit under a different license), as indicated in the
file; or
(c) The contribution was provided directly to me by some other person who
certified (a), (b) or (c) and I have not modified it; and
(d) In the case of each of (a), (b), or (c), I understand and agree that
this project and the contribution are public and that a record of the contribution
(including all personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with this project or the
open source license indicated in the file.
Note: The Developer's Certificate of Origin 1.1 <
http://web.archive.org/web/20070306195036/http://osdlab.org/newsroom/press_r...
is licensed under the terms of the Creative Commons
Attribution-ShareAlike
2.5 License http://creativecommons.org/licenses/by-sa/2.5/.
Submitting a patch using Gerrit
Sounds like a very good idea to make this guide more generic. I wonder if it shouldn't be on its own page, though? We should link to it of course.
Also, something like "Extra steps if you cloned from GitHub" might be nice so we don't have to drop these things.
Setting up a Gerrit account
All changes have to go through Gerrit on review.coreboot.org in order to be merged, even small patches such as adding support for a flash chip. Our Gerrit
supports
multiple authentication methods https://review.coreboot.org/login including OAUTH2, e.g. Google, GitHub or GitLab, or also OpenID.
Go to https://review.coreboot.org/login and sign in. 2.
Edit your settings by clicking on the gear icon in the upper right corner. 3.
Set your Gerrit username. 4.
Add an email address so that you receive notifications when others commented or reviewed your patch. 5.
Add a SSH public key to your account, or click the button to generate
an
HTTPS password.
Preparing your local repository
Open https://review.coreboot.org/admin/repos/flashrom and choose your desired method to clone the repository. Supported methods are HTTPS and SSH. The same method will also be used when you push your changes to
Gerrit
later.
Also, make sure to install the Change-Id hook. This generates a unique ID which is appended to your commit message. It is used by Gerrit to
identify
if a patch with the same ID exists, and if so it will add a new version
to
it called “patchset”.
Isn't this taken care of by `make`?
If you are just about getting a fresh copy of the flashrom repository,
then
you can use the command which you can find under “Clone with commit-msg hook”.
I never used this, might be new. How well does it work with our automatism?
If you have an existing copy of the repository and you need to install the hook afterwards, then you can run this command within your flashrom directory
mkdir -p .git/hooks && curl -Lo `git rev-parse --git-dir`/hooks/commit-msg https://review.coreboot.org/tools/hooks/commit-msg; chmod +x `git rev-parse --git-dir`/hooks/commit-msg)
Pushing your patch to Gerrit
Check out a new local branch that tracks origin/master: git checkout
-b
<branch_name> origin/master
Seems like something for the "Extra GitHub steps" category. Otherwise one would just want to checkout `master` right?
Do your changes 3.
Add your changes using `git add` and create a commit using `git commit -s` 4.
Push to gerrit: git push origin HEAD:refs/for/master.
NB. This might be a candidate for a tl;dr for experienced Git users.
If using HTTPS you will be prompted for the username and password you set in the Gerrit UI.
If successful, the Gerrit URL for your patch will be shown in the
output.