search

gazebo-tooling / release-tools

8 stars 9 forks source link

Jenkins is not reporting cppcheck failures #446

Open scpeters opened 3 years ago

scpeters commented 3 years ago

The Ubuntu jenkins builds are configured in dsl to report cppcheck errors by default, and I can see the xml files generated in the workspace of several jobs that includes some errors, but these are not reported by jenkins in the summary of that job. For example, the following job has errors in the cpplint.xml but does not report these error in the job summary, and is marked as a clean build:

chapulina commented 3 years ago

One thing to keep in mind is that we're also running cppcheck on GitHub actions, and if the cppcheck version is different between actions and Jenkins, it may be impossible to satisfy both at the same time.

So I think we should consider only running cppcheck in one place. The downside of actions is that the reports are hard to read. The downsides of Jenkins are this issue and also https://github.com/ignition-tooling/release-tools/issues/148.

scpeters commented 3 years ago

I used ign-physics as an example because it has a relatively small number of errors, but this affects osrf/gazebo, which doesn't use GitHub actions. If we don't want to run the code check in jenkins, then we should explicitly disable it in dsl for ignition and sdformat using the enable_cppcheck parameter to OSRFLinuxCompilationAnyGitHub. I'd appreciate it if we could fix this bug for gazebo though.

chapulina commented 3 years ago

I just noticed this again. It's confusing to have code_check.sh while it doesn't match make codecheck. We can only enforce one of them at a time, so I think having both does more harm than good.

How about we only run it if the file exists? This would allow us to remove it from Ignition while maintaining it on classic.

The downside of actions is that the reports are hard to read.

Come to think of it, I don't think this is a big deal, because it's not like we have flaky warnings whose history we want to check. We've been keeping the builds free of static warnings since that turns Actions red, so I think that just reading the logs has been working out.

scpeters commented 3 years ago

How about we only run it if the file exists? This would allow us to remove it from Ignition while maintaining it on classic.

sure, let's update the logic and delete code_check.sh for packages that use ign-cmake for configuration; that works for me

chapulina commented 3 years ago

I opened https://github.com/ignitionrobotics/ign-math/pull/211 to test removing it and #455 to update the logic.

I'll wait for the Ubuntu build on the ign-math PR to fail first (as I'd expect), and then trigger a build using the release-tools PR. Oh I see @scpeters already triggered one: Build Status https://build.osrfoundation.org/job/ignition_math-ci-main-bionic-amd64/4/