|
|
(6 intermediate revisions by 4 users not shown) |
Line 1: |
Line 1: |
− | This set of guidelines is intended to help both the developer and reviewers of | + | This page has moved to the 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
| + | * For the Patch Upstream Status documentation see the section ''[https://docs.yoctoproject.org/dev/contributor-guide/recipe-style-guide.html#patch-upstream-status Recipe Style Guide]'' |
− | commit messages that are part of the source control system, usually git. A patch
| + | * See section ''[https://docs.yoctoproject.org/dev/contributor-guide/submit-changes.html Contributing Changes to a Component]'' for everything else |
− | 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.
| |
− | | |
− | == 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
| |
− | | |
− | '''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
| |
− | no upstream (the upstream is no longer available -- dead project)
| |
− | 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>
| |
− | | |
− | == 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]]
| |