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

Collect p.buildtool_export_depends when parsing the manifest file #191

Closed
wants to merge 1 commit into from

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented Mar 6, 2020

I am not really sure if it is my usage problem or a bug.

Here is a example how I use rospkg.

import rospkg

# assuming it is a directories with all ROS 2 sources.
rospack = rospkg.RosPack([base_dir])
print(rospack.get_depends('ament_cmake'))

And in this case, I would expect ament_cmake_include_directories in the resulting list. However, it is not, and it seems to me that parse_manifest_file doesn't take buildtool_export_depends into consideration, which smells like a bug to me.

@seanyen
Copy link
Contributor Author

seanyen commented Mar 6, 2020

@dirk-thomas @tfoote I am not sure if the behavior I saw is expected or not. And hope you can take a look and triage my fix. Thanks!

@dirk-thomas dirk-thomas added the bug label Apr 2, 2020
@dirk-thomas
Copy link
Member

dirk-thomas commented Apr 2, 2020

run_depends is an emulated property in catkin_pkg. See how it is being aggregated from other dependencies: https://github.com/ros-infrastructure/catkin_pkg/blob/67af339ad6d419e943b4ed12507dbab072b5e0b7/src/catkin_pkg/package.py#L115-L121

I think buildtool_export_depends was intentionally left out since it is for dependencies on the build platform (rather than the target platform) as all other run dependencies.

The impact of this change is hard to foresee though. I am worried that this change of behavior having might have negative impact in other cases (e.g. tests failing which passed before). It would be good to perform a full build and test on a larger set of packages (like ros_core or even ros_base).

@dirk-thomas
Copy link
Member

I just noticed that you are using rospkg with ROS 2. rospkg was developed for ROS 1 and is not used in ROS 2 at all. One reason is that its API was designed even before package.xml files with their various dependency types existed.

@seanyen So I am wondering what you are trying to achieve and why you aren't using catkin_pkg instead?

@seanyen
Copy link
Contributor Author

seanyen commented Apr 7, 2020

@dirk-thomas Sorry for my late reply on this. I was using rospkg to collect the build and run requirements for both ROS1 and ROS2, but after hitting this issue, I switched over using catkin_pkg instead. So far catkin_pkg is doing well for what I needed and so I will close this pull request.

@seanyen seanyen closed this Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants