Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/75906?usp=email )
Change subject: doc: Add new Development guidelines ......................................................................
doc: Add new Development guidelines
Change-Id: I7fe9ab2e27fead8e795138294219b11240f15928 Signed-off-by: Anastasia Klimchuk aklm@flashrom.org --- M doc/contact.rst A doc/dev_guide/development_guidelines.rst M doc/dev_guide/index.rst 3 files changed, 296 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/75906/1
diff --git a/doc/contact.rst b/doc/contact.rst index dc89063..220d73f 100644 --- a/doc/contact.rst +++ b/doc/contact.rst @@ -3,6 +3,8 @@ .. The extra = is needed to prevent git from throwing a `leftover conflict marker` error when commiting.
+.. _mailing list: + Mailing List ------------ Flashrom related mails are welcome on the flashrom mailing list at `flashrom@flashrom.org mailto:flashrom@flashrom.org`_. diff --git a/doc/dev_guide/development_guidelines.rst b/doc/dev_guide/development_guidelines.rst new file mode 100644 index 0000000..e9ed165 --- /dev/null +++ b/doc/dev_guide/development_guidelines.rst @@ -0,0 +1,293 @@ +======================= +Development Guidelilnes +======================= + +Intro, is there anything useful to say? + +Set up the git repository and dev environment +============================================= + +#. Clone git repository + * If using https: :code:`git clone "https://review.coreboot.org/flashrom%22%60 + * If using ssh: :code:`git clone "ssh://aklm@review.coreboot.org:29418/flashrom"` +#. Follow the build guidelines to install dependencies :doc:`building_from_source` +#. Install Git hooks: :code:`./util/git-hooks/install.sh` +#. Add upstream as a remote: + * If using https: :code:`git remote add -f upstream https://review.coreboot.org/flashrom%60 + * If using ssh: :code:`git remote add -f upstream ssh://<gerrit_username>@review.coreboot.org:29418/flashrom` +#. Check out a new local branch that tracks :code:`upstream/master`: :code:`git checkout -b <branch_name> upstream/master` +#. Every patch is required to be signed-off (see also :ref:`sign-off`). + Set up your ``user.name`` and ``user.email`` in git config, and don't forget + to ``-s`` when creating a commit. +#. See also build guidelines :doc:`building_from_source` and `git docs https://git-scm.com/doc`_ + +Creating your patch +=================== + +Create a commit and sign-off ``git commit -s``. + +Commit message +-------------- + +Commit messages shall have the following format: + +.. code-block:: + + <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. + + Finally, follow the sign-off procedure to add your sign-off! + + Signed-off-by: Your Name your@domain + +Commit message should include: + +* Commit title +* Commit description: explain what the patch is doing, or what it is fixing. +* Testing information: how did you test the patch. +* Signed-off-by line (see below :ref:`sign-off`) +* If applicable, link to the ticket in the bugtracker `https://ticket.coreboot.org/projects/flashrom`_ +* Change-Id for Gerrit. If commit message doesn't have Change-Id, you forgot to install git hooks. + +.. _sign-off: + +Sign-off procedure +^^^^^^^^^^^^^^^^^^ + +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 + +.. code-block:: + + 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 known identity in the ``Signed-off-by`` line and in any +copyright notices you add. Anonymous patches 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/`_. + +Coding style +------------ + +Flashrom generally follows Linux kernel style: +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... + +The notable exception is 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. + + +Testing a patch +--------------- + +We expect the patch to be approproately tested by the patch owner. +Please add the testing information in commit message, for example that could be some of these: +programmer you were using, programmer params, chip, OS, operations you were running +(read/write/erase/verify), and anything else that is relevant. + +Working with Gerrit +=================== + +All of the patches and code reviews need to go via +`Gerrit on review.coreboot.org https://review.coreboot.org/#/q/project:flashrom`_. +While there it is technically possible to send a patch to the mailing list, that patch +still needs to be pushed to Gerrit by someone. We treat patches on the mailing list as a very +exceptional situation. Normal process is to push a patch to Gerrit. +Please read below for instructions and check `official Gerrit documentation https://gerrit-review.googlesource.com/Documentation/`_. + +Creating an account +--------------------- + +#. Go to https://review.coreboot.org/login and sign in using the credentials of + your choice. +#. Edit your settings by clicking on the gear icon in the upper right corner. +#. Set your Gerrit username (this may be the different from the username of an + external account you log in with). +#. Add an e-mail address so that Gerrit can send notifications to you about + your patch. +#. Upload an SSH public key, or click the button to generate an HTTPS password. + +Pushing a patch +--------------- + +To push patch to Gerrit, use the follow command: :code:`git push upstream 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. + +There is an option to add a topic to the patch. For one-off standalone patches this +is not necessary. However if your patch is a part of a larger effort, especially if the +work involves multiple contributors, it can be useful to mark that the patch belongs +to a certain topic. + +Adding a topic makes it easy to search "all the patches by the topic", even if the patches +have been authored by multiple people. + +To add a topic, push with the command: `git push upstream HEAD:refs/for/master%topic=example_topic`. +Alternatively, you can add a topic from a Gerrit UI after the patch in pushed +(on the top-left section) of patch UI. + +Adding reviewers to the patch +----------------------------- + +After pushing the patch, ideally try to make sure there are some reviewers added to your patch. + +flashrom has MAINTAINERS file with people registered for some areas of the code. People who +are in MAINTAINERS file will be automatically added as reviewers if the patch touches that +area. However, not all areas are covered in the file, and it is possible that for the patch you +sent no one is added automatically. + +If you know someone in the dev community who can help with patch review, add the person(s) you know. + +In general, it's a good idea to add someone who has a knowledge of whatever the patch is doing, +even if the person has not been added automatically. + +If you are new, and don't know anyone, and no one has been added automatically: you can add Anastasia Klimchuk as a reviewer. + +Going through code reviews +-------------------------- + +You will likely get some comments on your patch, and you will need to fix the comments. +After doing the work locally, amend you commit ``git commit --amend -s`` and push to Gerrit again. +Check that Change-Id in commit message stays the same. This way Gerrit knows your change belongs +to the same patch, and will upload new change as new patchset for the same patch. + +After uploading the work, go through comments and respond to them. Mark as Done the ones you done +and mark them as resolved. If there is something that is impossible to do, or maybe you have more questions, +or maybe you are not sure what you are asked about: respond to a comment **without marking it as resolved**. + +It is completely fine to ask a clarifying questions if you don't understand what the comment is asking you to do. +If is also fine to explain why a comment can't be done, if you think it can't be done. + +The patch is ready when all comments are resolved and the patch is approved (has +2 vote). + +Mirrors +======== + +The only official repository is https://review.coreboot.org/flashrom and GitHub and GitLab are just mirrors. +**Reviewers do not look at pull requests.** +Even if pull requests were automatically transferred to Gerrit, +requirements such as :ref:`sign-off` 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 https://review.coreboot.org/login`_. + +Code review process +=================== + +Our guidelines borrow heavily from `coreboot coding style +https://doc.coreboot.org/contributing/coding_style.html`_ and `Gerrit +guidelines https://doc.coreboot.org/contributing/gerrit_guidelines.html`_, +and most of them apply to flashrom as well. The really important part is about +the :ref:`sign-off procedure <sign-off>`. + +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, please use it. + +The **patch reviews** may take some time, but please don't get discouraged. +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 :ref:`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. + +Initial review should include a broad indication of acceptance or rejection of +the idea/rationale/motivation or the implementation + +In general, reviews should focus on the architectural changes and things that +affect flashrom as a whole. This includes (but is by no means limited to) +changes in APIs and types, safety, portability, extensibility, and +maintainability. The purpose of reviews is not to create perfect patches, but +to steer development in the right direction and produce consensus within the +community. The goal of each patch should be to improve the state of the project +- it does not need to fix all problems of the respective field perfectly. + +.. note:: + + New contributors may need more detailed advices and should be told about + minor issues like formatting problems more precisely. The result of a review + should either be an accepted patch or a guideline how the existing code + should be changed to be eventually accepted. + +If you sent a patch and later lost interest or no longer have time to follow up on code review, +please add a comment saying so. Then, if any of our maintainers are interested in finishing the work, +they can take over the patch. + +Merging patches +=============== + +Merging to branches is limited to the "flashrom developers" group on Gerrit. +The following rules apply, some are already +enforced by Gerrit: + + * Every commit has to be reviewed and needs at least one +2 that was not given + by the commit's author. + * Except, if a commit is authored by more than one person, each author may +2 + the other author's changes. + * flashrom developers are people from literally all around the planet, and various + timezones, so we usually wait for 3 days after the patch is fully approved just in case + of last minute concerns from all timezones. + * In the case of emergency, merging should not take place within less than 24 hours after the review + started (i.e. the first message by a reviewer on Gerrit). + * Finally, before hitting Submit, one is reponsible to check that all comments + have been addressed, especially if there was a negative review (-1). + +Bugtracker +========== + +We have a bugtracker on `https://ticket.coreboot.org/projects/flashrom`_. +Anyone can view tickets, but to be able to create/update/assign tickets you need an account. diff --git a/doc/dev_guide/index.rst b/doc/dev_guide/index.rst index b540127..d6aa30d 100644 --- a/doc/dev_guide/index.rst +++ b/doc/dev_guide/index.rst @@ -6,3 +6,4 @@
building_from_source building_with_make + development_guidelines