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

Change tf2_ros C to C++ headers #746

Open
sea-bass opened this issue Jan 4, 2025 · 6 comments
Open

Change tf2_ros C to C++ headers #746

sea-bass opened this issue Jan 4, 2025 · 6 comments
Labels
help wanted Extra attention is needed

Comments

@sea-bass
Copy link

sea-bass commented Jan 4, 2025

Bug report

Required Info:

  • Operating System:
    • Ubuntu 24.04
  • Installation type:
    • Binaries
  • Version or commit hash:
    • rolling
  • DDS implementation:
    • N/A
  • Client library (if applicable):
    • N/A

Steps to reproduce issue

In #720, all .h files in the tf2 package were converted to .hpp with deprecation warnings -- which is great!

However, this was not done consistently throughout the repo. For example, the tf2_ros package still is missing several .hpp files. This makes it pretty inconvenient to do blanket search/replaces in downstream repos that use this.

Expected behavior

Every header in the repo should ideally consistently have a .hpp counterpart now -- not just tf2.

Actual behavior

Not the case currently.

Additional information

N/A

@ahcorde
Copy link
Contributor

ahcorde commented Jan 6, 2025

Related PR #744

@clalancette
Copy link
Contributor

However, this was not done consistently throughout the repo. For example, the tf2_ros package still is missing several .hpp files. This makes it pretty inconvenient to do blanket search/replaces in downstream repos that use this.

For what it is worth, we usually do it on a package-by-package basis, not for the whole repository. It is just too disruptive to do everything at once. With #744 now in, I think the big remaining package to do here is tf2_ros, which will require another large round of downstream PRs.

@CursedRock17
Copy link
Contributor

Hey sorry to be late to the conversation, I've been pretty stacked up with some stuff. Like @clalancette said, I assumed we were going to tackle this at the package level. Especially since #259 (the original target) only mentioned tf2. I wouldn't mind doing tf2_ros as well, but seeing the bit of chaos #720 has created, I would recommend waiting for this chunk to settle in. I can create an issue and have it assigned to me if interested.

@sea-bass
Copy link
Author

sea-bass commented Jan 9, 2025

This makes sense!

For what it's worth, this "chaos" is a good thing. It means lots of CI pipelines that use this package are treating warnings as errors and reacting quickly to the changes.

I'm happy to close / rename this issue in favor of tracking tickets to consistently port all the other individual packages in this repo.

Though I will say, we did this for moveit2 recently ad well, with the conscious effort of ensuring all the changes to the binaries happened in the same sync.

@christophebedard
Copy link
Member

I'm happy to close / rename this issue in favor of tracking tickets to consistently port all the other individual packages in this repo.

That would indeed be good!

@CursedRock17
Copy link
Contributor

I would be in agreement to just renaming this issue since there's already discussion about the topic here. The only package we need change header files for, in this repo, is tf2_ros. Everything else would just be adjustments for that package.

all the changes to the binaries happened in the same sync.

Seems to be the next biggest problem in from #720's release.

@clalancette clalancette changed the title C++ headers were not deprecated consistently Change tf2_ros C to C++ headers Jan 30, 2025
@clalancette clalancette added the help wanted Extra attention is needed label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants