Catkin macros which provide standard linter configurations for C++ and Python.
ros / roslint Goto Github PK
View Code? Open in Web Editor NEWLint macros for ROS packages
Home Page: http://wiki.ros.org/roslint
Lint macros for ROS packages
Home Page: http://wiki.ros.org/roslint
For example:
roslint_python(src/mypkg/foo.py AS_TEST lint_python)
We would create the roslint and roslint_mypkg targets as usual, but also create a run_tests_mypkg_lint_python target, depended upon by run_tests. The idea would be that you could optionally have the linter check run as part of the CI's run_tests build.
On the implementation side, we should be able to call catkin_run_tests_target to wire it up, though you need to provide xunit xml output for that; that aspect of this intersects with #9. (And there may need to be transformation stage in there, to coerce the line-by-line output into the requisite XML.)
When "roslint_add_test()" is used in the CMakeLists.txt, "catkin_make run_tests" is able to execute roslint and populate the errors in the RESULTS_FILE.
However, this does not break the test and catkin_make completes without error.
Reported in #12.
Right now, the roslint_python and roslint_cpp macros support being passed lists of source files, or nothing, where the "nothing" behaviour is to glob and lint everything in the package. It would be great to be able to specify a path to lint everything below, eg:
roslint_cpp(DIRECTORY include)
Part of this could be supporting an optional corresponding FILES mode, which would be the same as the current behaviour for loose arguments, so the following would be equivalent:
roslint_cpp(FILES src/main.cpp src/foo.cpp)
roslint_cpp(src/main.cpp src/foo.cpp)
In this scenario, the follow calls would also be equivalent:
roslint_cpp(DIRECTORY .)
roslint_cpp()
At the very least, could check for valid XML, but perhaps also for deprecated tags, absolute paths, etc.
Reported in #12
Greetings! I'd love to release xacro into Kinetic, but it depends on roslint. Can you do a Kinetic release? Thanks ๐๐๐ช
This speaks for itself:
$ sudo apt-get install pylint
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following extra packages will be installed:
blt libdrm-intel1 libdrm-nouveau2 libdrm-radeon1 libfontenc1 libgl1-mesa-dri libgl1-mesa-glx libglapi-mesa libllvm3.2
libpciaccess0 libtxc-dxtn-s2tc0 libutempter0 libx11-xcb1 libxcb-dri2-0 libxcb-glx0 libxcb-shape0 libxss1 libxtst6 libxv1
libxxf86dga1 libxxf86vm1 python-egenix-mxdatetime python-egenix-mxtools python-logilab-astng python-logilab-common python-tk
tcl8.5 tcl8.5-lib tk8.5 tk8.5-lib x11-utils xbitmaps xterm
Suggested packages:
blt-demo libglide3 python-egenix-mxdatetime-dbg python-egenix-mxdatetime-doc python-egenix-mxtools-dbg
python-egenix-mxtools-doc pyro python-unittest2 tix python-tk-dbg tcl-tclreadline mesa-utils xfonts-cyrillic
Recommended packages:
libtxc-dxtn0
The following NEW packages will be installed:
blt libdrm-intel1 libdrm-nouveau2 libdrm-radeon1 libfontenc1 libgl1-mesa-dri libgl1-mesa-glx libglapi-mesa libllvm3.2
libpciaccess0 libtxc-dxtn-s2tc0 libutempter0 libx11-xcb1 libxcb-dri2-0 libxcb-glx0 libxcb-shape0 libxss1 libxtst6 libxv1
libxxf86dga1 libxxf86vm1 pylint python-egenix-mxdatetime python-egenix-mxtools python-logilab-astng python-logilab-common
python-tk tcl8.5 tcl8.5-lib tk8.5 tk8.5-lib x11-utils xbitmaps xterm
0 upgraded, 34 newly installed, 0 to remove and 150 not upgraded.
Need to get 17.1 MB of archives.
After this operation, 59.7 MB of additional disk space will be used.
Do you want to continue [Y/n]?
There's no way a linter should require 60MB of stuff. The issue is the dependency on python-logilab-astng, which brings in all kinds of xwindows junk.
Given #2 as well, I think we should swap out pylint for https://pypi.python.org/pypi/pep8
C++11 headers such as <future>
are "unapproved" in upstream cpplint.py. Support for these should be monkey-patched in.
The default set (in pylint) is way too pedantic, imo.
I think we can share some syntaxes for package.xml.
like
CMakeLists.txt
package.xml
build_depend
and run_depend
buildtool_depend
-> build_depend
-> run_depend
-> test_depend
Any idea?
In the following file:
/*********************************************************************
* Copyright (c) XXXXX, Inc.
*********************************************************************/
void *testFunction(void * /*unused_param*/)
{
}
Cpplint returns:
test.cpp:4: Using C-style cast. Use reinterpret_cast<void *>(...) instead [readability/casting] [4]
In cpplint.py here there is already a case to cover this. But the problem is that this condition only gets checked if the line ends (among other things) with the character {
, as you can see here.
So that case would be correctly caught if we use
/*********************************************************************
* Copyright (c) XXXXX, Inc.
*********************************************************************/
void *testFunction(void * /*unused_param*/) {
}
but then of course we get an error about the position of the braces.
test.cpp:4: when starting a new scope, { should be on a line by itself [whitespace/braces] [4]
In my code I have a do/while loop as follows:
do
{
// do something ....
} while (!msg_received);
I get the following error from lint:
} should be on a line by itself [whitespace/braces] [4]
But if I move the while statement to a new line, I get the next error:
Empty loop bodies should use {} or continue [whitespace/empty_loop_body] [5]
Is there anyway to solve this issue?
There are currently no rosdep keys defined for python-pylint or python-cpplint, which are needed to resolve the roslint package build dependencies.
Plus, these probably need to use the transitive <run_depend>
instead, since they are needed at build time by packages that depend on roslint. (With package format 2, that will become a <build_export_depend>
, see the rep-0140 draft).
This is a friendly reminder that the Melodic buildfarm is now available. It would be great to have a release of roslint
into Melodic, as it is currently directly blocking 18 packages and indirectly blocking 28 packages (http://repositories.ros.org/status_page/blocked_releases_melodic.html). Thank you!
Especially important for:
I couldn't find how to run roslint with catkin_tools at ROS wiki
How can I run it?
with
roslint_cpp()
in CMakeLists.txt?
I cannot see the LICENSE file (explicit, including copyright holders and year). This makes it hard to reuse the code from this repo lawfully.
Running the linters depends on rosrun, which does not work unless you've sourced the devel space.
Unsure if catkin_find fixes this, or if it will just remain an open issue.
Example failure case + fix here: moveit/moveit_msgs#8 (comment)
On the wiki. For example, an actual astyle config/invocation which brings C++ whitespace into compliance, and something similar for Python.
This is directly contrary to the Cpp style guide example: http://wiki.ros.org/CppStyleGuide#Formatting
To eliminate files from linting (eg, tests, files which come from elsewhere, etc).
Should support individual files or globs.
Creating an output file would be useful for users.
Such a target should cause the lint command only to run again when its dependencies change.
As discussed here: 0136f53
Use the python facilities provided by rosunit for this, rather than a template baked into the bash wrapper.
It might be useful to add basic checks on the package.xml
for a package.
Particularly, I've seen a few packages use <license></license>
which doesn't cause any errors, but makes RPM generation fail, and is really against the package.xml
format rep: http://www.ros.org/reps/rep-0127.html#license-multiple-but-at-least-one
For the packages who did this, they left it blank because the code was in public domain and was not licensed. I recommended using <license>Public Domain</license>
, and all so far have adopted it.
There could also be checks for things such as properly formatted E-mail addresses for authors and maintainers, and properly formatted (and non-empy) URLs.
With the v2 package.xml
spec rolling out soon, there will likely be more nuances like this that a lint tool should check.
Great work here!
The recommended style for #ifndef guards includes the package name
#ifndef PACKAGE_PATH_FILE_H
This tool suggests only PATH_FILE_H
**Unless I'm invoking the roslint tool with the wrong parameters. The documentation isn't super clear on the CLI.
The following code is does not pass cpplint::whitespace/empty_loop_body
check:
do
{
...
}
while (cond);
Error:
Empty loop bodies should use {} or continue [whitespace/empty_loop_body] [5]
This is a follow-up from http://answers.ros.org/question/246677
According to the ROS C++ style guide, function names should be camelCased
(lower-case first letter), not CamelCased
(capital-case first letter). However, roslint doesn't seem to complain about CamelCased
function names. Actually, it doesn't complain even if a function name is under_scored
๐ค
Here's a proof of concept:
void my_silly_test() {
return;
}
And here's the output of catkin build --no-deps <package> --make-args roslint_<package>
:
when starting a new scope, { should be on a line by itself [whitespace/braces]
It correctly complains about the scope delimiter, but not the function name.
As pointed out in #12, some ROS code which follows PCL triggers a ton of these.
The following code:
server = registerServer("server_name", {State::First, State::Second});
where we pass C++11 bracer-init-list as the second argument fails to pass the CheckBracers
. The special case where both bracers happen to be on the same line fails because only one character is expected after the closing bracer }
, but in our case we have 2 characters });
roslint/src/roslint/cpplint_wrapper.py
Lines 85 to 93 in 63e81f9
Chaninging the line
roslint/src/roslint/cpplint_wrapper.py
Line 89 in 63e81f9
if Match(r'^(.*){(.*)}.+$', line):
would probably solve the issue, but as I am not an expert in writing linters, it could potentially break other stuff.
Any suggestions ?
needed for: #1
While useful in general, this is needed for checking the unit test return status (#7).
A catkin linter is already being tracked here: ros/catkin#153
So for now, this is blocked on the availability of that tool.
I need to write some code that uses std::transform, like this:
std::transform(rows.begin(), rows.end(), std::back_inserter(concepts), [this](const mysqlx::Row & row)
{
return Concept(row[0], *this);
});
This will generate } should be on a line by itself [whitespace/braces] [4]
. If I do this though, I'll just get closing ) should be moved to the previous line [whitespace/parens] [2]
.
Would a PR that updates cpplint.py and pep8.py to newer versions be accepted? Thanks!
This is in direct contradiction to the ROS style guide.
Hi,
Would it be possible to set a longer limit for the python line size? 80 is really small - even pep8 says that 100 would be ok.
Cheers
Check for well-formedness, sane indentation.
cpplint
currently doesn't like do while loops and erroneously warns about them. The problem seems to be due to the change in brace styles: https://github.com/ros/roslint/blob/master/src/roslint/cpplint.py#L3130
Given:
// Copyright 2015 Alex Henning
void dowhile()
{
do
{
// Do some stuff
}
while (true);
}
Result:
dowhile.cpp:9: Empty loop bodies should use {} or continue [whitespace/empty_loop_body] [5]
Done processing dowhile.cpp
Total errors found: 1
Apart from catkin_lint, primarily check for:
Patched GetHeaderGuardCPPVariable
method expects to find "include" in a header file's path. When "include" is not found in the file's path this method hangs in a continuous loop:
var_parts = list()
head = filename
while head:
head, tail = os.path.split(head)
var_parts.insert(0, tail)
if head.endswith('include'):
break
Once the current Indigo sync occurs:
Add cpplint tests in order to allow cpplint.py to be updated more confidently.
Can roslint_cpp default glob files add .hpp suffix? or add another option for the function?
roslint/cmake/roslint-extras.cmake.em
Lines 46 to 56 in 1a90b02
The current brace check overlooks many instances of the braces being on the same line. This is due to the fact that in line 99 it suppresses any warnings where there is an =
sign on the line. However, this occurs in many common cases:
if (i == 0) {
for (int i = 0; i < 10; i++) {
while (i == 0) {
I think the best fix is to check if the {
is preceded by a )
in which case it is likely one of the above. I don't think that would occur with a brace initializer.
@mikepurvis Thoughts?
It looks like roslint
does not have a source entry for Noetic and is directly blocking 54 other repos. Mind adding a source entry for it? Will the noetic release use the master
branch?
A source entry signals to other packages it is being worked on, and makes it possible for downstream maintainers to use rosinstall_generator
to download their dependencies while they work on Python 3 incompatibilities.
I didn't see any Python 3 syntax errors, and all the tests appeared to pass using Python 3 on Debian Buster. Is roslint
already compatible with Python 3?
Accessors for structs should be indented the same as they would in classes (not at all). However, cpplint warns that they should be indented one space. This can likely be updated the same way it was for classes.
Given:
// Copyright 2015 Alex Henning
struct A
{
public:
}
Result:
structs.cpp:5: public: should be indented +1 space inside struct A [whitespace/indent] [3]
Done processing structs.cpp
Total errors found: 1
It looks like roslint's CPP module uses the Google style guide, which is slightly different from ROS's long-standing one (http://wiki.ros.org/CppStyleGuide). Notable items below.
ROS style guide specifies putting opening brace on it's own line, current rules throw:
{ should almost always be at the end of the previous line [whitespace/braces] [4]
An else should appear on the same line as the preceding } [whitespace/newline] [4]
Examples, and most of ROS core code put "public:" and similar labels at the beginning of the line. Style guide linked above also shows it this way. Causes the following to be thrown:
Labels should always be indented at least one space. If this is a member-initializer list in a constructor or the base class list in a class definition, the colon should be on the following line. [whitespace/labels] [4]
ROS Services use non-const references, causes:
Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2]
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.