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

Remove action_tutorials_interfaces. #701

Merged

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Aug 29, 2024

We have this message duplicated three times in the core; once in here, once in example_interfaces, and once in test_interface_files. That seems a bit excessive, so remove this third copy and make the action_tutorials use the example_interfaces version of Fibonacci.action instead.

This change obviously changes the dependencies of the action_tutorials_cpp and action_tutorials_py packages, in that it removes action_tutorials_interfaces and adds in example_interfaces. I believe that this should be fine from an inter-repository standpoint, as the demos repository already contains other packages (composition, quality_of_service_demo_cpp, demo_nodes_py, demo_nodes_cpp) that depend on example_interfaces.

This will fix #702 by at least removing one of our duplicate copies of this message.

We have this message duplicated three times in the core;
once in here, once in example_interfaces, and once in
test_interface_files.  That seems a bit excessive, so
remove this third copy and make the action_tutorials
use the example_interfaces version of Fibonacci.action
instead.

This change obviously changes the dependencies of the
action_tutorials_cpp and action_tutorials_py packages,
in that it removes action_tutorials_interfaces and adds
in example_interfaces.  I believe that this should be
fine from an inter-repository standpoint, as the demos
repository already contains other packages (composition,
quality_of_service_demo_cpp, demo_nodes_py, demo_nodes_cpp)
that depend on example_interfaces.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

Pulls: #701
Gist: https://gist.githubusercontent.com/clalancette/f40892f15019e2125c75b9041490e888/raw/9eee2c252a0399a1fc03246677ea5fcd590e76fd/ros2.repos
BUILD args: --packages-above-and-dependencies action_tutorials_py action_tutorials_cpp
TEST args: --packages-above action_tutorials_py action_tutorials_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14488

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

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

so the expectation is that nobody in the downstream depends on this action-tutorials-interfaces, i believe that should be fine.

lgtm with green CI.

@clalancette
Copy link
Contributor Author

so the expectation is that nobody in the downstream depends on this action-tutorials-interfaces, i believe that should be fine.

Actually, this is a great question, and one I should have considered.

I took a look at all of the packages currently released into Rolling, and besides action_tutorials_py and action_tutorials_cpp (fixed here), the following depend on action_tutorials_interfaces:

I'll follow up with those projects to remove their use of action_tutorials_interfaces before we merge this in.

@clalancette
Copy link
Contributor Author

I'm going to go ahead and merge this one in. That will break simple_actions for now, but it should be easy enough to fix it later on just by merging the above PR. Thanks @ahcorde for the extra work here!

@clalancette clalancette merged commit 0f2afe5 into rolling Sep 6, 2024
3 checks passed
@clalancette clalancette deleted the clalancette/remove-action-tutorials-interfaces branch September 6, 2024 16:50
clalancette added a commit to ros-infrastructure/rep that referenced this pull request Oct 9, 2024
It was removed in ros2/demos#701 (since
it was a duplicate with some other packages), and then further
removed from the variants in ros2/variants#44,
so that should be reflected here as well.

Signed-off-by: Chris Lalancette <[email protected]>
clalancette added a commit to ros-infrastructure/rep that referenced this pull request Oct 9, 2024
…409)

It was removed in ros2/demos#701 (since
it was a duplicate with some other packages), and then further
removed from the variants in ros2/variants#44,
so that should be reflected here as well.

Signed-off-by: Chris Lalancette <[email protected]>
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.

code line does not match the official tutorial
3 participants