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.ae0...@corniceresearch.com
<mailto:jpbonn-keyword-riotos.ae0...@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@riot-os.org <mailto:devel@riot-os.org>
https://lists.riot-os.org/mailman/listinfo/devel
<https://lists.riot-os.org/mailman/listinfo/devel>
_______________________________________________
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel
