X-Git-Url: https://review.openocd.org/gitweb?p=openocd.git;a=blobdiff_plain;f=HACKING;h=cf3f58906064381952c45a52c9494faf8a0aa5ce;hp=658aa76b592c13379584a2b9953eedf32438308d;hb=HEAD;hpb=0355d987938d57fcb50b2a9b39b0f89495403357 diff --git a/HACKING b/HACKING index 658aa76b59..74cbe02e74 100644 --- a/HACKING +++ b/HACKING @@ -1,13 +1,20 @@ // This file is part of the Doxygen Developer Manual /** @page patchguide Patch Guidelines -@b NB! If you're behind a corporate wall with http only access to the -world, you can still use these instructions! - -@b NB2! You can't send patches to the mailing list anymore at all. Nowadays +\attention You can't send patches to the mailing list anymore at all. Nowadays you are expected to send patches to the OpenOCD Gerrit GIT server for a review. +\attention If you already have a Gerrit account and want to try a +different sign in method, please first sign in as usually, press your +name in the upper-right corner, go to @a Settings, select @a +Identities pane, press Link Another Identity button. In case +you already have duplicated accounts, ask administrators for manual +merging. + +\attention If you're behind a corporate wall with http only access to the +world, you can still use these instructions! + @section gerrit Submitting patches to the OpenOCD Gerrit server OpenOCD is to some extent a "self service" open source project, so to @@ -22,12 +29,73 @@ The procedure to create a patch is essentially: - correct the patch and re-send it according to review feedback Your patch (or commit) should be a "good patch": focus it on a single -issue, and make it be easily reviewable. Don't make +issue, and make it easily reviewable. Don't make it so large that it's hard to review; split large -patches into smaller ones. (That can also help -track down bugs later on.) All patches should +patches into smaller ones (this will also help +to track down bugs later). All patches should be "clean", which includes preserving the existing -coding style and updating documentation as needed. +coding style and updating documentation as needed. When adding a new +command, the corresponding documentation should be added to +@c doc/openocd.texi in the same commit. OpenOCD runs on both Little +Endian and Big Endian hosts so the code can't count on specific byte +ordering (in other words, must be endian-clean). + +There are several additional methods of improving the quality of your +patch: + +- Runtime testing with Valgrind Memcheck + + This helps to spot memory leaks, undefined behaviour due to + uninitialized data or wrong indexing, memory corruption, etc. + +- Clang Static Analyzer + + Using this tool uncovers many different kinds of bugs in C code, + with problematic execution paths fully explained. It is a part of + standard Clang installation. + + To generate a report, run this in the OpenOCD source directory: + @code + mkdir build-scanbuild; cd build-scanbuild + scan-build ../configure + scan-build make CFLAGS="-std=gnu99 -I. -I../../jimtcl" + @endcode + +- Runtime testing with sanitizers + + Both GCC and LLVM/Clang include advanced instrumentation options to + detect undefined behaviour and many kinds of memory + errors. Available with @c -fsanitize=* command arguments. + + Example usage: + @code + mkdir build-sanitizers; cd build-sanitizers + ../configure CC=clang CFLAGS="-fno-omit-frame-pointer \ + -fsanitize=address -fsanitize=undefined -ggdb3" + make + export ASAN_OPTIONS=detect_stack_use_after_return=1 + src/openocd -s ../tcl -f /path/to/openocd.cfg + @endcode + +- Sparse Static Analyzer + + Using this tool allows identifying some bug in C code. + In the future, OpenOCD would use the sparse attribute 'bitwise' to + detect incorrect endianness assignments. + + Example usage: + @code + mkdir build-sparse; cd build-sparse + ../configure CC=cgcc CFLAGS="-Wsparse-all -Wno-declaration-after-statement \ + -Wno-unknown-attribute -Wno-transparent-union -Wno-tautological-compare \ + -Wno-vla -Wno-flexible-array-array -D__FLT_EVAL_METHOD__=0" + make + @endcode + +Please consider performing these additional checks where appropriate +(especially Clang Static Analyzer for big portions of new code) and +mention the results (e.g. "Valgrind-clean, no new Clang analyzer +warnings") in the commit message. Say in the commit message if it's a bugfix (describe the bug) or a new feature. Don't expect patches to merge immediately @@ -38,7 +106,7 @@ Add yourself to the GPL copyright for non-trivial changes. @section stepbystep Step by step procedure --# Create a Gerrit account at: http://openocd.zylin.com +-# Create a Gerrit account at: https://review.openocd.org - On subsequent sign ins, use the full URL prefaced with 'http://' For example: http://user_identifier.open_id_provider.com -# Add a username to your profile. @@ -47,8 +115,14 @@ Add yourself to the GPL copyright for non-trivial changes. add a username of your choice. Your username will be required in step 3 and substituted wherever the string 'USERNAME' is found. - -# Add an SSH public key following the directions on github: - https://help.github.com/articles/generating-ssh-keys + -# Create an SSH public key following the directions on github: + https://help.github.com/articles/generating-ssh-keys . You can skip step 3 + (adding key to Github account) and 4 (testing) - these are useful only if + you actually use Github or want to test whether the new key works fine. + -# Add this new SSH key to your Gerrit account: + go to 'Settings' > 'SSH Public Keys', paste the contents of + ~/.ssh/id_rsa.pub into the text field (if it's not visible click on + 'Add Key ...' button) and confirm by clicking 'Add' button. -# Clone the git repository, rather than just download the source: @code git clone git://git.code.sf.net/p/openocd/code openocd @@ -62,25 +136,30 @@ Add yourself to the GPL copyright for non-trivial changes. to instruct git locally how to send off the changes. -# Add a new remote to git using Gerrit username: @code -git remote add review ssh://USERNAME@openocd.zylin.com:29418/openocd.git -git config remote.review.push HEAD:refs/publish/master +git remote add review ssh://USERNAME@review.openocd.org:29418/openocd.git +git config remote.review.push HEAD:refs/for/master @endcode Or with http only: @code -git remote add review http://openocd.zylin.com/p/openocd.git -git config remote.review.push HEAD:refs/publish/master +git remote add review https://USERNAME@review.openocd.org/p/openocd.git +git config remote.review.push HEAD:refs/for/master @endcode - -# You will need to install this hook, we will look into a better solution: + The http password is configured from your gerrit settings - https://review.openocd.org/#/settings/http-password. + \note If you want to simplify http access you can also add your http password to the url as follows: @code -scp -p -P 29418 USERNAME@openocd.zylin.com:hooks/commit-msg .git/hooks/ +git remote add review https://USERNAME:PASSWORD@review.openocd.org/p/openocd.git @endcode - Or with http only: + \note All contributions should be pushed to @c refs/for/master on the +Gerrit server, even if you plan to use several local branches for different +topics. It is possible because @c for/master is not a traditional Git +branch. + -# You will need to install this hook, we will look into a better solution: @code -wget http://openocd.zylin.com/tools/hooks/commit-msg +wget https://review.openocd.org/tools/hooks/commit-msg mv commit-msg .git/hooks chmod +x .git/hooks/commit-msg @endcode -@b NOTE A script exists to simplify the two items above. execute: + \note A script exists to simplify the two items above. Execute: @code tools/initial.sh @endcode @@ -92,7 +171,7 @@ git config --global user.email "john@smith.org" @endcode -# Work on your patches. Split the work into multiple small patches that can be reviewed and - applied seperately and safely to the OpenOCD + applied separately and safely to the OpenOCD repository. @code while(!done) { @@ -101,17 +180,50 @@ while(!done) { run tools/checkpatch.sh to verify your patch style is ok. } @endcode - @b TIP! use "git add ." before commit to add new files. -@code ---- example comment, notice the short first line w/topic --- -topic: short comment + \note use "git add ." before commit to add new files. + + \note check @ref checkpatch for hint about checkpatch script + + Commit message template, notice the short first line. + The field 'specify touched area' + should identify the main part or subsystem the patch touches. +@code{.unparsed} +specify touched area: short comment -longer comments over several -lines... +Longer comments over several lines, explaining (where applicable) the +reason for the patch and the general idea the solution is based on, +any major design decisions, etc. Limit each comment line's length to 75 +characters; since 75 it's too short for a URL, you can put the URL in a +separate line preceded by 'Link: '. Signed-off-by: ... ------ @endcode + Examples: +@code{.unparsed} +flash/nor/atsame5: add SAME59 support + +Add new device ID +@endcode +@code{.unparsed} +flash/nor: flash driver for XYZ123 + +Add new flash driver for internal flash of ... +@endcode +@code{.unparsed} +target/cortex_m: fix segmentation fault in cmd 'soft_reset_halt' + +soft_reset_halt command failed reproducibly under following conditions: ... +Test for NULL pointer and return error ... + +Reported-by: John Reporter +Fixes: 123456789abc ("target: the commit where the problem started") +BugLink: https://sourceforge.net/p/openocd/tickets/999/ +@endcode +@code{.unparsed} +doc: fix typos +@endcode + See "git log" for more examples. + -# Next you need to make sure that your patches are on top of the latest stuff on the server and that there are no conflicts: @@ -130,11 +242,70 @@ git push review Further reading: http://www.coreboot.org/Git +@section checkpatch About checkpatch script + +OpenOCD source code includes the script checkpatch to let developers to +verify their patches before submitting them for review (see @ref gerrit). + +Every patch for OpenOCD project that is submitted for review on Gerrit +is tested by Jenkins. Jenkins will run the checkpatch script to analyze +each patch. +If the script highlights either errors or warnings, Gerrit will add the +score "-1" to the patch and maintainers will probably ignore the patch, +waiting for the developer to send a fixed version. + +The script checkpatch verifies the SPDX tag for new files against a very +short list of license tags. +If the license of your contribution is not listed there, but compatible +with OpenOCD license, please alert the maintainers or add the missing +license in the first patch of your patch series. + +The script checkpatch has been originally developed for the Linux kernel +source code, thus includes specific tests and checks related to Linux +coding style and to Linux code structure. While the script has been +adapted for OpenOCD specificities, it still includes some Linux related +test. It is then possible that it triggers sometimes some false +positive! + +If you think that the error identified by checkpatch is a false +positive, please report it to the openocd-devel mailing list or prepare +a patch for fixing checkpatch and send it to Gerrit for review. + +\attention The procedure below is allowed only for exceptional +cases. Do not use it to submit normal patches. + +There are exceptional cases in which you need to skip some of +the tests from checkpatch in order to pass the approval from Gerrit. + +For example, a patch that modify one line inside a big comment block +will not show the beginning or the end of the comment block. This can +prevent checkpatch to detect the comment block. Checkpatch can wrongly +consider the modified comment line as a code line, triggering a set of +false errors. + +Only for exceptional cases, it is allowed to submit patches +to Gerrit with the special field 'Checkpatch-ignore:' in the commit +message. This field will cause checkpatch to ignore the error types +listed in the field, only for the patch itself. +The error type is printed by checkpatch on failure. +For example the names of Windows APIs mix lower and upper case chars, +in violation of OpenOCD coding style, triggering a 'CAMELCASE' error: +@code +CHECK:CAMELCASE: Avoid CamelCase: +#96105: FILE: src/helper/log.c:505: ++ error_code = WSAGetLastError(); +@endcode +Adding in the commit message of the patch the line: +@code +Checkpatch-ignore: CAMELCASE +@endcode +will force checkpatch to ignore the CAMELCASE error. + @section timeline When can I expect my contribution to be committed? The code review is intended to take as long as a week or two to allow maintainers and contributors who work on OpenOCD only in their spare -time oportunity to perform a review and raise objections. +time opportunity to perform a review and raise objections. With Gerrit much of the urgency of getting things committed has been removed as the work in progress is safely stored in Gerrit and @@ -149,8 +320,22 @@ master branch will be much reduced. If a contributor pushes a patch, it is considered good form if another contributor actually approves and submits that patch. +It should be noted that a negative review in Gerrit ("-1" or "-2") may (but does +not have to) be disregarded if all conditions listed below are met: + +- the concerns raised in the review have been addressed (or explained), +- reviewer does not re-examine the change in a month, +- reviewer does not answer e-mails for another month. + @section browsing Browsing Patches -All OpenOCD patches can be reviewed here. +All OpenOCD patches can be reviewed here. + +@section reviewing Reviewing Patches +From the main Review +page select the patch you want to review and click on that patch. On the +appearing page select the download method (top right). Apply the +patch. After building and testing you can leave a note with the "Reply" +button and mark the patch with -1, 0 and +1. */ /** @file This file contains the @ref patchguide page.