[riot-devel] Pull request

Martine Lenders mail at martine-lenders.eu
Tue Dec 19 08:34:32 CET 2017


Hi JP,

First of all, welcome to the RIOT community and thank you for your
contribution!

There are two steps to our CI at the moment. The first one is Travis, which
provides you with some preliminary static check regarding our coding
conventions, documentation and also some static code analysis. When someone
was able to review your PR they will task our build bot Murdock, the second
step, to try to build your PR for most known build configurations.

For now, I suggest that you review the results of Travis to make sure the
PR is ready for review. Here are the most important ones to fix right
now  (links are from the latest build of your PR as of writing this PR, you
can view the most current version by clicking on "Details" next to the
Travis build result at the bottom of the PR):

* Commit messages (
https://travis-ci.org/RIOT-OS/RIOT/builds/318473955#L705-L709): we are
loosely enforcing the 50/72 rule [1]. In our case this means: a commit
message should at most contain 50 characters, in special cases it may be
longer, but never longer than 72 characters. It is also preferable to
prefix the commit message with the module you manipulate / add
* Whitespaces (
https://travis-ci.org/RIOT-OS/RIOT/builds/318473955#L711-L1008): The only
whitespace a line should and with is `\n`. If you are on Windows and *have
your Git configured as such* it is also possible to have `\r\n`, Git will
than automatically strip the `\r`. Also use 4 spaces for indendentation and
don't mix spaces and tabs
* License (https://travis-ci.org/RIOT-OS/RIOT/builds/318473955#L1011-L1030):
The code you provided seems to be Apache 2 license. Please make sure it is
compatible to our LGPLv2.1 and update our license pattern list [2]
accordingly, if so (or ask for help in the PR).
* C++ compatible headers (
https://travis-ci.org/RIOT-OS/RIOT/builds/318473955#L1033-L1049): RIOT has
C++ support, thus it's headers need to be C++-compatible. Have a look [3]
[4] at existing headers how to achieve this.
* cppcheck's static code analysis (
https://travis-ci.org/RIOT-OS/RIOT/builds/318473955#L1051-L1055): Always
useful to review what it says. If it's a false positive suppress the
warning. Ask in the PR how to do that, if that's the case.
* The PR check (
https://travis-ci.org/RIOT-OS/RIOT/builds/318473955#L1057-L1077) you can
ignore for now. After the review is done, the reviewer will ask you to
squash your commits down to a sensible number.
* Header guards (
https://travis-ci.org/RIOT-OS/RIOT/builds/318473955#L1080-L1097): Header
guards should be the path of the header file, starting after the "include",
but all upper-case. E.g. boards/hifive1/include/sifive/hifive1.h would have
the header guard `SIFIVE_HIFIVE1_H`.

In general, have a look at our coding conventions [5]. Most of what I
paraphrased here is described there in more detail.

Cheers,
Martine

[1] https://gist.github.com/vecano/8494051#committing
[2] https://github.com/RIOT-OS/RIOT/tree/master/dist/tools/licenses/patterns
[3]
https://github.com/RIOT-OS/RIOT/blob/master/boards/samr21-xpro/include/periph_conf.h#L30-L32
[4]
https://github.com/RIOT-OS/RIOT/blob/master/boards/samr21-xpro/include/periph_conf.h#L281-L283
[5] https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions

2017-12-18 20:47 GMT+01:00 JP <
jpbonn-keyword-riotos.ae0e7f at corniceresearch.com>:

> I just created a pull request for merging support for the SiFive HiFive1
> board.  I've never used the pull request mechanism before so hopefully
> nothing is too buggered.  What need to be done for the CI integration?
> Those checks are failing.
>
> JP
> _______________________________________________
> devel mailing list
> devel at riot-os.org
> https://lists.riot-os.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/devel/attachments/20171219/218cfd8c/attachment.html>


More information about the devel mailing list