Add a codecheck make target #682
Conversation
Utilizes ignition-cmake's IgnCodecheck module to create the codecheck target. Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
github-actions
bot
added
Gazebo 1️1️
Codecov Report
@@ Coverage Diff @@
## sdf9 #682 +/- ##
=======================================
Coverage 86.84% 86.84%
=======================================
Files 62 62
Lines 9738 9738
=======================================
Hits 8457 8457
Misses 1281 1281
Continue to review full report at Codecov.
|
adlarkin
added
the
needs upstream release
There was a problem hiding this comment.
LGTM. I tested it with gazebosim/gz-cmake#186 and had no issues. I just have one minor question.
cmake/SearchForStuff.cmake
Outdated
| # Find ignition cmake2 | ||
| # Only for using the testing macros and creating the codecheck target, not | ||
| # really being use to configure the whole project | ||
| find_package(ignition-cmake2 2.3 REQUIRED) |
There was a problem hiding this comment.
Is there a particular reason why version 2.3 is the minimum required version? It looks like ign_setup_target_for_codecheck exists before 2.3: https://github.com/ignitionrobotics/ign-cmake/blob/ignition-cmake2_2.2.0/cmake/IgnCodeCheck.cmake#L1-L2
There was a problem hiding this comment.
This will actually need to be updated since this PR needs a new release of ign-cmake to ensure make codecheck is clean.
There was a problem hiding this comment.
I wouldn't add that as a version requirement, since that will cause build failures if people have an outdated version of ign-cmake, which is a bit extreme to ensure correct linting. I would recommend a find_package call with no version requirement, since IgnCodeCheck was present in ign-cmake's 2.0.0 release
There was a problem hiding this comment.
That's a good point; I think I agree with @scpeters comments.
There was a problem hiding this comment.
Makes sense. If it won't cause failure in CI, since we assume CI will have the latest ign-cmake, we can remove the version requirement.
Also, I originally thought gazebosim/gz-cmake#187 would be needed to make CI pass, but it turns out without that PR, we do get output from codechech, but they are just warnings, not errors. So technically, this PR can go in without the ign-cmake release.
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
|
@osrf-jenkins run test |
🎉 New feature
Summary
Follow up from #680 (comment)
Utilizes ignition-cmake's IgnCodecheck module to create a
codechecktarget.Needs gazebosim/gz-cmake#186 to eliminate some false positives.
Test it
Run
make codecheckChecklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge