-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Not all static analyzers are run for all packages #57
Comments
The Space ROS project has been doing currently relies on the fact that many ROS 2 core packages rely on a metapackage called ament_lint_common, which is for the "common" set of linters and static analysis tools used by ROS 2 core packages, and patching ament_lint_common in Space ROS to include additional static analysis tools (or their ament / ament_cmake wrappers). Here's the current list of "common" linters from ament_lint's spaceros branch. This has the advantage of being relatively low touch and only requiring changes in one repository but the disadvantages are that it relies on all packages being considered for static analysis depending on ament_lint_common rather than an explicit list of analysis tools. Since the listed linters are expressed as dependencies that means that changing the list must be something happens in source at build time rather than during some later operation, especially since the CMake hooks which add tool runs to the test processes need to be updated when changing these dependencies. I'm just getting back from extended vacation but somewhere I've got the start of a proposal to try and decouple the build-time dependencies from the test-time static analysis runs. However doing so isn't trivial and there are several considerations and trade-offs with moving to that approach. I'll try to get that proposal resurrected and share it for feedback both within the ROS team and Space ROS itself. |
We'll revisit which static analyzers are executed for each package. However, this issue has no action item or taker attached to it, and it's too big to be addressed. I'm closing this and we'll revisit in the future. Thanks a lot, @Ronoman 😄 |
Reopened to cover @xfiderek's work demoed at the Space ROS Technical Meeting |
…). Signed-off-by: xfiderek <fiderekblazejgmail.com>
…). Signed-off-by: xfiderek <fiderekblazejgmail.com> Signed-off-by: xfiderek <[email protected]>
…). Signed-off-by: xfiderek <[email protected]>
PRs raised. they need to be merged in the following order: Check PR in spaceros repo for conceptual overview and gist of implementation |
…pace-ros/space-ros#57). Signed-off-by: xfiderek <[email protected]>
…pace-ros/space-ros#57). Signed-off-by: xfiderek <[email protected]>
Once again, we going to have to bump this because ament/ament_lint#482 is stalled outside our org. |
@xfiderek I see you have some understanding of how our static analyzers are run. can you please comment on my question of upgrade to jazzy in relation to ament_lint status? #205 (comment) |
If I'm not mistaken (which is quite possible, new to ROS2), the only tests run for a package under
colcon test
are ones that are listed in that package'spackage.xml
within<test_depend>package_name</test_depend>
tags. Some ROS2 packages (takercutils
for example) test_depend on ament_lint_common, which in turn depend on all of our static analyzers (ament_clang_tidy, ament_copyright, ament_cobra, ament_cppcheck, etc). However, other packages (taketf2
for example) only depend on a subset of these analyzers, so they are not all run.I'm not sure what the best way to resolve this is. There are two things I can think of:
package.xml
(which I believe is the case, but again could be wrong), insert<test_depend>
tags duringdocker build
, or when another common command is triggered (colcon build
,colcon test
,entrypoint.sh
for the docker image, or maybe an explicit command run by the user).Option 1 feels wrong to me, and option 2 feels overbearing.
The text was updated successfully, but these errors were encountered: