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

Return non-zero if rosdep check cannot locate dependent #948

Merged

Conversation

SubaruArai
Copy link
Contributor

This fixes #937.

As for the tests, I concluded it'll be quite difficult to implement with the current architecture.
The reason being the data in ./test/tree is expected to succeed, see here.

Please tell me if there's a good way to inject a non-functioning package, I'll add a test.

Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

This seems like the right call. An unresolveable rule is arguably a more critical error than an uninstalled package.

src/rosdep2/main.py Outdated Show resolved Hide resolved
@cottsay cottsay requested a review from tfoote April 10, 2024 19:25
Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Catching these errors seems like a good idea. We may want to add a command line flag to keep the old permissive behavior to allow someone who was relying on it to keep using it.

src/rosdep2/main.py Outdated Show resolved Hide resolved
@SubaruArai SubaruArai requested review from cottsay and tfoote April 11, 2024 00:48
@SubaruArai
Copy link
Contributor Author

@cottsay @tfoote I've changed the code, please review.
Somehow github insists there's still a change requested - please ignore it.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

That review status doesn't go away until. The review has been dismissed by a maintainer or changed by the reviewer.

This is very simple now and will avoid what used to be false negatives.

@cottsay
Copy link
Member

cottsay commented Apr 16, 2024

We may want to add a command line flag to keep the old permissive behavior to allow someone who was relying on it to keep using it.

I think that providing a unique return code gives a developer the same ability to ignore the new behavior. Either way they'd need to change how they call the tool, but the unique return code is more widely usable than an explicit command line flag.

@cottsay cottsay changed the title Return 1 if rosdep check cannot locate dependent Return non-zero if rosdep check cannot locate dependent Apr 16, 2024
@cottsay cottsay merged commit c20520e into ros-infrastructure:master Apr 16, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rosdep check does not exit with status 1 for missing dependencies
3 participants