Fix location of rosdep cache dir

[peci1]

catkin_lint internally uses rosdep to check for existence of packages mentioned in package.xml. When running tests on ROS buildfarm, rosdep database is unfortunately not inited or updated, so any rosdep lookups fail. And there is no way to tell the buildfarm to init and update the db before running tests.

So currently, any code that wants to use catkin_lint in run_tests and should be released via ROS buildfarm, needs to do some non-trivial checks to see if there is a rosdep database cache, and in case there isn't, catkin_lint has to be disabled (or at least checking package existence should be disabled via a flag).

One way around this limitation is using a custom (non-root writable) location for the rosdep db and cache and downloading the cache files again every time the tests are run (this can be later optimized via CI cache storage, if the folder name is predictable).

So this PR is a first step towards making it possible to use catkin_lint on ROS buildfarm. The other part would be implemented in the CMake calling code (and I'm thinking about submitting another PR with the CMake support similar to https://github.com/tue-robotics/catkin_lint_cmake).

[MatthijsBurgh]

Use contextlib.redirect_stdout

[peci1]

I'd like to, but that's not available in Python 2.7 :(

[MatthijsBurgh]

Are we still supporting Python 2? 😲

[peci1]

Pretty well, actually :) Melodic isn't dead yet =)

But not in catkin_lint_cmake...

[MatthijsBurgh]

No, I decided to not support Python 2 in a project I created significant time past the EOL of Python 2.

[peci1]

That's completely okay you decided to abandon Melodic. And that's why I wanted to pull a (simpler) CMake support directly into catkin_lint.

[MatthijsBurgh]

I have no clue whether this entire hassle is really needed for ONLY the ROS buildfarm.

Would it not be an better/simpler option to expose the use_rosdep.

[peci1]

Would it not be an better/simpler option to expose the use_rosdep.

Well, the buildfarm actually checks for the dependency existence in other ways, so the check from catkin_lint is redundant. However, if you test locally before releasing, the check from catkin_lint is very useful. I haven't yet found a way to reliably detect whether the build is running on ROS buildfarm or not, so it is difficult to make a conditional disabling the checks on buildfarm.

[MatthijsBurgh]

Would it not be an better/simpler option to expose the use_rosdep.

Well, the buildfarm actually checks for the dependency existence in other ways, so the check from catkin_lint is redundant. However, if you test locally before releasing, the check from catkin_lint is very useful. I haven't yet found a way to reliably detect whether the build is running on ROS buildfarm or not, so it is difficult to make a conditional disabling the checks on buildfarm.

I think having a way of detecting it, is the only way to get things working, isn't it? How do you otherwise determine the custom paths which you need to provide to catkin_lint?

[peci1]

I think having a way of detecting it, is the only way to get things working, isn't it?

The best (still indirect) way I found is checking whether the rosdep cache folder exists. If not, then it is probably the buildfarm or some other weird environment.

How do you otherwise determine the custom paths which you need to provide to catkin_lint?

When using the functionality added in this PR, you won't determine them, you will specify them. Either in /tmp, or in CMAKE_CURRENT_BINARY_DIR. In my view, that is the only way to provide an actual check of the packages even on the buildfarm.

So the proposed solution would not introduce anything that would be buildfarm-specific - it would be a general solution for all environments without initialized rosdep database (which, maybe, is only the buildfarm, though). I like this much more than some hacks, e.g. examining hostname and guessing whether it is one of the buildfarm hosts, or checking for the TRAVIS env variable.

[MatthijsBurgh]

I like this much more than some hacks, e.g. examining hostname and guessing whether it is one of the buildfarm hosts, or checking for the TRAVIS env variable.

I think this is totally fine, when done in a wrapper, CMake macro, etc..

[peci1]

I think this is totally fine, when done in a wrapper, CMake macro, etc..

This is probably where we won't agree...

However, can you identify what exactly is the "hassle" you don't like about this PR? Is it adding the 4 arguments? Alternatively, all of them could be squeezed in a single one, say --rosdep-base-path. If this argument would be specified, both sources list and cache dir would be set under this directory, and init and update could be called automatically/as needed. This would be a bit less configurable than with the 4 arguments, but maybe easier to understand for users...

[peci1]

I also try to "attack" this problem from the other side - rosdep+buildfarm : ros-infrastructure/ros_buildfarm#923 (comment) . But in my view, the chances of those changes to be merged are not very high...

[roehling]

So currently, any code that wants to use catkin_lint in run_tests and should be released via ROS buildfarm, needs to do some non-trivial checks to see if there is a rosdep database cache, and in case there isn't, catkin_lint has to be disabled (or at least checking package existence should be disabled via a flag).

Turns out this is actually not that difficult to detect for catkin_lint itself: if rosdep update has not been called, the resulting view has no packages in it, which I can treat as failure condition.

[peci1]

@roehling Detecting the error condition is good, but I'd still rather get a functioning catkin_lint. With the change you did, catkin_lint would still fail on the buildfarm...

[roehling]

Why do you think that? catkin_lint will simply ignore unknown packages and work fine otherwise.

[peci1]

Ahh, right, you didn't change the exit code. But still, being able to check the packages would be even better :)

[peci1]

@roehling I see you were faster than me with integrating the changes :) There is, however, a small bit that would be better fixed. Rosdep uses several cache folders - so far the sources cache and meta cache. catkin_lint has so far only "depended" on the location of the sources cache, but it is possible that there would be other cache directories added in the future, or that it will start being important to also point to a proper meta cache location. Fortunately, both sources cache and meta cache reside in a common parent directory. It'd seem to me more future-proof to configure this parent directory as the base for all caches. And that's also the same interpretation that ROSDEP_CACHE_PATH has in my rosdep PR. I updated this PR to do this little change.

I also see you started refereing to ROSDEP_CACHE_PATH env var in the documentation. However, the rosdep PR adding this variable has not yet been merged (or, even started being discussed).

[roehling]

Thanks, merged.

Regarding the documentaiton: I chose my wording carefully that the command line option has the same effect as setting ROSDEP_CACHE_PATH (which is correct, because that is how I chose to implement the feature in catkin_lint). I deliberately did not claim anything about rosdep, so the documentation is correct whether your PR gets merged or not. Of course, if your PR gets merged, the rosdep implementation will effectively do the same thing as catkin_lint does (and I might even revert the change then).

[peci1]

Yes, that makes perfect sense. Do you actually plan a release into Bionic and Focal anytime soon? I see the last version released there is 1.6.15...

[roehling]

I have no write access to the Open Robotics repository. I open pull requests, and sometimes they get merged.

[peci1]

I was wondering where these updates happen as I haven't found it on rosdistro. Thanks :)