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=revision&diff=2513&oldid=2512 2. Strong preference for Gerrit code reviews p.1 of “Sending a patch”. Diffs2 https://www.flashrom.org/index.php?title=Development_Guidelines&type=revision&diff=2514&oldid=2513 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=revision&diff=2515&oldid=2514 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=revision&diff=2516&oldid=2515 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 <nic...@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/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: > > > > 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 > > SGTM > > > 2. > > > > 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. > > > > 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”. > > 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 > > > > 1. > > > > 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? > > > 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. > > 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. > > > > > > -- Anastasia.
_______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-le...@flashrom.org
