Skip to content

Add new interfaces to enable intropsection for action #1207

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 4, 2025
Merged

Add new interfaces to enable intropsection for action #1207

merged 2 commits into from
Mar 4, 2025

Conversation

@Barry-Xu-2018
Copy link
Contributor

@Barry-Xu-2018 Barry-Xu-2018 commented Feb 13, 2025

This was referenced Feb 13, 2025
Open
Merged
Merged
@Barry-Xu-2018 Barry-Xu-2018 marked this pull request as ready for review February 25, 2025 09:15
fujitatomoya
fujitatomoya reviewed Feb 27, 2025
View reviewed changes
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 had a few comments, can you check them?

@Barry-Xu-2018
Copy link
Contributor Author

Barry-Xu-2018 commented Feb 28, 2025

I had a few comments, can you check them?

Thank you, I have addressed your review comments.

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 28, 2025 •
edited
Loading

Pulls: #1207, ros2/rclcpp#2750
Gist: https://gist.githubusercontent.com/fujitatomoya/91d94028e31077ee4d79acaba2e26143/raw/613e62322a68fdca5345cf727161389cc6bbb260/ros2.repos
BUILD args: --packages-above-and-dependencies rcl rcl_action rclcpp
TEST args: --packages-above rcl rcl_action rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15256

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

@fujitatomoya fujitatomoya mentioned this pull request Feb 28, 2025
Merged
@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 28, 2025

@Barry-Xu-2018 DCO is missing, can you check that? CI is green, so once DCO error is fixed, i will merge this.

@Barry-Xu-2018
Address review comments
4356183 
Signed-off-by: Barry Xu <[email protected]>
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the review/topic-action-configure-service-introspection branch from 7196992 to 4356183 Compare March 3, 2025 02:16
@Barry-Xu-2018
Copy link
Contributor Author

Barry-Xu-2018 commented Mar 3, 2025

@fujitatomoya My mistake, I have resubmitted it.

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Mar 3, 2025

Pulls: #1207
Gist: https://gist.githubusercontent.com/fujitatomoya/1803d79b6090aa14db93253b2643fed1/raw/5c414a4d416c14e1aaf54080b6631fdc3a8057d8/ros2.repos
BUILD args: --packages-above-and-dependencies rcl rcl_action
TEST args: --packages-above rcl rcl_action
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15271

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

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Mar 3, 2025

@Barry-Xu-2018 can you merge this when CI is green.

@Barry-Xu-2018
Copy link
Contributor Author

Barry-Xu-2018 commented Mar 4, 2025 •
edited
Loading

The build for Windows is unstable.
After checking log, the failure case is test_launch_ros.pytest.missing_result. It is unrelated to this PR.
So this PR can be merged.

@Barry-Xu-2018
Copy link
Contributor Author

Barry-Xu-2018 commented Mar 4, 2025

@fujitatomoya I cannot do merge for this PR.

@fujitatomoya fujitatomoya merged commit f9ad1c6 into ros2:rolling Mar 4, 2025
3 checks passed
Crola1702
Crola1702 reviewed Mar 4, 2025
View reviewed changes
rcl_action/src/rcl_action/action_server.c
const rosidl_action_type_support_t * type_support,
const rcl_publisher_options_t publisher_options,
rcl_service_introspection_state_t introspection_state)
{
Copy link

@Crola1702 Crola1702 Mar 4, 2025

Choose a reason for hiding this comment

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

There is a warning in Windows CI:

'rcl_action_server_configure_action_introspection': inconsistent dll linkage

See reference build:

FYI: @Barry-Xu-2018 @fujitatomoya

Copy link
Collaborator

@fujitatomoya fujitatomoya Mar 4, 2025

Choose a reason for hiding this comment

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

@Barry-Xu-2018 actually this was detected by https://ci.ros2.org/job/ci_windows/23428/msbuild/, but we overlooked it.

Copy link
Collaborator

@fujitatomoya fujitatomoya Mar 4, 2025

Choose a reason for hiding this comment

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

@Crola1702 @Barry-Xu-2018 #1212 should fix this inconsistent linkage warning.

@Crola1702 Crola1702 mentioned this pull request Mar 4, 2025
Closed
@fujitatomoya fujitatomoya mentioned this pull request Mar 4, 2025
Merged
@Barry-Xu-2018
Copy link
Contributor Author

Barry-Xu-2018 commented Mar 5, 2025 •
edited
Loading

@Crola1702 @fujitatomoya

Sorry. I overlooked the build issue.
Thank fujitatomoya for helping to fix this issue.

@christophebedard christophebedard mentioned this pull request Apr 5, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@Crola1702 Crola1702 Crola1702 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Barry-Xu-2018 @fujitatomoya @Crola1702