Correct CMake Logic and update cpplint to Python3 #117

mjcarroll · 2020-10-09T20:57:13Z

For Python3 compatibility, this updates the version of cpplint to the latest version available on their repo: https://github.com/cpplint/cpplint/blob/0f2319741f3407d8638cdc7c41e4fc9bad217f68/cpplint.py

I also adjusted the CMake logic to prefer the Python3 interpreter when available. This logic can be further improved once CMake 3.12 is available across our supported platforms: https://cmake.org/cmake/help/v3.12/module/FindPython.html

Resolves #116

mjcarroll · 2020-10-09T20:57:52Z

Since we don't call this logic directly in the ign-cmake package, I think it would be worth checking cpplint results on a few of the other packages in the ign suite to make sure that warnings/errors haven't diverged from the version we were using previously.

chapulina

checking cpplint results on a few of the other packages

+1

cmake/IgnPython.cmake

scpeters · 2020-10-09T22:21:14Z

checking cpplint results on a few of the other packages

+1

I see 1967 complaints in ign-sensors3, which has no complaints with sh tools/code_check.sh or make codecheck with the released version of ign-cmake2: https://gist.github.com/scpeters/e3c4a36b9fdb392e7a58f804350792c9

chapulina · 2020-10-09T22:27:52Z

1967 complaints

ugh, that seems to complain about our style 😬

scpeters · 2020-10-09T22:34:21Z

1967 complaints

ugh, that seems to complain about our style

I wonder if we had made changes to cpplint.py?

mjcarroll · 2020-10-12T13:13:29Z

I wonder if we had made changes to cpplint.py?

I looked at the diff, and it wasn't obvious that we made any changes. But that's certainly a huge difference.

adlarkin

This PR seems to fix the issue of cppcheck not working with Ubuntu focal 👍

However, similar to what @scpeters mentioned, I got 11983 errors when running make codecheck on the main branch of ign-gazebo. If I got back to the released version of ign-cmake2, I get no errors when running make codecheck on the main branch of ign-gazebo.

Signed-off-by: Michael Carroll <[email protected]>

Co-authored-by: Louise Poubel <[email protected]> Signed-off-by: Michael Carroll <[email protected]>

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll · 2020-10-26T15:48:56Z

@adlarkin and @scpeters I believe this is ready for another look, I updated the filters to more closely match our previous configuration: ebc2fca

scpeters · 2020-10-26T20:20:52Z

how do you feel about adding a file or readme next to cpplint.py that documents where it came from? it's a little easier to find than looking back in the GitHub PR discussion

mjcarroll · 2020-10-26T20:27:17Z

how do you feel about adding a file or readme next to cpplint.py that documents where it came from? it's a little easier to find than looking back in the GitHub PR discussion

Ah, I had originally put a permalink to the upstream, but I must have removed it at some point. I'll add it back in for clarity 👍

scpeters · 2020-10-26T20:30:04Z

I got the following error when testing ign-sensors:

/data_fast/scpeters/ws/ign-sensors/src/ign-sensors/test/integration/TransportTestTools.hh:20:  <condition_variable> is an unapproved C++11 header.  [build/c++11] [5]

According to git blame, that block was last edited in 2014 (cpplint/cpplint@02af628). It looks like mutex, thread and chrono are also "unapproved"

mjcarroll · 2020-10-26T20:31:37Z

I already dropped a few of those headers, I'm thinking we can drop that category build/c++11 altogether?

chapulina · 2020-10-26T20:37:51Z

I'm thinking we can drop that category build/c++11 altogether?

+1

Signed-off-by: Michael Carroll <[email protected]>

adlarkin · 2020-11-04T01:47:34Z

Here are the connecting blueprint PRs that should resolve the codecheck issues introduced by this PR:

There are a few repositories that I couldn't fix:

ign-math:

root@097debe81253:~/ws/build/ignition-math6# make codecheck
/root/ws/src/ign-math/src/Kmeans.cc:152:  Empty loop bodies should use {} or continue  [whitespace/empty_loop_body] [5]
Total errors found: 1
CMakeFiles/cpplint.dir/build.make:57: recipe for target 'CMakeFiles/cpplint' failed
make[3]: *** [CMakeFiles/cpplint] Error 1
CMakeFiles/Makefile2:131: recipe for target 'CMakeFiles/cpplint.dir/all' failed
make[2]: *** [CMakeFiles/cpplint.dir/all] Error 2
CMakeFiles/Makefile2:171: recipe for target 'CMakeFiles/codecheck.dir/rule' failed
make[1]: *** [CMakeFiles/codecheck.dir/rule] Error 2
Makefile:236: recipe for target 'codecheck' failed
make: *** [codecheck] Error 2

ign-rendering:

root@097debe81253:~/ws/build/ignition-rendering2# make codecheck
/root/ws/src/ign-rendering/ogre/src/OgreRenderEngine.cc:497:  Empty loop bodies should use {} or continue  [whitespace/empty_loop_body] [5]
/root/ws/src/ign-rendering/ogre2/src/Ogre2RenderEngine.cc:480:  Empty loop bodies should use {} or continue  [whitespace/empty_loop_body] [5]
Total errors found: 2
CMakeFiles/cpplint.dir/build.make:57: recipe for target 'CMakeFiles/cpplint' failed
make[3]: *** [CMakeFiles/cpplint] Error 1
CMakeFiles/Makefile2:196: recipe for target 'CMakeFiles/cpplint.dir/all' failed
make[2]: *** [CMakeFiles/cpplint.dir/all] Error 2
CMakeFiles/Makefile2:75: recipe for target 'CMakeFiles/codecheck.dir/rule' failed
make[1]: *** [CMakeFiles/codecheck.dir/rule] Error 2
Makefile:197: recipe for target 'codecheck' failed
make: *** [codecheck] Error 2

The codecheck errors produced by these repositories are generated by do ... while loops (here's the example in ign-math). It looks like the linter sees while (...);, and interprets this as an empty while loop when it's really part of a do ... while loop. @mjcarroll is there a way to modify cpplint to handle this case?

scpeters · 2020-11-04T01:59:56Z

There are a few repositories that I couldn't fix:

I think we could suppress these with a // NOLINT on the same line

adlarkin · 2020-11-04T12:23:29Z

I think we could suppress these with a // NOLINT on the same line

That seemed to do the trick for ign-math: gazebosim/gz-math#175

For ign-rendering, I'm still seeing the error when using // NOLINT - could this be because the while extends to multiple lines (example here)?

chapulina · 2020-11-05T17:30:43Z

I'm still seeing the error when using // NOLINT - could this be because the while extends to multiple lines

Did you try using NOLINT on both lines?

Otherwise, I think it would be ok to add an empty {} to that while loop. If it complains about it being empty, you could add a comment in there?

scpeters · 2020-11-05T18:30:57Z

I'm still seeing the error when using // NOLINT - could this be because the while extends to multiple lines

Did you try using NOLINT on both lines?

Otherwise, I think it would be ok to add an empty {} to that while loop. If it complains about it being empty, you could add a comment in there?

it's a do - while loop, so we can't add a {} to the loop. I suppose we could add empty braces {}, but they wouldn't be connected to the loop. I would prefer NOLINT

adlarkin · 2020-11-05T19:10:29Z

Did you try using NOLINT on both lines?

Yeah, but I was still getting the false positive.

Otherwise, I think it would be ok to add an empty {} to that while loop. If it complains about it being empty, you could add a comment in there?

Like @scpeters said, I can't add {} to the loop since it's a do-while loop. I tried adding empty braces after the loop (see below for an example), but this still didn't solve the issue.

    while (renderSys &&
           renderSys->getName().compare("OpenGL 3+ Rendering Subsystem") != 0); 
    {}

chapulina · 2020-11-06T01:08:01Z

it's a do - while loop,

ha, I should have scrolled, I was wondering how that loop ever ended 😬

If the problem is that the while is split in 2 lines, maybe putting it all in one line is the solution? The NOLINT may prevent the complaint about the line being too long...

adlarkin · 2020-11-06T03:01:08Z

If the problem is that the while is split in 2 lines, maybe putting it all in one line is the solution? The NOLINT may prevent the complaint about the line being too long...

That worked! I'll make the PR for ign-rendering so that you guys can see how this fix looks before we merge it in 👍

mjcarroll · 2020-11-09T16:50:48Z

gazebosim/gz-sim#443 is the only thing left holding this one up.

adlarkin · 2020-11-10T23:04:53Z

ignitionrobotics/ign-gazebo#443 is the only thing left holding this one up.

All connecting PRs have now been merged, so I believe this PR is ready to merge now.

mjcarroll · 2020-11-10T23:05:38Z

All connecting PRs have now been merged, so I believe this PR is ready to merge now.

Thanks for doing the work on those!

chapulina · 2020-11-11T01:52:26Z

All connecting PRs have now been merged

They haven't all been forward-ported though 😉

adlarkin · 2020-11-11T02:07:29Z

All connecting PRs have now been merged

They haven't all been forward-ported though

Good point! Should I make some PRs that forward-port them, or do any of the other branches need to wait before forward porting?

chapulina · 2020-11-11T02:14:29Z

Should I make some PRs that forward-port them

That would be very helpful! I'm forward-porting ign-gazebo now, but I think all other libraries should be ready to go. Thanks!

scpeters · 2020-12-09T07:12:32Z

cmake/IgnPython.cmake

+  set(PYTHON_VERSION "3")
+endif()
+
+find_package(PythonInterp ${PYTHON_VERSION} REQUIRED)


does this mean python is now a required dependency of ignition-cmake2? the homebrew bottle build of 2.6.0 in osrf/homebrew-simulation#1222 is failing

https://build.osrfoundation.org/job/generic-release-homebrew_triggered_bottle_builder/label=osx_mojave/124/

I think we might want this to be QUIET instead of REQUIRED

does this mean python is now a required dependency of ignition-cmake2?

I think that this is what this line implies, although I'm not sure if we meant to create this dependency or not.

I think we might want this to be QUIET instead of REQUIRED

If we do need python though, what would the implications be of replacing REQUIRED with QUIET? I'm wondering if marking this QUIET can cause issues elsewhere if python is not found. @mjcarroll, what are your thoughts?

If it's an optional dependency that's only needed for optional modules (e.g. testing?), then whatever modules depend on it should check ${PythonInterp_FOUND} to conditionally build only if this package was found. If the package was found, then the module should skip being built.

we can continue the discussion in #132, which proposes to revert python3 to an optional dependency

Prior to #117, python was an optional dependency; this restores that behavior. Signed-off-by: Steve Peters <[email protected]>

Prior to #117, python was an optional dependency; this restores that behavior. * IgnPython: find PythonInterp with QUIET instead of REQUIRED * IgnCodeCheck: skip cpplint if python is not found Signed-off-by: Steve Peters <[email protected]>

mjcarroll requested review from scpeters and adlarkin October 9, 2020 20:57

mjcarroll requested a review from mxgrey as a code owner October 9, 2020 20:57

github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Oct 9, 2020

chapulina reviewed Oct 9, 2020

View reviewed changes

cmake/IgnPython.cmake Outdated Show resolved Hide resolved

adlarkin reviewed Oct 12, 2020

View reviewed changes

mjcarroll and others added 5 commits October 26, 2020 10:47

Update cpplint to python3 compatible-version

c90bbd9

Signed-off-by: Michael Carroll <[email protected]>

Find Python3 if available

ef66797

Signed-off-by: Michael Carroll <[email protected]>

Split targets but maintain codecheck

ed595cb

Signed-off-by: Michael Carroll <[email protected]>

Update cmake/IgnPython.cmake

084ab9a

Co-authored-by: Louise Poubel <[email protected]> Signed-off-by: Michael Carroll <[email protected]>

Add Ignition-specific default filters and suppressions

ebc2fca

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll force-pushed the update_cpplint_py3 branch from 1677a03 to ebc2fca Compare October 26, 2020 15:47

mjcarroll added 2 commits October 26, 2020 15:42

Don't do C+11 checks

3785150

Signed-off-by: Michael Carroll <[email protected]>

Add permalink

32c7f59

Signed-off-by: Michael Carroll <[email protected]>

adlarkin mentioned this pull request Nov 4, 2020

Resolve updated codecheck issues gazebosim/gz-transport#194

Merged

adlarkin mentioned this pull request Nov 4, 2020

Resolve updated codecheck issues gazebosim/gz-math#175

Merged

adlarkin mentioned this pull request Nov 6, 2020

Resolve updated codecheck issues gazebosim/gz-rendering#173

Merged

mjcarroll merged commit a559c57 into ign-cmake2 Nov 10, 2020

mjcarroll deleted the update_cpplint_py3 branch November 10, 2020 23:06

adlarkin mentioned this pull request Nov 13, 2020

3 ➡️ 4 gazebosim/gz-gui#143

Merged

scpeters reviewed Dec 9, 2020

View reviewed changes

scpeters mentioned this pull request Dec 9, 2020

ignition-cmake2 2.6.0 osrf/homebrew-simulation#1222

Closed

adlarkin mentioned this pull request Dec 10, 2020

Replace tools/code_check.sh with 'make codecheck' gazebo-tooling/action-gz-ci#26

Merged

scpeters added a commit that referenced this pull request Dec 10, 2020

IgnPython: revert python to optional dependency

d6a7f08

Prior to #117, python was an optional dependency; this restores that behavior. Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Dec 10, 2020

Revert python to optional dependency #132

Merged

mjcarroll mentioned this pull request Dec 22, 2020

Enable make codecheck for ign-cmake #138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct CMake Logic and update cpplint to Python3 #117

Correct CMake Logic and update cpplint to Python3 #117

mjcarroll commented Oct 9, 2020

mjcarroll commented Oct 9, 2020

chapulina left a comment

scpeters commented Oct 9, 2020

chapulina commented Oct 9, 2020

scpeters commented Oct 9, 2020

mjcarroll commented Oct 12, 2020

adlarkin left a comment

mjcarroll commented Oct 26, 2020

scpeters commented Oct 26, 2020

mjcarroll commented Oct 26, 2020

scpeters commented Oct 26, 2020

mjcarroll commented Oct 26, 2020

chapulina commented Oct 26, 2020

adlarkin commented Nov 4, 2020

scpeters commented Nov 4, 2020

adlarkin commented Nov 4, 2020

chapulina commented Nov 5, 2020

scpeters commented Nov 5, 2020

adlarkin commented Nov 5, 2020

chapulina commented Nov 6, 2020

adlarkin commented Nov 6, 2020

mjcarroll commented Nov 9, 2020

adlarkin commented Nov 10, 2020

mjcarroll commented Nov 10, 2020

chapulina commented Nov 11, 2020

adlarkin commented Nov 11, 2020

chapulina commented Nov 11, 2020

scpeters Dec 9, 2020

scpeters Dec 9, 2020

adlarkin Dec 9, 2020

mxgrey Dec 10, 2020

scpeters Dec 10, 2020

Correct CMake Logic and update cpplint to Python3 #117

Correct CMake Logic and update cpplint to Python3 #117

Conversation

mjcarroll commented Oct 9, 2020

mjcarroll commented Oct 9, 2020

chapulina left a comment

Choose a reason for hiding this comment

scpeters commented Oct 9, 2020

chapulina commented Oct 9, 2020

scpeters commented Oct 9, 2020

mjcarroll commented Oct 12, 2020

adlarkin left a comment

Choose a reason for hiding this comment

mjcarroll commented Oct 26, 2020

scpeters commented Oct 26, 2020

mjcarroll commented Oct 26, 2020

scpeters commented Oct 26, 2020

mjcarroll commented Oct 26, 2020

chapulina commented Oct 26, 2020

adlarkin commented Nov 4, 2020

scpeters commented Nov 4, 2020

adlarkin commented Nov 4, 2020

chapulina commented Nov 5, 2020

scpeters commented Nov 5, 2020

adlarkin commented Nov 5, 2020

chapulina commented Nov 6, 2020

adlarkin commented Nov 6, 2020

mjcarroll commented Nov 9, 2020

adlarkin commented Nov 10, 2020

mjcarroll commented Nov 10, 2020

chapulina commented Nov 11, 2020

adlarkin commented Nov 11, 2020

chapulina commented Nov 11, 2020

scpeters Dec 9, 2020

Choose a reason for hiding this comment

scpeters Dec 9, 2020

Choose a reason for hiding this comment

adlarkin Dec 9, 2020

Choose a reason for hiding this comment

mxgrey Dec 10, 2020

Choose a reason for hiding this comment

scpeters Dec 10, 2020

Choose a reason for hiding this comment