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

Add support for --sources-cache-dir to update verb #845

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Dec 16, 2021

From what I can tell, support for this option was only implemented for the install verb.

From rosdep update --help:

  ...
  -c SOURCES_CACHE_DIR, --sources-cache-dir=SOURCES_CACHE_DIR
                        Override ~/.ros/rosdep/sources.cache
  ...

@cottsay cottsay added the bug label Dec 16, 2021
@cottsay cottsay self-assigned this Dec 16, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2021

Codecov Report

Merging #845 (9850258) into master (45290f4) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #845   +/-   ##
=======================================
  Coverage   75.00%   75.00%           
=======================================
  Files          42       42           
  Lines        3317     3317           
=======================================
  Hits         2488     2488           
  Misses        829      829           
Impacted Files Coverage Δ
src/rosdep2/main.py 48.94% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45290f4...9850258. Read the comment docs.

@nuclearsandwich
Copy link
Contributor

I am torn between approving this as-is since it's a net improvement and concern that improving the piecemeal support for this option is going to increase its usage and create more inconsistency.

Looking back it seems like this feature, introduced in 2c5cae3 is meant primarily (possibly exclusively) for testing.

looking for usage of get_sources_cache_dir() in the codebase turns up the following:

default_sources_cache = get_sources_cache_dir()

This is the fetching of the sources list for use as the argument default so that's fine.


def init_rospack_interface():
class Options(object):
def __init__(self):
self.os_override = None
self.sources_cache_dir = get_sources_cache_dir()
self.verbose = False
self.dependency_types = []
lookup = _get_default_RosdepLookup(Options())
return lookup.get_rosdep_view(DEFAULT_VIEW_KEY)

I haven't figured out if this is dead code or not but if it's at all usable right now it won't be affected by this patch without further work.


if sources_cache_dir is None:
sources_cache_dir = get_sources_cache_dir()

This usage is the initialization of an optional kwarg sources_cache_dir for the update_sources_list function.

if sources_cache_dir is None:
sources_cache_dir = get_sources_cache_dir()

This usage is likewise the initialization of an optional kwarg sources_cache_dir for the load_cached_sources_list function.


All uses of the latter two functions within rosdep2 pass the optional kwarg. I think the smallest delta from this patch which would give me the confidence to approve it would be to make the kwarg required (or at least start throwing warnings up when no sources_cache_dir is provided)

If we just ignore rospack (init_rospack_interface is not called within rosdep2 at all) then I think this could go through.

If we wanted to do something more holistic like a refactoring which creates a single context object for the sources list directory, sources cache directory, and other similar parameters which should persist through all rosdep operations and use that ubiquitiously I could see that being a good direction to go as well.

As pointed out, it doesn't seem like this has ever been implemented for update operations, which at least means that we've only ever written to a consistent incorrect default directory but
without something to help assert that we would reliably be using the configured cache directory for all operations I think that starting to add support could just make an old problem new and potentially worse as we could be writing to inconsistently incorrect directories.

If this is all "a lot" and we want to punt, I think we can do an even smaller thing and just suppress the help for this flag and consider it just for testing (as it is used in tests).

@peci1
Copy link

peci1 commented Jan 2, 2023

A more complete solution is provided in #908 . It was not only the sources cache, but also the meta cache that goes by default into ~/.ros/rosdep. I've added options to configure both - either via CLI options, or via a single env variable ROSDEP_CACHE_PATH.

@peci1
Copy link

peci1 commented Jan 2, 2023

I found three major references to init_rospack_interface on github:

  1. https://github.com/ros-infrastructure/rospkg/blob/721a25c7da39837655f43daeae2820422341d215/src/rospkg/manifest.py#L414
  2. https://github.com/ros/rospack/blob/ad85a874575bbed74124b722b42b545537cc6aa3/src/rospack.cpp#L1733
  3. https://github.com/lagadic/robotpkg/blob/065f4f94160362c43659f9798dbdad4c491fc977/sysutils/py-rospkg/patches/patch-aa#L1-L16

ad 3) This only disables rosdep usage, so no problem.

ad 1 and 2) In both cases, the ROSDEP_CACHE_PATH environment variable introduced in #908 would work out of the box. Whereas the added options (-c from this PR, or both -c and -m from #908) would need more downstream work.

So I agree with @nuclearsandwich that the wisest option would probably be to hide -c and -m from the help message and keep them for usage in tests. The only user-facing interface should be the ROSDEP_CACHE_PATH environment variable.

Would that be acceptable?

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

Successfully merging this pull request may close these issues.

4 participants