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

Deprecate C Headers #720

Merged
merged 10 commits into from
Dec 18, 2024
Merged

Deprecate C Headers #720

merged 10 commits into from
Dec 18, 2024

Conversation

CursedRock17
Copy link
Contributor

This pull request is an attempt to help #259, specifically the first task

Rename all .h -> .hpp files. This will have to also have a deprecation warning, and should be done post-Foxy.

Since it's now post-Foxy I did something similar to this message_filters pull request, which was to simply deprecate a .h file with a warning, include the .hpp file, and just copy paste the logic from the .h file into the .hpp file. There were some slight formatting changes that had to be made to make cpplint and uncrustify happy. There is currently a copyrighting problem within the LinearMath files since they seem to be from the bullet library. I would like to know where we should move from this point when it comes to those files.

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

include/tf2/LinearMath/Scalar.h
include/tf2/LinearMath/Transform.h
include/tf2/LinearMath/Vector3.h
include/tf2/LinearMath/Matrix3x3.hpp
Copy link
Contributor

Choose a reason for hiding this comment

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

you should keep here both the h and the hpp

Copy link
Contributor

Choose a reason for hiding this comment

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

you should keep here both the h and the hpp

Agreed.

@clalancette
Copy link
Contributor

Besides some of @ahcorde 's inline comments, we will also need to have PRs to the rest of the ROS 2 core to use the .hpp files instead of the .h ones. Also, this is a large enough change that once we are ready to merge it, we should probably send a message to https://discourse.ros.org letting people know that we've done this.

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@CursedRock17
Copy link
Contributor Author

Yeah I figured we would have to extend PRs to various libraries, I have one linked from rviz, but I haven't found anymore, I'll have to do some digging around to find out if I've missed any more of the core ones. As far as the copyrighting goes, I believe those inline comments were the missing piece, I've submitted a fix.

@Timple
Copy link

Timple commented Oct 29, 2024

Would it make sense to provide .hpp files for all active distros? With as only content the include of the .h file.

This would mean all projects that do not have a branch per release can use this syntax already. Preventing ugly statements like this everywhere: https://github.com/autowarefoundation/autoware.universe/blob/9bd0f77c255edcff129ff08d190a5fdd8c45b6dc/localization/yabloc/yabloc_common/src/pub_sub.cpp#L18

For rolling we can keep the deprecation flag, not for older distros.

@CursedRock17
Copy link
Contributor Author

Yeah I don't see much of a compile downside, just a weirder file structure for older distributions. I assume we would just never phase out the .h files unlike the rolling branch in which we can remove them in L-Turtle. I would be able to add this "backport" later today if that's something we agree on.

Also there already seems to be some consensus in the other core repositories:

we should create backwards compatibilty headers that emit compile warnings so users have time to migrate to the new headers.

Can you add conditioning in the code so that from rolling on it uses the .hpp, and in the prior versions it uses the .h version?

This was referenced Oct 30, 2024
@CursedRock17
Copy link
Contributor Author

When I get back to my workstation later today, I can roll these improvements (whitespacing & visibility_control) onto the other branches I adjusted for the backport.

@clalancette
Copy link
Contributor

Pulls: #720, ros2/rviz#1289, ros-perception/laser_geometry#98
Gist: https://gist.githubusercontent.com/clalancette/b83127091186f2489c09c4c787585861/raw/3c16fd56b4d645d09b9cadf800da79cbfd1acbaf/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14792

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

@clalancette
Copy link
Contributor

So there are at least two problems here:

  1. We need a PR to interactive_markers to deal with the change.
  2. Windows doesn't seem to like the #warning construct, so we may have to do something special for Windows there. Maybe something like https://github.com/ros2/common_interfaces/blob/473511f5807610bb8ed7744dfd2986dba4601fa6/sensor_msgs/include/sensor_msgs/point_cloud_conversion.hpp#L41-L49

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Update Matrix3x3.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update MinMax.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update QuadWord.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update Quaternion.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update Scalar.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update Transform.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update Vector3.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update buffer_core.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update buffer_core_interface.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update convert.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update exceptions.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update convert.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update utils.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update time.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update time_cache.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update transform_datatypes.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update transform_storage.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update utils.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update MinMax.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update QuadWord.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update Quaternion.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update Scalar.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update Transform.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update Vector3.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update buffer_core.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update buffer_core_interface.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update convert.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update exceptions.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update convert.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>

Update utils.h

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@CursedRock17
Copy link
Contributor Author

Hopefully windows likes the current implemention. Also backport PRs are ready to go as well.

@ahcorde
Copy link
Contributor

ahcorde commented Nov 8, 2024

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

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

There are still some warnings

@CursedRock17
Copy link
Contributor Author

I think it's just the repos being built, I changed up interactive_markers which seems to be the repository throwing out some warnings as it's still on the rolling branch. My updated branch stops using the .h files that are the only ones triggering the warnings, which should be ok.

@MichaelOrlov
Copy link

@ahcorde Could you please try to rerun CI?

@ahcorde
Copy link
Contributor

ahcorde commented Dec 17, 2024

included PRs: geometry2, laser_geometry, RViz2 and interactive_markers

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

@ahcorde
Copy link
Contributor

ahcorde commented Dec 17, 2024

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

@ahcorde ahcorde merged commit 7c71648 into ros2:rolling Dec 18, 2024
2 checks passed
ahcorde pushed a commit that referenced this pull request Dec 18, 2024
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>
ahcorde pushed a commit that referenced this pull request Dec 19, 2024
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>
ahcorde added a commit to ros-perception/image_pipeline that referenced this pull request Dec 26, 2024
Related to this [pull
request](ros2/geometry2#720) in `geometry2` in
which we deprecated the `.h` style headers in favor of `.hpp`.

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
ahcorde pushed a commit to ros-perception/imu_pipeline that referenced this pull request Dec 27, 2024
Related to this [pull
request](ros2/geometry2#720) in `geometry2` in
which we deprecated the `.h` style headers in favor of `.hpp`.

---------

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@CursedRock17 CursedRock17 deleted the deprecate_c_headers branch January 17, 2025 03:48
@Ryanf55
Copy link
Contributor

Ryanf55 commented Jan 21, 2025

Iron I guess?

Can you do humble if it's not too much work?

What's the story on humble? Our work environment is now broken from the docker images supplied from ros:humble-ros-core.

Currently, in this broken state, we have the following installed.

  • ros-humble-tf2 version 0.25.9-1jammy.20241128.023902
  • ros-humble-tf2-eigen version 0.25.11-1jammy.20250114.170617

In tf2_eigen.hpp, it includes "tf2/convert.hpp".

Then, the files that exist:

$ ls /opt/ros/humble/include/tf2/tf2/ -1
LinearMath
buffer_core.h
buffer_core_interface.h
convert.h
exceptions.h
impl
time.h
time_cache.h
transform_datatypes.h
transform_storage.h
utils.h
visibility_control.h

Where's the deprecation period and how did this get all the way through the build farm into a docker image with debians that don't work together. Why isn't tf2-eigen expressing a minimum version on tf2 so it fails at configure time?

Sorry to be blunt, but this is just frustrating.

@clalancette
Copy link
Contributor

  • ros-humble-tf2 version 0.25.9-1jammy.20241128.023902

  • ros-humble-tf2-eigen version 0.25.11-1jammy.20250114.170617

These aren't the same versions, which is why you are having trouble here. The debians that are released should all match as 0.25.11; the fact that they aren't probably means your local scripts are missing an apt-get update somewhere (i.e. it is using a cached layer with tf2, and a new layer with tf2_eigen).

@Ryanf55
Copy link
Contributor

Ryanf55 commented Jan 21, 2025

Thanks. If there's any changes we could do to ensure that we fail at configure time rather than build time, I'd be happy to contribute those.

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.

6 participants