Comments (10)
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.
from rospack.
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?
from rospack.
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.
from rospack.
@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?
from rospack.
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.
from rospack.
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?
from rospack.
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.
from rospack.
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.
from rospack.
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.
from rospack.
I'm almost ready to submit a PR. There is no
noetic-devel
branch 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.
from rospack.
Related Issues (20)
- Avoid Python linkage? HOT 6
- Calling "mkstemp" without securely setting umask first HOT 5
- Coverity - code unreachable HOT 1
- Coverity - TOCTOU time-of-check, time-of-use race condition HOT 2
- rospack cache files created with 000 permissions HOT 6
- Depends incorrectly fails for 'non-existant' package libuvc HOT 4
- rospack depends on rosdep HOT 5
- rospack picks up the wrong opencv libs HOT 3
- popen problem on qnx HOT 6
- TinyXML(2) might cause problems with the XSD schema now often attached due to the new package format (V2) HOT 26
- Require specific versions of tinyxml2 HOT 5
- New release? HOT 1
- New release for kinetic HOT 4
- $rospack find throws exception error HOT 4
- 'boost/tr1/unordered_set.hpp' file not found HOT 6
- "Unsupported Pickle Protocol: 4" when calling rospack depends returning HOT 1
- ../
- Kinetic release with #97 included (ros_environment dep) HOT 2
- rospack depend does not resolve distribution dependencies HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rospack.