Skip to content

Add warning that actions require ament_cmake, not ament_python #4986 #5201

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

Open
wants to merge 13 commits into
base: rolling
Choose a base branch
from

Conversation

vimal0athithan
Copy link

No description provided.

@vimal0athithan
Copy link
Author

@Yadunund please check this

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the valuable improvement to our documentation.

Apart from the comments inline, I recommend updating the instructions to create the package to specify that ament_cmake should be the build_type.

      ros2 pkg create --license Apache-2.0 --build_type ament_cmake custom_action_interfaces

Comment on lines 45 to 46
Actions can only be created in a C++ package using ``ament_cmake``. They are not supported in Python packages using ``ament_python``. This restriction also applies to messages and services, so ensure your package is configured as a C++ package before proceeding with this tutorial.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Actions can only be created in a C++ package using ``ament_cmake``. They are not supported in Python packages using ``ament_python``. This restriction also applies to messages and services, so ensure your package is configured as a C++ package before proceeding with this tutorial.
Interfaces for Actions (along with Message and Service) can only be generated by a package which has ``ament_cmake`` as its ``build_type``.

Comment on lines 48 to 49
It is recommended to define action interfaces (as well as messages and services) in a separate package dedicated to interfaces, as shown in this tutorial. This improves modularity and makes it easier to reuse them across multiple projects.

Copy link
Member

@Yadunund Yadunund Mar 31, 2025

Choose a reason for hiding this comment

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

I suggest to move these two statements above the warning and outside the note block (ie, standalone statements) right after the Creating an interface package header.

Copy link
Author

Choose a reason for hiding this comment

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

okay

@kscottz
Copy link
Collaborator

kscottz commented Apr 1, 2025

Looks like @Yadunund and @fujitatomoya's suggestions are sufficient. I do want to point out that it helps us a lot if you can include a one or two sentence description of your pull request.

I just approved #5050 which should make #4985 a lot easier. Once you wrap up this PR you are more than welcome to give that one a try once you read my comment there.

@kscottz
Copy link
Collaborator

kscottz commented Apr 7, 2025

@vimal0athithan friendly ping. Can you accepted the suggestions and make the necessary changes please so we can get this out the door?

@vimal0athithan
Copy link
Author

@kscottz @Yadunund @fujitatomoya I have applied the suggested changes. Please review the updated version.

@vimal0athithan
Copy link
Author

@kscottz Thank you for the ping! I apologize for always contributing late always —I'm a university student, and my exams are currently ongoing, which has affected my availability. I’ve accepted the suggestions and made the necessary changes as requested. Please review the updates. Sorry again for the delay!

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.

I do not think suggested fixes are applied.

besides, github action test failing, can you check and test local environment with https://docs.ros.org/en/rolling/The-ROS2-Project/Contributing/Contributing-To-ROS-2-Documentation.html?

@@ -41,6 +41,14 @@ Tasks
1 Creating an interface package
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Actions can only be created in a C++ package using ``ament_cmake``. They are not supported
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think this is true. it does not have to be C++ package, rather it has to be CMake package.

Copy link
Collaborator

@kscottz kscottz left a comment

Choose a reason for hiding this comment

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

@vimal0athithan I sent you some suggestions to fix your linter problems. Our linter is extra picky unfortunately.

Please accept these changes, address Tomoya's comment and we can get you merged.

@vimal0athithan
Copy link
Author

@kscottz @Yadunund @fujitatomoya, I’ve pushed updates to fix the linting and test failures based on the check logs. Please review my new PR changes. Thank you!

@vimal0athithan
Copy link
Author

@kscottz @Yadunund @fujitatomoya, I’ve fixed the trailing whitespace and missing newline issues. Please review again. Thanks!

For more detailed information about ROS actions, please refer to the `design article <http://design.ros2.org/articles/actions.html>`__.
For more detailed information about ROS actions, please refer to the `design article <https://design.ros2.org/articles/actions.html>`__.

[blank line]
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this?

Suggested change
[blank line]
[blank line]

@kscottz
Copy link
Collaborator

kscottz commented Apr 24, 2025

@vimal0athithan can you hit the signoff / commit button on these?

kscottz and others added 4 commits April 28, 2025 15:23
Co-authored-by: Christophe Bedard <[email protected]>
Signed-off-by: Katherine Scott <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]>
Signed-off-by: Katherine Scott <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
Signed-off-by: Katherine Scott <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
Signed-off-by: Katherine Scott <[email protected]>
@kscottz
Copy link
Collaborator

kscottz commented May 1, 2025

@vimal0athithan second ping. This is so close.

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.

5 participants