How to automatically run catkin_lint with other tests?

[peci1]

I'd like to automatically run catkin_lint with other tests, e.g. with roslint. First, I thought about writing something as simple as

if(CATKIN_ENABLE_TESTING)
  find_package(roslint REQUIRED)
  roslint_custom(catkin_lint "-W2" .)
  roslint_add_test()
endif()

However, this approach fails horribly on the ROS buildfarm, which (for a reason I can't understand) runs tests in an environment without rosdep cache (i.e. rosdep update has not been called), and sometimes does not even contain ROS_ROOT or ROS_DISTRO environment variables. See e.g. ros-infrastructure/ros_buildfarm#923 . It either results in exceptions, or in packages like python-tqdm reported as unknown packages.

So, after a lot of tinkering, I've come with this ugly piece of CMake code that disables the catkin_lint test on ROS buildfarm, but keeps it enabled elsewhere (e.g. IndustrialCI or local testing).

if(CATKIN_ENABLE_TESTING)
  find_package(roslint REQUIRED)

  # catkin_lint - checks validity of package.xml and CMakeLists.txt
  # ROS buildfarm calls this without any environment and with empty rosdep cache,
  # so we have problems reading the list of packages from env
  # see https://github.com/ros-infrastructure/ros_buildfarm/issues/923
  if(DEFINED ENV{ROS_HOME})
    #catkin_lint: ignore_once env_var
    set(ROS_HOME "$ENV{ROS_HOME}")
  else()
    #catkin_lint: ignore_once env_var
    set(ROS_HOME "$ENV{HOME}/.ros")
  endif()

  #catkin_lint: ignore_once env_var
  if(DEFINED ENV{ROS_ROOT} AND EXISTS "${ROS_HOME}/rosdep/sources.cache")
    roslint_custom(catkin_lint "-W2" .)
    roslint_add_test()
  endif()
endif()

I don't consider this particularly nice. This snippet would need to go to all packages which I want to lint during testing.

Does anybody have a better idea how to run this linter during tests?

[Timple]

I think there is also a cmake version. See this example for implementation:

https://github.com/tue-robotics/ed/blob/master/CMakeLists.txt

[peci1]

Thanks for finding it. However, that package doesn't solve the ROS buildfarm problem - the package itself has devel jobs enabled, but they fail earlier than this problem could manifest.

Wouldn't you be interested in integrating a cmake helper directly in catkin_lint? I might find some time for a PR. It doesn't necessarily have to be as complicated as catkin_lint_cmake with the XSL transform of the output... And it could handle the ROS buildfarm problems out of the box.

I also wanted to look if it wouldn't be possible to move the rosdep cache to the build directory (probably via some parameters) and autoupdate it - or at least add an option to do so.

[peci1]

@Timple what do you say to #109 ? The PR allows specifying custom paths for rosdep sources and cache, and allows running init and update automatically. That could be first step to a nicer solution on the ROS buildfarm.

[Timple]

Relaying this to @MatthijsBurgh as he made the initial attempt on cmake I believe

Matthijs: do you have an interest or opinion on this?

[MatthijsBurgh]

So I have no clue why rospack can't find catkin_lint_cmake in the jenkins job. It works fine in the GH actions job I run. Which is comparable with building and testing in a devel catkin workspace.

But as explained in #109, catkin_lint_cmake would also fail on the same issue. As the issue is the combination of catkin_lint and the ROS buildfarm.

catkin_lint provides an --offline option. Does that already solve the issue for the ROS buildfarm? Otherwise I suggest to expose the use_rosdep option of the CatkinEnvironment instead of the hassle of #109.

We also need to ask ourselves, why does the ROS buildfarm not have a rosdep cache available?

Whether @fkie wants to integrate a simple CMake macro, integrate my project catkin_lint_cmake or doesn't want to integrate any CMake support is up to him. And is actually a separate question from the ROS buildfarm issue.

[peci1]

I think the rationale for not running init and update on the buildfarm before tests is described here:

Not at the moment, no. The dominant pattern regarding dependencies in the various jobs of ros_buildfarm employs minimalism as a mechanism to promote portability.

I don't, however, really understand it very well. Some of the preceding steps of the build call rosdep to install the dependencies, so it's still not clear to me why the run_tests step could not do the same...

[peci1]

catkin_lint provides an --offline option. Does that already solve the issue for the ROS buildfarm?

With --offline, no checking of rosdep keys is done.

[gavanderhoorn]

Taking a step back: what is it you actually want to test @peci1?

"checking of rosdep keys" sounds like what a pre-release test is supposed to be used for, not a devel task running on the buildfarm.

[peci1]

what is it you actually want to test @peci1?

Simply said: I'd like the automated tests to run catkin_lint -W2 ..

"checking of rosdep keys" sounds like what a pre-release test is supposed to be used for, not a devel task running on the buildfarm.

Agree. But how to distinguish between these two in CMakeLists? And it should definitely work in pull-request-testing tasks, which I haven't investigated yet - but I guess they would work very similar to devel tasks.

[gavanderhoorn]

But how to distinguish between these two in CMakeLists?

that's not what I meant.

The build script is not the place to detect what is being run, it should just build the package.

The build environment setup by the pre-release test script(s) is what makes the build a pre-release test. Not the fact a special path is taken/executed in your build script.

[peci1]

Actually, this problem manifests in pre-release tests, devel tasks and release tasks. In all three of them, the environment is set up so that the package checks in catkin_lint cannot run the package checks. With 9acaa0e it should at least not result in total build failure, but it would instead "silently" skip the package tests. By "silently" I mean the build would succeed, a warning will be written to the build log, but really, who does read build logs until something fails?

I would still prefer having an option to run the package checks in all cases, including local development (where it normally works).

[roehling]

I think this is something that should be addressed in the ROS build farm. catkin_lint is just a rosdep consumer, and not in the business of setting up a working rosdep infrastructure. Besides, even if catkin_lint supported private rosdep caches as you propose, it will create new problems:

1. Where will the rosdep files be stored? You cannot overwrite arbitrary folders outside the build tree, so it has to be in the package build folder. This means every package using your macro will download a separate copy, which scales really poorly for larger workspaces.
2. There are build systems which download all dependencies before the build and deliberately restrict internet access during the build, to ensure self-containment. Your solution will break them.
3. What happens if a user simply forgot to run rosdep update and has additional sources configured in /etc/ros/rosdep/sources.list.d? catkin_lint will "helpfully" download its own cache and silently ignore the correctly configured one.

[peci1]

Ok, now I got convinced that the solution I proposed here is probably not really a good way. You're right that downloading and updating rosdep cache is definitely not a task for catkin_lint. However, as a "consumer of rosdep", it should contain a way to configure path to the rosdep cache. That is currently not supported. But that might change if ros-infrastructure/rosdep#908 would get merged - that would add an environment variable through which the cache location can be specified, requiring no code changes from catkin_lint.

Alternatively, I could squeeze #109 to just the --rosdep-cache-dir option of catkin_lint. That would not require ros-infrastructure/rosdep#908 and would make it right away possible to correctly configure caktin_lint to use a different cache location. Would this be a good compromise? (the option might get a bit redundant if ros-infrastructure/rosdep#908 gets merged in the future, though, so it might make sense to wait a few weeks to see whether there is will to accept the rosdep change).

[roehling]

I guess that would be feasible, as long as the PR is not too intrusive and does not change the default behavior when the option is omitted.

[peci1]

Okay, I'll update #109 accordingly. Thank you all for the insightful discussion.

[roehling]

I'm closing this issue, now that the discussed changes have been implemented and released in version 1.6.22