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

Hotfix for rosidl cpp visibility header addition #371

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

calderpg-tri
Copy link
Collaborator

@calderpg-tri calderpg-tri commented Dec 6, 2024

This change is Reviewable

Copy link
Member

@IanTheEngineer IanTheEngineer left a comment

Choose a reason for hiding this comment

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

+@IanTheEngineer for review.

Reviewable status: 0 of 1 files reviewed, all discussions resolved

Copy link
Member

@IanTheEngineer IanTheEngineer left a comment

Choose a reason for hiding this comment

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

I am in agreement that finding and using the actual template file on disk is preferable to having it here in string form. For future reference, the file is now located here:
/opt/ros/humble/share/rosidl_generator_cpp/resource/rosidl_generator_cpp__visibility_control.hpp.in

With that said, the required Bazel-structure to access this file is not immediately clear, so I am okay leaving this as a TODO for now.

:LGTM: Thank you for the quick patch, Calder.

Reviewed 1 of 1 files at r2.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @calderpg-tri)

@jwnimmer-tri
Copy link
Contributor

jwnimmer-tri commented Dec 6, 2024

Let me know if you want me to force this into main (and if the PR title is what you want for the squashed commit subject).

@calderpg-tri calderpg-tri changed the title Hotfix for cpp visibility header addition Hotfix for rosidl cpp visibility header addition Dec 6, 2024
Copy link
Collaborator Author

@calderpg-tri calderpg-tri left a comment

Choose a reason for hiding this comment

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

I've changed the title slightly. I believe @IanTheEngineer is going to try to fix the CI configuration here, but it's probably fine to force it into main for now.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @IanTheEngineer)

@jwnimmer-tri
Copy link
Contributor

Okay, I've confirmed that the CI failure is not reproducible locally. I'll force this in now.

@jwnimmer-tri jwnimmer-tri merged commit 7f47134 into RobotLocomotion:main Dec 6, 2024
4 of 6 checks passed
@IanTheEngineer
Copy link
Member

I was about to comment the same thing! I cannot reproduce the CI failure either. Thanks for merging, Jeremy.

@jwnimmer-tri
Copy link
Contributor

I think the failure happens when the older ROS packages are installed but the newer code is being run -- and because the GHA CI doesn't run apt upgrade apparently the image: ros:humble-ros-base-jammy image doesn't have the latest packages yet.

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.

3 participants