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
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
So I've asked some questions in the PR but receive few answers and it usually takes weeks for an answer. Should I be asking in the PR or here? In any case here's some questions.
For the SiFive license is there someone who could review it? I assuming there's some RIOT licensing guru who checks these things. Based on a quick search I read "If your software is a combined/derivate work with/of Apache-2 software, you cannot license that software under the GPL-2 and therefore cannot license it under the LGPL-2.1 either." ref: https://opensource.stackexchange.com/questions/5664/linking-from-lgpl-2-1-software-to-apache-2-0-library So maybe the SiFive code can not be used with RIOT.
I've never used a the PR system so this may be obvious to someone more experienced. Regarding the request to squash commits: Isn't that going to screw up anyone that's pulled that branch? Do I assume no one has pulled it or create a new PR? Also it looks like Travis tries to rebase the PR. There's also the issues that the format of the development commits are not in the RIOT format. It seems to me like doing a github's "Squash and merge" on when incorporating the PR solves these problems.
Regarding vendor supplied files: My assumption is those would be left as-is. Specifically no changes to formatting or header guards. Is that correct?
How are vendor supplied source (not header) files handled such that they are not subject to the strict format etc. tests. Is there an equivalent of the "vendor" directory for headers files but for source code?
thanks, JP
Hi JP,