|
|
Line 1: |
Line 1: |
− | This set of guidelines is intended to help both the developer and reviewers of | + | This page has moved to the [https://docs.yoctoproject.org/dev/contributor-guide/submit-changes.html Yocto Documentation Contributors Guide] |
− | changes determine reasonable patch headers and change commit messages. Often
| |
− | when working with the code, you forget that not everyone is as familiar with the
| |
− | problem and/or fix as you are. Often the next person in the code doesn't
| |
− | understand what or why something is done so they quickly look at patch header
| |
− | and commit messages. Unless these messages are clear it will be difficult to
| |
− | understand the relevance of a given change and how future changes may impact
| |
− | previous decisions.
| |
− | | |
− | This policy refers both to patches that are being applied by recipes as well as
| |
− | commit messages that are part of the source control system, usually git. A patch
| |
− | file needs a header in order to describe the specific changes that are made
| |
− | within that patch, while a commit message describes one or more changes to the
| |
− | overall project or system. Both the patch headers and commit messages require
| |
− | the same attention and basic details, however the purposes of the messages are
| |
− | slightly different. A commit message documents all of the changes made as part
| |
− | of a commit, while a patch header documents items specific to a single patch. In
| |
− | many cases the patch header can also be used as the commit message.
| |
− | | |
− | This policy does not cover the testing of the changes, the technical criteria
| |
− | for accepting a patch, nor the style requirements for the technical changes. See
| |
− | the [http://www.openembedded.org/wiki/Styleguide Styleguide] for more information on style requirements. | |
− | | |
− | By following these guidelines we will have a better record of the problems and
| |
− | solutions made over the course of development. It will also help establish a
| |
− | clear provenance of all of the code and changes.
| |
− | | |
− | Draft version, changes from last approved version:
| |
− | * Revised General Information section to correct broken link
| |
− | * Clarify the ''Developer's Certificate of Origin''
| |
− | | |
− | == General Information ==
| |
− | | |
− | While specific to the Linux kernel development, the following could also be
| |
− | considered a general guide for any Open Source development:
| |
− | | |
− | https://www.linux.com/publications/how-participate-linux-community | |
− | | |
− | Many of the guidelines in this document are related to the items referenced in
| |
− | the link above.
| |
− | | |
− | Pay particular attention to section 5.3 that talks about patch preparation.
| |
− | The key thing to remember is to break up your changes into logical sections.
| |
− | Otherwise you run the risk of not being able to even explain the purpose of a
| |
− | change in the patch headers!
| |
− | | |
− | In addition section 5.4 explains the purpose of the Signed-off-by: tag line. The
| |
− | signed-off-by: tag line is the ''Developer's Certificate of Origin''. The full text
| |
− | of which can be found in the Linux kernel's Documentation/SubmittingPatches.
| |
− | | |
− | Pay particular attention to section 11 of
| |
− | | |
− | https://www.kernel.org/doc/html/latest/process/submitting-patches.html
| |
− | | |
− | Due to the importance of the ''Developer's Certificate of Origin'', part of section 11
| |
− | is copied below:
| |
− | | |
− | The sign-off is a simple line at the end of the explanation for the
| |
− | patch, which certifies that you wrote it or otherwise have the right to
| |
− | pass it on as an open-source patch. The rules are pretty simple: if you
| |
− | can certify the below:
| |
− |
| |
− | 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.
| |
− |
| |
− | (d) 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(s) involved.
| |
− |
| |
− | then you just add a line saying::
| |
− |
| |
− | Signed-off-by: Random J Developer <random@developer.example.org>
| |
− |
| |
− | using your real name (sorry, no pseudonyms or anonymous contributions.)
| |
− | | |
− | == Patch Headers and Commit Messages ==
| |
− | | |
− | There seems to always be a question or two surrounding what a person
| |
− | should put in a patch header, or commit message.
| |
− | | |
− | The general rules always apply, those being that there is a single line
| |
− | short log or summary of the change (think of this as the subject when the patch
| |
− | is e-mailed), and then the more detailed long log, and closure with tag lines
| |
− | such as "Signed-off-by:".
| |
− | | |
− | If the bug relates to a bug in the Yocto Project Bugzilla (https://bugzilla.yoctoproject.org/) then a reference to that bugzilla entry should be added to the details in the form:
| |
− | | |
− | [YOCTO #12345]
| |
− | | |
− | where 12345 is the number of the bug being referenced. The commit message should be understandable in its own right without needing to look at the bug.
| |
− | | |
− | It is useful to include a version number in the [PATCH] prefix of the subject line, e.g. [PATCH v2] so that its known to be a new version of an earlier patch. It can also be useful to include information about what changed in the new version. This should either be in the cover letter of the series (if one is present) or be done under the detailed commit message and after a "---" separator line. This will mean git will discard the revision information when the patch is applied. This is because the history of the patch revisions is not usually useful to the information being stored against the change. If it is useful information, it should be in the commit message instead.
| |
− | | |
− | == New Development ==
| |
− | | |
− | A minimal patch or commit message would be of the format:
| |
− | | |
− | Short log / Statement of what needed to be changed.
| |
− |
| |
− | (Optional pointers to external resources, such as defect tracking)
| |
− |
| |
− | The intent of your change.
| |
− |
| |
− | (Optional, if it's not clear from above) how your change resolves the
| |
− | issues in the first part.
| |
− |
| |
− | Tag line(s) at the end.
| |
− | | |
− | For example:
| |
− | | |
− | foobar: Adjusted the foo setting in bar
| |
− |
| |
− | When using foobar on systems with less than a gigabyte of RAM common
| |
− | usage patterns often result in an Out-of-memory condition causing
| |
− | slowdowns and unexpected application termination.
| |
− |
| |
− | Low-memory systems should continue to function without running into
| |
− | memory-starvation conditions with minimal cost to systems with more
| |
− | available memory. High-memory systems will be less able to use the
| |
− | full extent of the system, a dynamically tunable option may be best,
| |
− | long-term.
| |
− |
| |
− | The foo setting in bar was decreased from X to X-50% in order to
| |
− | ensure we don't exhaust all system memory with foobar threads.
| |
− |
| |
− | Signed-off-by: Joe Developer <joe.developer@example.com>
| |
− | | |
− | The minimal commit message is good for new code development and simple changes.
| |
− | | |
− | The single short log message indicates what needed to be changed. It should
| |
− | begin with an indicator as to the primary item changed by this work,
| |
− | followed by a short summary of the change. In the above case we're indicating
| |
− | that we've changed the "foobar" item, by "adjusting the foo setting in bar".
| |
− | | |
− | The single short log message is analogous to the git "commit summary". While no
| |
− | maximum line length is specified by this policy, it is suggested that it remains
| |
− | under 78 characters wherever possible.
| |
− | | |
− | Optionally, you may include pointers to defects this change corrects. Unless
| |
− | the defect format is specified by the component you are modifying, it is
| |
− | suggested that you use a full URL to specify the reference to the defect
| |
− | information. Generally these pointers will precede any long description, but as
| |
− | an optional item it may be after the long description.
| |
− | | |
− | For example, you might refer to an open source defect in the RPM project:
| |
− | | |
− | rpm: Adjusted the foo setting in bar
| |
− |
| |
− | <nowiki>[RPM Ticket #65] -- http://rpm5.org/cvs/tktview?tn=65,5</nowiki>
| |
− |
| |
− | The foo setting in bar was decreased from X to X-50% in order to
| |
− | ensure we don't exhaust all system memory with foobar threads.
| |
− |
| |
− | Signed-off-by: Joe Developer <joe.developer@example.com>
| |
− | | |
− | You must then have a full description of the change. Specifying the intent of
| |
− | your change and if necessary how the change resolves the issue.
| |
− | | |
− | As mentioned above this is intended to answer the "what were you thinking"
| |
− | questions down the road and to know what other impacts are likely to
| |
− | result of the change needs to be reverted. It also allows for better
| |
− | solutions if someone comes along later and agrees with paragraph 1
| |
− | (the problem statement) and either disagrees with paragraph 2
| |
− | (the design) or paragraph 3 (the implementation).
| |
− | | |
− | Finally one or more tag lines should exist. Each developer responsible
| |
− | for working on the patch is responsible for adding a Signed-off-by: tag line.
| |
− | | |
− | It is not acceptable to have an empty or non-existent header, or just a single
| |
− | line message. The summary and description is required for all changes.
| |
− | | |
− | === Describing license changes ===
| |
− | | |
− | When you change the LICENSE or LIC_FILES_CHKSUM lines in the recipe, you need to
| |
− | briefly explain the reason for the change in via License-Update: tag. Quite often
| |
− | it's trivial, such as
| |
− | | |
− | License-Update: copyright years refreshed
| |
− | | |
− | but if the licensing terms themselves changed, do try to include a link to the upstream
| |
− | making/justifying that decision.
| |
− | | |
− | == Importing from Elsewhere ==
| |
− | | |
− | If you are importing work from somewhere else, such as a cherry-pick from
| |
− | another repository or a code patch pulled from a mailing list, the minimum patch
| |
− | header or commit message is not enough. It does not clearly establish the
| |
− | provenance of the code.
| |
− | | |
− | The following specifies the additional guidelines required for importing changes
| |
− | from elsewhere.
| |
− | | |
− | By default you should keep the original author's summary and description,
| |
− | along with any tag lines such as, "Signed-off-by:". If the original change that
| |
− | was imported does not have a summary and/or commit message from the original
| |
− | author, it is still your responsibility to add the summary and commit message to
| |
− | the patch header. Just as if you wrote the code, you should be able to clearly
| |
− | explain what the change does. It is also necessary to document the original
| |
− | author of the change. You should indicate the original author by simply stating
| |
− | "written by" or "posted to the ... mailing list by". You must not add a
| |
− | Signed-off-by: tag line with their name, only the original author may add their
| |
− | own Signed-off-by: tag line.
| |
− | | |
− | It is also required that the origin of the work be fully documented. The origin
| |
− | should be specified as part of the commit message in a way that an average
| |
− | developer would be able to track down the original code. URLs should reference
| |
− | original authoritative sources and not mirrors.
| |
− | | |
− | If changes were required to resolve conflicts, they should be documented as
| |
− | well. When incorporating a commit or patch from an external source, changes to
| |
− | the functionality not related to resolving conflicts should be made in a
| |
− | second commit or patch. This preserves the original external commit, and makes
| |
− | the modifications clearly visible, and prevents confusion over the author of the
| |
− | code.
| |
− | | |
− | Finally you must add a "Signed-off-by:" tag line to indicate that you worked
| |
− | with the patch.
| |
− | | |
− | | |
− | === Example: Importing from Elsewhere Unmodified ===
| |
− | | |
− | For example, if you were to pull in the patch from the above example unmodified:
| |
− | | |
− | foobar: Adjusted the foo setting in bar
| |
− |
| |
− | When using foobar on systems with less than a gigabyte of RAM common
| |
− | usage patterns often result in an Out-of-memory condition causing
| |
− | slowdowns and unexpected application termination.
| |
− |
| |
− | Low-memory systems should continue to function without running into
| |
− | memory-starvation conditions with minimal cost to systems with more
| |
− | available memory. High-memory systems will be less able to use the
| |
− | full extent of the system, a dynamically tunable option may be best,
| |
− | long-term.
| |
− |
| |
− | The foo setting in bar was decreased from X to X-50% in order to
| |
− | ensure we don't exhaust all system memory with foobar threads.
| |
− |
| |
− | Signed-off-by: Joe Developer <joe.developer@example.com>
| |
− |
| |
− | The patch was imported from the OpenEmbedded git server
| |
− | (<nowiki>git://git.openembedded.org/openembedded</nowiki>) as of commit id
| |
− | b65a0e0c84cf489bfa00d6aa6c48abc5a237100f.
| |
− |
| |
− | Signed-off-by: Your Name <your.name@openembedded.org>
| |
− | | |
− | The above example shows a clear way for a developer to find the original source
| |
− | of the change. It indicates that the OpenEmbedded developer simply integrated
| |
− | the change into the system without making any further modifications.
| |
− | | |
− | === Example: Importing from Elsewhere Modified ===
| |
− | | |
− | When importing a patch, occasionally changes have to be made in order to resolve
| |
− | conflicts. The following is an example of the commit message when the item needed
| |
− | to be modified:
| |
− | | |
− | foobar: Adjusted the foo setting in bar
| |
− |
| |
− | When using foobar on systems with less than a gigabyte of RAM common
| |
− | usage patterns often result in an Out-of-memory condition causing
| |
− | slowdowns and unexpected application termination.
| |
− |
| |
− | Low-memory systems should continue to function without running into
| |
− | memory-starvation conditions with minimal cost to systems with more
| |
− | available memory. High-memory systems will be less able to use the
| |
− | full extent of the system, a dynamically tunable option may be best,
| |
− | long-term.
| |
− |
| |
− | The foo setting in bar was decreased from X to X-50% in order to
| |
− | ensure we don't exhaust all system memory with foobar threads.
| |
− |
| |
− | Signed-off-by: Joe Developer <joe.developer@example.com>
| |
− |
| |
− | The patch was imported from the OpenEmbedded git server
| |
− | (<nowiki>git://git.openembedded.org/openembedded</nowiki>) as of commit id
| |
− | b65a0e0c84cf489bfa00d6aa6c48abc5a237100f.
| |
− |
| |
− | A previous change had modified the memory threshold calculation,
| |
− | causing a conflict. The conflict was resolved by preserving the
| |
− | memory threshold calculation from the existing implementation.
| |
− |
| |
− | Signed-off-by: Your Name <your.name@openembedded.org>
| |
− | | |
− | == Patch Header Recommendations: Upstream-Status ==
| |
− | | |
− | In order to keep track of patches and ultimately reduce the number of patches
| |
− | that are required to be maintained, we need to track the status of the patches
| |
− | that are created. The following items are specific to patches applied by recipes.
| |
− | | |
− | In addition to the items mentioned above, we also want to track if it's
| |
− | appropriate to get this particular patch into the upstream project. Since we
| |
− | want to track this information, we need a consistent tag and status indicator
| |
− | that can be searched.
| |
− | | |
− | While adding this tracking information to the patch headers is currently
| |
− | optional, it is highly recommended and some maintainers may require it. It is
| |
− | optional at this time so that it can be evaluated as to its usefulness over
| |
− | time. Existing patches will be updated with the tag as they are modified.
| |
− | | |
− | As mentioned in the above, be sure to include any URL to bug tracking, mailing
| |
− | lists or other reference material pertaining to the patch. Then add a new tag
| |
− | "Upstream-Status:" which contains one of the following items:
| |
− | | |
− | '''Pending'''
| |
− | - No determination has been made yet or not yet submitted to upstream
| |
− | | |
− | '''Submitted''' [where]
| |
− | - Submitted to upstream, waiting approval
| |
− | - Optionally include where it was submitted, such as the author, mailing
| |
− | list, etc.
| |
− | | |
− | '''Accepted'''
| |
− | - Accepted in upstream, expect it to be removed at next update, include
| |
− | expected version info
| |
− | | |
− | '''Backport'''
| |
− | - Backported from new upstream version, because we are at a fixed version,
| |
− | include upstream version info
| |
− | | |
− | '''Denied'''
| |
− | - Not accepted by upstream, include reason in patch
| |
− | | |
− | '''Inactive-Upstream [lastcommit: when (and/or) lastrelease: when]'''
| |
− | - The upstream is no longer available. This typically means a defunct project
| |
− | where no activity has happened for a long time - measured in years. To make
| |
− | that judgement, it is recommended to look at not only when the last release
| |
− | happened, but also when the last commit happened, and whether newly made bug
| |
− | reports and merge requests since that time receive no reaction. It is also
| |
− | recommended to add any relevant links to the commit message where the inactivity
| |
− | can be clearly seen.
| |
− | | |
− | '''Inappropriate [reason]'''
| |
− | - The patch is not appropriate for upstream, include a brief reason on the
| |
− | same line enclosed with []
| |
− | reason can be:
| |
− | not author (You are not the author and do not intend to upstream this,
| |
− | source must be listed in the comments)
| |
− | native
| |
− | licensing
| |
− | configuration
| |
− | enable feature
| |
− | disable feature
| |
− | bugfix (add bug URL here)
| |
− | embedded specific
| |
− | other (give details in comments)
| |
− | | |
− | The various "Inappropriate [reason]" status items are meant to indicate that the
| |
− | person responsible for adding this patch to the system does not intend to
| |
− | upstream the patch for a specific reason. Unless otherwise noted, another
| |
− | person could submit this patch to the upstream, if they do the status should be
| |
− | changed to "Submitted [where]", and an additional Signed-off-by: line added to
| |
− | the patch by the person claiming responsibility for upstreaming.
| |
− | | |
− | For example, if the patch has been submitted upstream:
| |
− | | |
− | rpm: Adjusted the foo setting in bar
| |
− |
| |
− | <nowiki>[RPM Ticket #65] -- http://rpm5.org/cvs/tktview?tn=65,5</nowiki>
| |
− |
| |
− | The foo setting in bar was decreased from X to X-50% in order to
| |
− | ensure we don't exhaust all system memory with foobar threads.
| |
− |
| |
− | Upstream-Status: Submitted [rpm5-devel@rpm5.org]
| |
− |
| |
− | Signed-off-by: Joe Developer <joe.developer@example.com>
| |
− | | |
− | A future commit can change the value to "Accepted" or "Denied" as appropriate.
| |
− | | |
− | === CVE Patches ===
| |
− | | |
− | In order to have a better control of vulnerabilities, patches that fix CVEs must contain the "CVE:" tag. This tag list all CVEs fixed by the patch. If more than one CVE is fixed, separate them using spaces.
| |
− | | |
− | ==== Example: CVE patch header ====
| |
− | | |
− | This should be the header of patch that fixes grub2 CVE-2015-8370:
| |
− | | |
− | grub2: Fix CVE-2015-8370
| |
− |
| |
− | <nowiki>[No upstream tracking] -- https://bugzilla.redhat.com/show_bug.cgi?id=1286966</nowiki>
| |
− |
| |
− | Back to 28; Grub2 Authentication
| |
− |
| |
− | Two functions suffer from integer underflow fault; the grub_username_get() and grub_password_get()located in
| |
− | grub-core/normal/auth.c and lib/crypto.c respectively. This can be exploited to obtain a Grub rescue shell.
| |
− |
| |
− | <nowiki>Upstream-Status: Accepted [http://git.savannah.gnu.org/cgit/grub.git/commit/?id=451d80e52d851432e109771bb8febafca7a5f1f2]</nowiki>
| |
− | CVE: CVE-2015-8370
| |
− | Signed-off-by: Joe Developer <joe.developer@example.com>
| |
− | | |
− | == Patches addressing bugs from Bugzilla ==
| |
− | | |
− | If your patch is addressing/fixing a bug from the Yocto Project bugzilla [https://bugzilla.yoctoproject.org/] then please use the <nowiki>[YOCTO #12345]</nowiki> tag in your commit message.
| |
− | Where 12345 is the bug id e.g. Bug 12345 [https://bugzilla.yoctoproject.org/show_bug.cgi?id=12345]
| |
− | | |
− | Consistent use of this syntax allows for automated tracking of bug fixes and enhancements.
| |
− | | |
− | === Example of Bugzilla tag in a commit message===
| |
− | | |
− | <nowiki>pseudo: Ignore mismatched inodes from the db
| |
− | | |
− | Currently, where pseudo finds a database entry for an inode but the path
| |
− | doesn't match, it reuses that database entry metadata. This is causing
| |
− | real world "corruption" of file attributes.
| |
− |
| |
− | See [YOCTO #14057] for an example of this.
| |
− |
| |
− | This can happen when files are deleted outside of pseudo context and the
| |
− | inode is reused by a new file which pseudo then "sees".
| |
− | | |
− | Its possible the opposite could happen, it needs to reuse attributes
| |
− | but this change would prevent it. As far as I can tell, we don't want
| |
− | pseudo to reuse these attributes though so this code should be safer
| |
− | and avoid bugs like the above.
| |
− | | |
− | </nowiki>Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
| |
− | | |
− | == Common Errors in Patch and Commit Messages ==
| |
− | | |
− | - Short log does not start with the file or component being modified. Such as
| |
− | "foo: Update to new upstream version 5.8.9"
| |
− | | |
− | - Avoid starting the summary line with a capital letter, unless the component
| |
− | being referred to also begins with a capital letter.
| |
− | | |
− | - Don't have one huge patch, split your change into logical subparts. It's
| |
− | easier to track down problems afterward using tools such as git bisect. It also
| |
− | makes it easy for people to cherry-pick changes into things like stable branches.
| |
− | | |
− | - Don't simply translate your change into English for a commit log. The log
| |
− | "Change compare from zero to one" is bad because it describes only the code
| |
− | change in the patch; we can see that from reading the patch itself. Let the code
| |
− | tell the story of the mechanics of the change (as much as possible), and let
| |
− | your comment tell the other details -- i.e. what the problem was, how it
| |
− | manifested itself (symptoms), and if necessary, the justification for why the
| |
− | fix was done in manner that it was. In other words, the long commit message
| |
− | must describe *why* the change was needed (instead of what has changed).
| |
− | | |
− | - Don't start your commit log with "This patch..." -- we already know it is a patch.
| |
− | | |
− | - Don't repeat your short log in the long log. If you really really don't have
| |
− | anything new and informational to add in as a long log, then don't put a long
| |
− | log at all. This should be uncommon -- i.e. the only acceptable cases for no
| |
− | long log would be something like "Documentation/README: Fix spelling mistakes".
| |
− | | |
− | - Don't put absolute paths to source files that are specific to your site, i.e.
| |
− | if you get a compile error on "fs/exec.c" then tidy up the parts of the compile
| |
− | output to just show that portion. We don't need to see that it was
| |
− | /home/wally/src/git/oe-core/meta/classes/insane.bbclass or similar - that full
| |
− | path has no value to others. Always reduce the path to something more
| |
− | meaningful, such as oe-core/meta/classes/insane.bbclass.
| |
− | | |
− | - Always use the most significant ramification of the change in the words of
| |
− | your subject/shortlog. For example, don't say "fix compile warning in foo" when
| |
− | the compiler warning was really telling us that we were dereferencing complete
| |
− | garbage off in the weeds that could in theory cause an OOPS under some
| |
− | circumstances. When people are choosing commits for backports to stable or
| |
− | distro kernels, the shortlog will be what they use for an initial sorting
| |
− | selection. If they see "Fix possible OOPS in...." then these people will look
| |
− | closer, whereas they most likely will skip over simple warning fixes.
| |
− | | |
− | - Don't put in the full 20 or more lines of a backtrace when really it is just
| |
− | the last 5 or so function calls that are relevant to the crash/fix. If the entry
| |
− | point, or start of the trace is relevant (i.e. a syscall or similar), you can
| |
− | leave that, and then replace a chunk of the intermediate calls in the middle
| |
− | with a single [...]
| |
− | | |
− | - Don't include timestamps or other unnecessary information, unless they are
| |
− | relevant to the situation (i.e. indicating an unacceptable delay in a driver
| |
− | initialization etc.)
| |
− | | |
− | - Don't use links to temporary resources like pastebin and friends. The commit
| |
− | message may be read long after this resource timed out.
| |
− | | |
− | - Don't reference mirrors, but instead always reference original authoritative
| |
− | sources.
| |
− | | |
− | - Avoid punctuation in the short log.
| |
− | | |
− | [[Category:Policy]]
| |