Allow full configuration of the cache directories used by rosdep via environment variable, deprecate --sources-cache-dir

[peci1]

Currently, the sources cache and meta cache location cannot be configured when calling rosdep update from command line. This could be helpful e.g. on the buildfarm or in CI scripts - it would then be possible to completely isolate the build from the host environment and filesystem - ROSDEP_SOURCE_PATH would define the sources list path for both init and update, and a new environment variable ROSDEP_CACHE_PATH would point to a parent directory for both sources and meta caches, used by update, install and most other verbs. If ROSDEP_CACHE_PATH is not specified, the behavior is as without this PR.

As mentioned in #845 (comment), the --sources-cache-dir CLI option was initially introduced mainly for testing purposes. And nobody noticed that there is a second cache that also needs relocating - meta cache. @nuclearsandwich was afraid in his comment that keeping track of correct usage of --sources-cache-dir would be difficult and it would lead to inconsistencies. Therefore, I have deprecated --sources-cache-dir in this PR (just by hiding it from the help message) - it should now be replaced by setting ROSDEP_CACHE_PATH. On the other hand, there were places in code that were accessing the meta cache always in its default location, so to complete the "feature parity", I also added a (right away deprecated) option --meta-cache-path. I'm not sure whether this is a good or bad thing and I'm ready to remove it if it's not desired.

ROSDEP_CACHE_PATH environment variable makes it pretty easy to keep consistency in code. All places in code that "do not care" about the cache location will get it correct through the default getters get_meta_cache_dir() and get_sources_cache_dir(). So it is no longer possible to get inconsistent behavior via non-deprecated parts of CLI (with the only exception when the user calls sudo rosdep init and passes the env var in a way that doesn't go through the sudo command). In Python API, it is still possible to pass around overrides of the cache paths. If the downstream code does that, it should know what it is doing. Currently, there might be some code calling update_sources_list() specifying only the sources cache path and not specifying the meta cache path - this might lead to "leaking" of a supposedly isolated program into the host filesystem. It might make sense to issue a warning if only one of the two caches is specified.

Locally, I tested the init, update and install verbs with ~/.ros/ directory set to readonly. When using ROSDEP_CACHE_PATH, the cache directories are correctly created in the specified directory and the default one is not touched. If I unset the variable, rosdep fails with permission denied error pointing to the default location. With this setup, I ran the whole test suite and discovered one test that relied on host filesystem contents, so I corrected the test to use a test directory instead.

Technical note: --sources-cache-dir pointed directly to a sources cache directory. ROSDEP_CACHE_PATH points to a parent directory, i.e. a directory that contains folder sources.cache (and also meta.cache). I don't see a problem in this. I just had to rename a folder in the test/ directory from sources_cache to sources.cache to comply with the suffix.

[peci1]

This PR would allow to use rosdep cache during tests on the ROS buildfarm while maintaining minimal interference with the host system: ros-infrastructure/ros_buildfarm#923 .

[nuclearsandwich]

Thank you for the detailed PR writeup! A cursory review of this PR shows me that it's very thorough.

I particularly appreciate the testing methodology, setting .ros to read-only in order to reveal off-spec cache usage is very promising. I'm going to bring this PR and my initial review to the next infrastructure team meeting to discuss and do a group review.

[peci1]

Thanks for looking at this and for bringing it to the team meeting. Let me know if there is anything else I could do in this regard.

[peci1]

I'm going to bring this PR and my initial review to the next infrastructure team meeting to discuss and do a group review.

Has the meeting already been held? :)

[peci1]

Friendly ping.

[nuclearsandwich]

I focused my review on the documentation rather than implementation.

Most comments are small formatting questions or suggestions which @peci1 may accept, modify, or decline at their discretion.

There is one documented behavior that I think should be changed, which I've described inline. As well as a request to document how precedence between the environment variables and command line flags should be handled.

Thanks for taking on this large feature!

[nuclearsandwich]

I would prefer that an "invalid" custom path produce an error rather than falling back to the default since that's probably not what the user intends if they're setting this value in the first place.

[peci1]

This is a behavior this PR hasn't touched. It might seem a bit unfortunate, but I fear changing it, as this behavior has been there for 9 years already.

The env var behaves as a path list. If at least one path from it exists, it is used. If it is empty or all paths do not exist, the system default is used.

[peci1]

@nuclearsandwich I followed your review and hopefully fixed all issues.

I needed to integrate #929 into this PR to make some tests succeed.

Could you please allow the workflow on this PR to get the buildfarm tests?