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

Fix: Missing dependency that causes cmake error in downstream (resolves https://github.com/ros2/rosidl_python/issues/198) #199

Merged
merged 3 commits into from
Aug 4, 2023

Conversation

130s
Copy link
Contributor

@130s 130s commented Jul 14, 2023

Issue aimed at

Changes

  • Add a missing package in package.xml.
    • Even if that the pkg in question is only required conditionally Missing dependency on ament_cmake_flake8? #198 (comment), it makes more sense to me in the upstream (here) to install it than making it a job for downstream pkgs.
    • That said, I have little idea if the implementation I'm suggesting is the best/reasonable. Open for advises.

@130s 130s requested review from quarkytale and sloretz as code owners July 14, 2023 22:14
@quarkytale
Copy link
Contributor

Please add signature to your commit git commit -s

@130s 130s force-pushed the fix-missing-dep branch from c2f4386 to 5b36dd0 Compare July 14, 2023 22:25
@130s
Copy link
Contributor Author

130s commented Jul 14, 2023

@quarkytale Thanks, done.

ros2#198)

Signed-off-by: Isaac Saito <130s@2000.jukuin.keio.ac.jp>
@130s 130s force-pushed the fix-missing-dep branch from 5b36dd0 to ad6769f Compare July 14, 2023 22:34
rosidl_generator_py/package.xml Show resolved Hide resolved
130s added a commit to 130s/rosidl_python that referenced this pull request Jul 14, 2023
…ment ros2#199 (comment))

Signed-off-by: Isaac Saito <130s@2000.jukuin.keio.ac.jp>
@130s 130s requested a review from sloretz July 14, 2023 23:21
@@ -37,6 +37,8 @@
<exec_depend>rosidl_runtime_c</exec_depend>
<exec_depend>rpyutils</exec_depend>

<test_depend>ament_cmake_flake8</test_depend>
<test_depend>ament_cmake_pep257</test_depend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, oops I wasn't requesting any changes. I think you were correct at first that these need to be exec_depend to make sure downstream message packages depend on them.

I was only commenting to say the current package.xml depend declarations don't cover this case. We would need to create one, probably called test_export_depend, to support it better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, reverted.

#199 (comment)

Adding them as exec_depend means that they'll always be installed when a message package is installed, but these dependencies are probably light enough that this is fine.

It's a bit surprising no one (especially from those who are making products) has requested test_export_depend-like functionality. I can think of huge test_depend-ed packages in an upstream package.

@130s 130s force-pushed the fix-missing-dep branch from 9b4c1df to ad6769f Compare July 14, 2023 23:27
@130s 130s requested a review from sloretz July 14, 2023 23:32
…ment ros2#199 (comment))

Signed-off-by: Isaac Saito <130s@2000.jukuin.keio.ac.jp>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change to the comment here, then I'm happy with this.

rosidl_generator_py/package.xml Outdated Show resolved Hide resolved
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
@130s 130s requested a review from clalancette August 3, 2023 15:55
@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit 0945b61 into ros2:rolling Aug 4, 2023
@130s 130s deleted the fix-missing-dep branch August 4, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing dependency on ament_cmake_flake8?
4 participants