-
Notifications
You must be signed in to change notification settings - Fork 96
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
Run rosdep init and update in build_and_test devel tasks #987
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@cottsay You had a comment #923 (comment) against this kind of fix, but I did not quite understand it. Could you check whether it still applies to this PR? |
I would say that the comment still applies. A populated rosdep cache isn't something you can declare a dependency on from a deb or RPM package, so it isn't something we should pollute the "pristine" build environment with. There is also a fair amount of ambiguity about how "fresh" the cache might be. For at least the past decade there have been environment variables for controlling the rosdep cache location and sources list file location. I think it would be reasonable that if your test required a populated rosdep cache, a test fixture should be made to ensure that the data is available. It could initialize and download the data into a temporary directory for the test to use without modifying the user's directory or system's
I disagree. At the very least, rosdep is not used on Windows at all. What is |
I only suggest adding it for tests.
The tests Docker image is built right before running, so it would always be fresh.
Not true. That would need ros-infrastructure/rosdep#908 getting merged.
That would make the tests dependent on Internet connection. I don't think it's necessary.
The In my view, when you run the test locally, it just uses your global rosdep cache as you have it (thus not requiring internet connection), and on the buildfarm, it should use a cache downloaded when building the docker image (thus also not requiring internet connection when actually running the tests). And if you want to be super-sure it uses a clean rosdep cache, with ros-infrastructure/rosdep#908 you could just create a temporary directory, download the cache in it and set
I hope you're joking :-D |
This pytest code works in both Python 2 and Python 3 and uses only rosdep public API. I believe this approach has been possible since about 2014. https://gist.github.com/cottsay/549deca7a6664cebbfc3cf42a3421f6e
👎 |
Hmm, and would it also move the meta.cache dir? |
This approach redirects all of the content in
If there was already a usable rosdep cache on the system, that example code will no-op. |
Great. So should it be sufficient to prefix both lines added in this PR by |
I created that example to demonstrate that no changes to any of the tooling are required for a test to use data from the rosdep DB and/or rosdistro index. If a package's tests require internet resources, it need not rely on the user to acquire and cache them for the test to run - the test can acquire the resources itself. If you were to take the example code I linked, remove the demo You could re-use this same workflow in GTest or even create a wrapper script for invoking a subprocess the needs rosdep. All without touching the sources of |
I don't think that reusing the global cache (however old it is) can be called breaking the dependency on the host environment. This can actually make things much worse if it's too out-of-date. Looking in more detail to the proposed test fixture and the overall approach using Try to look at it from the maintenance point of view - 2 or 3 lines of code here could completely resolve the issue for all downstream packages. Your suggestion basically throws the ball to the downstream packages which would need to implement complicated setups just for the sake of satisfying ROS buildfarm. I still think populated rosdep cache is something a ROS package can count with on relevant platforms. |
Init and update rosdep database before running tests in devel tasks.
Having an environment with non-initialized rosdep database is (IMO) invalid, as the commands to init and update rosdep are present in every tutorial to installing ROS, even if no packages are intended to be built on the host system.
Fixes a part of #923 (and e.g. running catkin_lint directly from a CMake test target as reported in fkie/catkin_lint#108).