Hello Flashrom community,
Recently, we (Anastasia and Felix, your Org Admins) were looking into the Flashrom Dev Guidelines page https://www.flashrom.org/Development_Guidelines#Patch_submission with the main goal to prepare the page for GSoC. Very soon (7th March) the results are announced, and in case Flashrom is accepted, we will suddenly have lots of people (potential contributors) looking into our Dev Guidelines! :)
We have a suggestion on how to improve the guidelines. Specifically, this is about the “Patch Submission” section (not the whole guidelines). There are few most important points to address:
1.
Update the links, so that they don’t point to instructions from the old coreboot wiki (which explains how to clone coreboot repo). 2.
State very clearly that we have a strong preference for Gerrit code reviews (not mailing list reviews). 3.
Explain that we don’t look at pull requests on GitHub.
The updated version of the “Patch Submission” section that we are suggesting is below. Comments/feedback is welcome, the wording can be changed/improved, as long as the three main points are addressed.
Unchanged sections are marked as such.
Thank you so much!
----------------------------------------------------------------- *Patch Submission* Coding style (unchanged)
Flashrom generally follows Linux kernel style https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst .
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
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.
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. 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:
1.
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 2.
For small patches, with the reviewer’s agreement, there is an option to send via our mailing list https://www.flashrom.org/Contact#Mailing_List. 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.
Our guidelines borrow heavily from the coreboot development guidelines https://doc.coreboot.org/contributing/index.html, and most of them apply 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@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_releases/2004/2004_05_24_dco.html do. Add a note such as
Signed-off-by: Random J Developer random@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_releases/2004/2004_05_24_dco.html 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 GerritSetting up a Gerrit accountAll 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.
1.
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”.
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”. 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
1.
Check out a new local branch that tracks origin/master: git checkout -b <branch_name> origin/master 2.
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.
-
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.