avoid cppcheck version 1.88 due to performance issues #168

wjwwood · 2019-07-31T00:40:31Z

I'm opening this as a possible solution to the cppcheck 1.88 issues we've been seeing, see: ros2/build_farmer#220 (comment)

This isn't ideal as our build machines will not be checking cppcheck during this period until a new fixed version of cppcheck is available (master already has fixes), and the same will be true for our users who may install cppcheck, but won't get checking until 1.88+.

However, it avoids the need for us to modify the installation instructions and instruct people to pin or install older versions of cppcheck only to remove those instructions again when 1.88+ is available (hopefully soon).

It modifies the CLI to print a message and return 188, e.g.:

% ament_cppcheck --include_dirs ./include ./test
cppcheck 1.88 has known performance issues and therefore will not be used, set the AMENT_CPPCHECK_ALLOW_1_88 environment variable to override this.

As the message implies if you set a specific environment variable you can override that:

% AMENT_CPPCHECK_ALLOW_1_88=1 ament_cppcheck --include_dirs ./include ./test
[test/test_subscription_traits.cpp:0]: (error: cppcheckError) Internal error: Child process crashed with signal 9
1 errors

Which currently crashes on my computer for rclcpp.

If you're using ament_cmake_cppcheck then you'll get a CMake warning:

--- stderr: rclcpp
CMake Warning at /Users/william/ros2_ws/install/ament_cmake_cppcheck/share/ament_cmake_cppcheck/cmake/ament_cppcheck.cmake:61 (message):
  [ament_cmake_cppcheck] cppcheck 1.88 has known performance issues, skipping
Call Stack (most recent call first):
  /Users/william/ros2_ws/install/ament_cmake_cppcheck/share/ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake:67 (ament_cppcheck)
  /Users/william/ros2_ws/install/ament_cmake_core/share/ament_cmake_core/cmake/core/ament_execute_extensions.cmake:48 (include)
  /Users/william/ros2_ws/install/ament_lint_auto/share/ament_lint_auto/cmake/ament_lint_auto_package_hook.cmake:21 (ament_execute_extensions)
  /Users/william/ros2_ws/install/ament_cmake_core/share/ament_cmake_core/cmake/core/ament_execute_extensions.cmake:48 (include)
  /Users/william/ros2_ws/install/ament_cmake_core/share/ament_cmake_core/cmake/core/ament_package.cmake:66 (ament_execute_extensions)
  CMakeLists.txt:490 (ament_package)


---
Finished <<< rclcpp [7.78s]

And the test will "pass":

% colcon test --event-handlers console_cohesion+ desktop_notification- --packages-select rclcpp --ctest-args -R cppcheck
Starting >>> rclcpp
--- output: rclcpp
UpdateCTestConfiguration  from :/Users/william/ros2_ws/build/rclcpp/CTestConfiguration.ini
Parse Config file:/Users/william/ros2_ws/build/rclcpp/CTestConfiguration.ini
   Site: kisra
   Build name: (empty)
 Add coverage exclude regular expressions.
SetCTestConfiguration:CMakeCommand:/usr/local/Cellar/cmake/3.13.4/bin/cmake
UpdateCTestConfiguration  from :/Users/william/ros2_ws/build/rclcpp/CTestConfiguration.ini
Parse Config file:/Users/william/ros2_ws/build/rclcpp/CTestConfiguration.ini
Test project /Users/william/ros2_ws/build/rclcpp
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 36
    Start 36: cppcheck

36: Test command: /Users/william/.pyenv/shims/python3 "-u" "/Users/william/ros2_ws/install/ament_cmake_test/share/ament_cmake_test/cmake/run_test.py" "/Users/william/ros2_ws/build/rclcpp/test_results/rclcpp/cppcheck.xunit.xml" "--package-name" "rclcpp" "--skip-test" "--output-file" "/Users/william/ros2_ws/build/rclcpp/ament_cppcheck/cppcheck.txt" "--command" "/Users/william/ros2_ws/install/ament_cppcheck/bin/ament_cppcheck" "--xunit-file" "/Users/william/ros2_ws/build/rclcpp/test_results/rclcpp/cppcheck.xunit.xml" "--include_dirs" "/Users/william/ros2_ws/src/ros2/rclcpp/rclcpp/include" "/Users/william/ros2_ws/src/ros2/rclcpp/rclcpp/test/"
36: Test timeout computed to be: 120
1/1 Test #36: cppcheck .........................   Passed    0.20 sec

The following tests passed:
	cppcheck

100% tests passed, 0 tests failed out of 1

Label Time Summary:
cppcheck    =   0.20 sec*proc (1 test)
linter      =   0.20 sec*proc (1 test)

Total Test time (real) =   0.20 sec
---
Finished <<< rclcpp [1.14s]

Summary: 1 package finished [2.89s]

But the test result will show it as skipped:

% colcon test-result --test-result-base build/rclcpp --all | grep cppcheck
build/rclcpp/test_results/rclcpp/cppcheck.xunit.xml: 1 test, 0 errors, 0 failures, 1 skipped

I'd like to hear from others if they think this is an appropriate workaround for now or not.

Signed-off-by: William Woodall <william@osrfoundation.org>

dirk-thomas

I haven't tried running this but it looks reasonable to me as a workaround.

wjwwood · 2019-07-31T00:57:11Z

One thing to consider is that 1.89 is "due" in 5 weeks according to their trac instance:

https://trac.cppcheck.net/milestone/1.89

I don't know if they'll consider doing a 1.88.1 or something similar and I don't know how long after that it will be picked up by homebrew and choco, so we might be looking at a gap of months.

wjwwood · 2019-07-31T00:59:18Z

Either way, this patch will prevent the test from misbehaving so I think we should take it.

Perhaps we can manually install an older version (e.g. 1.87) on the CI machines, then it would only affect end users who don't choose to downgrade themselves.

wjwwood · 2019-07-31T01:02:45Z

CI on the upgraded build machines with this patch:

mini1 (macOS):
portable (Windows):

nuclearsandwich · 2019-07-31T04:45:12Z

By emitting a warning this patch causes a bunch of CMake warnings in CI. I'm not sure if there's whether it's better to relax the warning to just a message, have it warn but squelch it during CI runs, or another workaround but it doesn't seem useful to live with the warnings in CI, though I'm willing to be swayed by a compelling argument for doing so.

wjwwood · 2019-07-31T18:45:43Z

My suggestion (but I forgot to include it) was going to be to add it to the filter in CI, that way it would remain highly visible for end users, but I can also downgrade it to a status message if you guys prefer.

wjwwood · 2019-07-31T18:46:00Z

I would not like to live with the warnings persisting on CI.

dirk-thomas · 2019-07-31T18:54:31Z

For the sake of keeping it simple I would downgrade the message to STATUS. The test will be marked as skipped already - I would consider that to be enough.

nuclearsandwich

I don't see anything showstopping so this can merge as long as we have a satisfactory CI build.

ament_cppcheck/ament_cppcheck/main.py

nuclearsandwich · 2019-07-31T20:00:24Z

I do have a question about the interplay between skipping the test via CMake checking the version and the runtime check: I haven't dug into the CMake logic but my expectation from reading the patch is that if cppcheck 1.88 is found during CMake build, tests using ament_cmake_cppcheck will be skipped unconditionally, so there's no way to force the use of cppcheck 1.88. Is that correct?

On the other hand, the runtime check in ament_cppcheck is override-able via environment variable? --again assuming that I'm reading the patch correctly.

nuclearsandwich · 2019-07-31T20:02:37Z

Also, I think this will need a backport (via fast-forward or otherwise) into Dashing in order to allow CI builds against the Dashing release to be run without this problem.

nuclearsandwich · 2019-07-31T20:06:13Z

I don't see anything showstopping

Actually, I thought of something. If the performance impact noted here: ros2/build_farmer#220 (comment) on cppcheck master is present in a future release is there any possibility that we would extend the test skip even though the impact was not fatal? If so, should we consider disabling versions >= 1.88 and then updating the patch when a satisfactory release is made?

Edit: I mostly raise this concern because I think we'll need to backport this to Dashing in order to enable CI builds against the release state once all Mac and Windows hosts have the upgraded cppcheck.

wjwwood · 2019-07-31T20:32:01Z

tests using ament_cmake_cppcheck will be skipped unconditionally, so there's no way to force the use of cppcheck 1.88. Is that correct?

That's correct. I didn't think it necessary, but I could add a cmake option if you guys want.

On the other hand, the runtime check in ament_cppcheck is override-able via environment variable? --again assuming that I'm reading the patch correctly.

That's also correct, but my thinking was that if we were testing it we might want to override the CLI, but that there was little value in overriding it via cmake, but that was just a arbitrary decision on my part, I'm happy to change it if desired.

wjwwood · 2019-07-31T20:33:50Z

I've already tested master and it's a lot faster and has all the patches that @cottsay pointed out, so I hope the next version will be fixed. I can update it to trigger on >= 1.88 if you guys want.

I'll open a pr for backport into dashing. We could ignore the failure, but it adds time to the CI since it has to repeatedly try it 10 times, timing out each time after 120s.

nuclearsandwich · 2019-07-31T20:41:42Z

Thanks for the clarifications. I am good with everything as-is. If we're sanguine about the next cppcheck release I am okay with skipping just on ==1.88.

wjwwood · 2019-07-31T21:08:12Z

Waiting on CI from portable before merging and doing the backport to Dashing:

wjwwood · 2019-07-31T23:23:51Z

All the test failures were know ones, so I'm merging this and I'll open the backport.

* avoid cppcheck version 1.88 due to performance issues Signed-off-by: William Woodall <william@osrfoundation.org> * downgrade to status from warning when skipping

avoid cppcheck version 1.88 due to performance issues

718e137

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood added the in review Waiting for review (Kanban column) label Jul 31, 2019

wjwwood self-assigned this Jul 31, 2019

wjwwood requested review from dirk-thomas, cottsay and nuclearsandwich July 31, 2019 00:40

wjwwood mentioned this pull request Jul 31, 2019

Upgrade Mac and Windows CI hosts ros2/build_farmer#220

Closed

dirk-thomas approved these changes Jul 31, 2019

View reviewed changes

downgrade to status from warning when skipping

3636725

nuclearsandwich approved these changes Jul 31, 2019

View reviewed changes

ament_cppcheck/ament_cppcheck/main.py Show resolved Hide resolved

wjwwood merged commit 21ffe80 into master Jul 31, 2019

wjwwood deleted the avoid_cppcheck_1_88 branch July 31, 2019 23:23

wjwwood mentioned this pull request Jul 31, 2019

[dashing backport] avoid cppcheck version 1.88 due to performance issues (#168) #170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid cppcheck version 1.88 due to performance issues #168

avoid cppcheck version 1.88 due to performance issues #168

wjwwood commented Jul 31, 2019

dirk-thomas left a comment

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019 •

edited

nuclearsandwich commented Jul 31, 2019

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019

dirk-thomas commented Jul 31, 2019

nuclearsandwich left a comment

nuclearsandwich commented Jul 31, 2019 •

edited

nuclearsandwich commented Jul 31, 2019

nuclearsandwich commented Jul 31, 2019 •

edited

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019

nuclearsandwich commented Jul 31, 2019

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019

avoid cppcheck version 1.88 due to performance issues #168

avoid cppcheck version 1.88 due to performance issues #168

Conversation

wjwwood commented Jul 31, 2019

dirk-thomas left a comment

Choose a reason for hiding this comment

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019 • edited

nuclearsandwich commented Jul 31, 2019

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019

dirk-thomas commented Jul 31, 2019

nuclearsandwich left a comment

Choose a reason for hiding this comment

nuclearsandwich commented Jul 31, 2019 • edited

nuclearsandwich commented Jul 31, 2019

nuclearsandwich commented Jul 31, 2019 • edited

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019

nuclearsandwich commented Jul 31, 2019

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019

wjwwood commented Jul 31, 2019 •

edited

nuclearsandwich commented Jul 31, 2019 •

edited

nuclearsandwich commented Jul 31, 2019 •

edited