[riot-devel] Pull request for RISCV

JP jpbonn-keyword-riotos.ae0e7f at corniceresearch.com
Sat Jan 13 01:12:01 CET 2018


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


On 12/19/2017 12:34 AM, Martine Lenders wrote:
> 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 
> <mailto: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 <mailto:devel at riot-os.org>
>     https://lists.riot-os.org/mailman/listinfo/devel
>     <https://lists.riot-os.org/mailman/listinfo/devel>
> 
> 
> 
> 
> _______________________________________________
> devel mailing list
> devel at riot-os.org
> https://lists.riot-os.org/mailman/listinfo/devel
> 


More information about the devel mailing list