Update package.xml #18
Conversation
|
@audrow Could you review? |
package.xml
Outdated
| @@ -15,6 +15,12 @@ | |||
|
|
|||
| <buildtool_depend>ament_cmake_core</buildtool_depend> | |||
|
|
|||
| <build_depend>rosidl_default_generators</build_depend> | |||
There was a problem hiding this comment.
To match other message packages (e.g. https://github.com/ros2/common_interfaces/blob/master/diagnostic_msgs/package.xml), this should be a <buildtool_depend>
There was a problem hiding this comment.
Done. By the way, I copied this from the ROS 2 tutorial.
|
@audrow any chance this PR could be reviewed soon? If you don't have time, I can review it and merge it. Thanks. |
|
@nnmm thanks for addressing my feedback. The last commit is missing your signature and it's making the DCO test to fail, can you sign it? Thanks. |
I believe those three lines should be part of every package's `package.xml`. Signed-off-by: Nikolai Morin <nnmmgit@gmail.com>
|
Ah yes, my bad. Thanks! |
|
So this PR was merged without any CI being run on it, and it is causing us to not be able to build with circular dependencies. I'm going to open a revert PR, as right now we are totally dead in the water. |
This reverts commit f35d71b.
This reverts commit f35d71b. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
|
@clalancette Sorry for causing so much trouble with this! |
No worries, this should be fixed by #19. I think it is worthwhile to revisit what you were trying to accomplish here. You are entirely correct that for most interface packages, the package.xml lines you added are necessary. We never noticed/needed them here because we don't actually release this package onto the buildfarm, and so nothing complained. I think the problem with the lines you added is that it introduces a circular dependency between this package and rosidl, which is what colcon was complaining about. What was the original problem that caused you to examine this and try to add those lines? Maybe we can find another way to fix it. |
|
@clalancette The purpose was to use this package in ros2-rust/ros2_rust#80 for a demo executable. I was unable to generate Rust code for message definitions in this package and thought the missing dependencies might be the issue. It's probably not (only) the dependencies, but also the way CMakeLists.txt is written. We can always use a custom message package instead of |
|
I'm sincerely sorry for the premature go-ahead. The changes seemed perfectly logical and consistent with all other message packages, so I never imagined there would be such an edge case. The green check marks in the GitHub workflow also fooled me into thinking that the CI was okay. I'll make sure I'm familiar with the procedure of running the full CI on a PR in the future 🤦 |
|
@clalancette so sorry about that, I had forgotten about the buildfarm and only looked at the GitHub CI. Thanks for the prompt fix! |
I believe those three lines are required for every message package.