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/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
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. 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_releases/2004/2004_05_24_dco.html 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_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 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.