Added search command to search for rosdep keys.#997
Conversation
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: |
peci1
left a comment
There was a problem hiding this comment.
I've tested this locally and it works very nice! Just one more bug I found, you have a fix for it as a suggestion.
I've tested on U20.04/Noetic and tried searching both Noetic and Jazzy packages (via --rosdistro). Both worked as expected.
|
Once this is merged, it should be followed by a flood of tutorial/docs updates :) |
…ose packages. Co-authored-by: Martin Pecka <peci1@seznam.cz>
|
@cottsay any chance you could review this or assign someone who can do a review? |
|
Thanks for the contribution @StefanFabian I ran the tests again to make sure we're good. Scott is busy with Kilted so I pinged @christophebedard to do a quick second review. Let's get this out the door. |
|
Ah, yes. As I see it, there are three options. Which would you prefer? |
|
@StefanFabian This is just my preference but option number 3, " Make regex optional by using a try-catch import and falling back to a search using only re" seems like it has the lowest maintainer overhead with the most cross platform support. |
|
Sorry for the delay. I'll try to take a look tomorrow |
|
If it works with the built-in |
Although I see that fuzzy matching is just not (easily) possible with the built-in |
christophebedard
left a comment
There was a problem hiding this comment.
Some minor comments.
I think I understand the difference between "keys" and "packages" (i.e., searching actual rosdep keys vs searching what a key might point to), but I think it might be a bit confusing to users if it's not explained. That could be left for some eventual user documentation.
| rosdep search <searchstrings>... | ||
| search for a key in the rosdep database. E.g. using the system package name. | ||
|
|
There was a problem hiding this comment.
From what I understand, you can provide multiple "search strings" and the potential results have to match (fuzzy or not) all of them: it's not an OR, it's an AND. I think rosdep is pretty unclear about a lot of its options and commands, so I'd prefer to make that crystal clear here 😁
Also, I think making it clear that this searches system packages and ROS packages (even though it's implied by "key in the rosdep database") would also help.
Lastly, we should mention somewhere (possibly in user documentation and not here) that providing --rosdistro= is required in order to search ROS packages. I know this is always the case for rosdep, but again this is often unclear to users, especially new users.
There was a problem hiding this comment.
Are you sure --rosdistro= is necessary?
I can find ROS packages just fine without it.
I will make the help string more clear 👍
There was a problem hiding this comment.
It gets filled from ROS_DISTRO env var.
There was a problem hiding this comment.
Good point, but it's not available if you haven't sourced ROS (ros_environment) in your terminal.
Anyway, like I was saying, this is the case for all commands that need a ROS distro. I just feel like this isn't very clear, but we can always make it clearer later.
| view = sources_loader.get_source(view_name=view_name) | ||
| if not isinstance(view, CachedDataSource): | ||
| if options.verbose: | ||
| print('Skipping non-cached source %s' % view_name) |
There was a problem hiding this comment.
Can we tell users what to do in this case?
There was a problem hiding this comment.
Honestly, I don't know what to do there.
I'm skipping them because I don't know the structure if they're not this type.
This print is purely for debugging purposes.
I think a typical installation wouldn't encounter it, at least I never did.
|
Oh, and some tests would be great! At least just for the main use-cases. |
Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>
|
@christophebedard Thank you for the review! Let me know if there's anything else that needs to be improved. |
|
There are new flake8 errors caused by unnecessary It would be simpler to just fix them in this PR. Could you do that? |
| results = [regex.search(pkg) for regex in regexes] | ||
| if all(results): | ||
| error = count_errors(results) | ||
| close_pkgs.append({'key': key, 'pkg': pkg, 'os': os_entry, 'error': error}) |
There was a problem hiding this comment.
I'm a bit confused by the intended or unintended use of the os info for packages:
$ rosdep search tracetools --rosdistro=rolling
WARNING: ROS_PYTHON_VERSION is unset. Defaulting to 3
Closest keys:
tracetools_image_pipeline
tracetools
tracetools_launch
tracetools_read
tracetools_test
tracetools_trace
tracetools_acceleration
tracetools_analysis
Closest packages:
ros2trace_analysis: ros/rolling/tracetools_analysis [osx]
Why is it only listing osx? As a user, I would think this means that this package is only available on macOS. Could we instead list all OSes, e.g., [osx, debian, rhel, ubuntu]? The code seems to be stopping after finding results for the first available OS.
There was a problem hiding this comment.
I've decided to stop when a match is found because usually it's not relevant if there's another os with a matching name, since you usually know what you are looking for.
The main use case here is also looking for the package name in your distro's package manager to find the associated key, and in that case, only that would match anyway.
Debian and Ubuntu, for example, would almost always both have the same results, and this would unnecessarily bloat the output.
If I were to also list the other OS, it would be confusing if I didn't also list the matching package name for each OS, and this could get quite a long list if the search string matches many packages.
There was a problem hiding this comment.
What about searching just for the currently used OS (or the one passed in --os) ?
There was a problem hiding this comment.
Yeah, I think that would make a lot of sense. Not sure what the proper way to do it is, but looks like you could use OsDetect + the OS override info.
I've decided to stop when a match is found because usually it's not relevant if there's another os with a matching name, since you usually know what you are looking for.
If the OS information doesn't really matter, it shouldn't be shown IMO.
There was a problem hiding this comment.
Okay, I have changed it to use the os information and only look through packages for the current OS.
This is also updated in the help string, and I've removed the information for which OS the package was found as this is now no longer relevant.
Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>
|
I think I've addressed all feedback. If there's something that still needs to be changed, please let me know. |
peci1
left a comment
There was a problem hiding this comment.
Great, thanks a lot, it works for me with the updates!
There was a problem hiding this comment.
Thanks for iterating! Looks good to me now.
@nuclearsandwich @cottsay I don't have permissions on this repo: could one of you approve the CI run and possibly review and/or merge this? Note that this PR includes the same changes as in #1004, since they were necessary to prevent CI from failing fast due to unrelated flake8 errors.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #997 +/- ##
==========================================
- Coverage 74.22% 71.76% -2.47%
==========================================
Files 44 44
Lines 3376 3488 +112
Branches 0 686 +686
==========================================
- Hits 2506 2503 -3
+ Misses 870 808 -62
- Partials 0 177 +177 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@StefanFabian can you take a look at the CI failure for the Run tests / pytest / pytest (macos-latest, 3.12) job? https://github.com/ros-infrastructure/rosdep/actions/runs/15075300653/job/42381233133?pr=997 |
|
@christophebedard I think it was caused by the search command using the os of the test runner. |
|
Thanks, that makes sense. @nuclearsandwich could you approve a new CI run? 😅 |
|
CI looks good. Just waiting on an additional review or merge, but people are a bit busy. |
cottsay
left a comment
There was a problem hiding this comment.
I didn't do a close review here, but I don't see anything dangerous.
Thanks for your contribution!
Co-authored-by: Martin Pecka <peci1@seznam.cz> Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com> Co-authored-by: Steven! Ragnarök <steven@nuclearsandwich.com>
|
Any timeline on when the next release can be expected? @christophebedard @cottsay @kscottz |
|
Not sure about the timeline, but @nuclearsandwich is working on it. |
I've had to open the rosdep repo one time too many to look for a package key because I couldn't guess the correct one.
Hence, I've added a search command for rosdep which fuzzy searches the sources for a key or system package matching the search terms.
It allows one error per search term provided after
rosdep search.This could probably be made a parameter but typically this should be enough.
Examples: