Skip to content
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

Fix location of rosdep cache dir #109

Merged
merged 1 commit into from
Jan 8, 2023
Merged

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Dec 30, 2022

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).

# Python 3
from io import StringIO
stdout = sys.stdout
sys.stdout = StringIO()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we still supporting Python 2? 😲

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

But not in catkin_lint_cmake...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@MatthijsBurgh MatthijsBurgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

peci1 commented Jan 2, 2023

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
Copy link

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
Copy link
Contributor Author

peci1 commented Jan 2, 2023

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
Copy link

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
Copy link
Contributor Author

peci1 commented Jan 2, 2023

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
Copy link
Contributor Author

peci1 commented Jan 2, 2023

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
Copy link
Member

roehling commented Jan 2, 2023

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.

roehling added a commit that referenced this pull request Jan 2, 2023
See #109 for the corresponding discussion
@peci1
Copy link
Contributor Author

peci1 commented Jan 2, 2023

@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
Copy link
Member

roehling commented Jan 2, 2023

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

@peci1
Copy link
Contributor Author

peci1 commented Jan 2, 2023

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

roehling added a commit that referenced this pull request Jan 3, 2023
See #108 and #109 for the corresponding discussion
@peci1 peci1 marked this pull request as ready for review January 8, 2023 14:48
@peci1
Copy link
Contributor Author

peci1 commented Jan 8, 2023

@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).

@peci1 peci1 changed the title Allow specifying custom rosdep paths Fix location of rosdep cache dir Jan 8, 2023
@roehling roehling merged commit 9c6a16f into fkie:master Jan 8, 2023
@roehling
Copy link
Member

roehling commented Jan 8, 2023

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
Copy link
Contributor Author

peci1 commented Jan 8, 2023

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
Copy link
Member

roehling commented Jan 8, 2023

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

@peci1
Copy link
Contributor Author

peci1 commented Jan 8, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants