rospack depends should list all (un)resolved dependencies when it exits with non-zero
Closed this issue · 10 comments
We have a debian package build script that uses rospack depends to gather the dependencies it has to build.
However, some packages may have dependencies that are not ROS packages, e.g., moveit_planners_ompl which depends on ompl.
$ rospack depends moveit_planners_ompl
[rospack] Error: package 'moveit_planners_ompl' depends on non-existent package 'ompl'
and rosdep claims that it is not a system dependency. Check the ROS_PACKAGE_PATH
or try calling 'rosdep update'
While rosdep can resolve ompl, it is not in the keys collection since it's not a ROS package.
The current behavior is to exit prematurely with an error which is not really optimal.
I would suggest to either add an argument to ignore the errors (since there is already a parameter to do so in the relevant method) or improve the lookup to not fail in such cases.
I think it would also be better to print errors to the error stream and continue instead of throwing but that would be a significant change in behavior.
If you tell me which changes would be preferred, I would be happy to submit a PR.
Alternatively, I assume we would have to use rosdep keys and do the recursion on our own?
The dependencies in a package manifest must be valid rosdep keys. That can be either:
- a ROS package released in the specific ROS distribution or
- a named mapped to system packages on the specific platforms
ompl falls into the first category: see https://github.com/ros/rosdistro/blob/ff15ecbf4e4eb3b52273ddbda31e0b7ebca5a77a/melodic/distribution.yaml#L5348
In general if any recursive dependency is not resolvable it makes sense to return with a non zero code to indicate that something is wrong. Relying on reading the stderr to determine if something failed isn't the right approach.
So the behavior you are seeing is intentional and I don't think it should be changed.
The question is why rospack depends moveit_planners_ompl fails even though ompl is a valid rosdep key? The packages in the first category are identified by their package manifest which is found by crawling the ROS_PACKAGE_PATH. It seems that the ompl package doesn't install the package.xml file. I created a ticket in the corresponding repo for this: ros-gbp/ompl-release#15.
Yes, I didn't mean to say that you shouldn't return an error code.
However, I do think it should do a best-effort result as well.
moveit_planners_ompl is most likely not the only package with a dependency that can not be resolved.
I don't see any reason why rospack should just fail with no usable output instead of continuing and possibly printing multiple errors before returning a non zero exit code.
The reason why stderr should be used for error output is that it can be easily ignored and wouldn't require more complex filtering of the program output.
In our use case, we would just route the error output to /dev/null, perhaps print a warning to run rospack depends FAILED_PACKAGE if the exit code was non-zero and do further work filtering on the packages returned by rospack depends.
Currently, if rospack depends fails, I have to recursively call rosdep keys which is really cumbersome and has a lot of overhead.
If you don't want such a behavior as default, I still don't see why it couldn't be an option such as --continue-on-errors or something like that?
I do think it should do a best-effort result as well.
That sounds good. I will reopen the issue and updated the title accordingly. Please consider contributing a pull request to implement the enhancement you would like to see.
@dirk-thomas I have added the necessary changes. However, before I'll make a PR, I'd like to know if there is any (scripted) way of testing the functionality to make sure everything still behaves as expected?
The tests of the rospack package should pass. A CI build testing that will also run for each PR. Beyond that I think it would be good to manually try that multiple error are now reported (instead of only the first). It would be good to also create a unit test for this so that we make sure this doesn't regress in the future.
I assume catkin run_tests --no-deps --this is the correct way to run the tests?
I'm only familiar with rostest.
Regarding the additional unit test, I can only add a test that checks whether normal output is given despite an exit code != 0 without breaking ABI compatibility for all compilers because to test whether it also reports multiple errors the log function signature has to be changed to virtual to allow for a mock override.
Changing the behavior from exiting after an error to continuing already requires changing the return type of some functions from void to bool which is not an issue with gcc but I believe breaks ABI compatibility for MSVC (because the latter encodes the return type in the name mangling).
If you want to keep ABI for all compilers, I would have to create copies of the methods which would be a very compatible but also ugly solution.
Would you prefer complete ABI compatibility or is ABI compatibility on most compilers for Unix/Mac systems sufficient?
Regarding the additional unit test, I can only add a test that checks whether normal output is given despite an exit code != 0 without breaking ABI compatibility for all compilers because to test whether it also reports multiple errors the log function signature has to be changed to virtual to allow for a mock override.
How about calling the not-mocked API on a test directory which actually reports more than one failure?
If you want to keep ABI for all compilers, I would have to create copies of the methods which would be a very compatible but also ugly solution.
I would say first start with an ABI changing PR which targets the upcoming Noetic distro. And then we can discuss if it makes sense to go through the extra effort to make this available in an ABI stable fashion for already released distributions.
How about calling the not-mocked API on a test directory which actually reports more than one failure?
That would just test whether the exit code is correct and it still generates normal output.
The number of errors is in that case completely transparent.
I can't test whether it actually outputs any errors because that would require capturing the error stream. I tried to do that once with gtest+rostest but couldn't get it to work. And as far as I know, with printf it gets even more complicated. Hence, mocking log would be a clean solution to test whether errors are actually logged.
I'm almost ready to submit a PR. There is no noetic-devel branch yet, though.
However, I've noticed that depends-on1 will now generally return an exit code of 1 because I assume it computes the deps for all packages and these contain some invalid packages (in my workspace and in the test workspace).
The current behavior is to ignore errors, so my question is whether I should just ignore the result and return an exit code of 0 regardless of what happens.
Since the exit code would, in that case, be meaningless since there is no error message, I assume that ignoring the result would make sense but maybe I'm missing something.
I'm almost ready to submit a PR. There is no
noetic-develbranch yet, though.
Just create the PR against the current default branch and add a note that the change should only be merged into a noetic specific branch.
I can't recommend anything for the tests atm since I simply don't have enough context. Fell free to choose something and I will look at it when reviewing the PR.