-
Notifications
You must be signed in to change notification settings - Fork 170
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
Return non-zero if rosdep check
cannot locate dependent
#948
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
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. |
rosdep check
cannot locate dependentrosdep check
cannot locate dependent
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.