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

Tick-tock Export Headers #249

Merged
merged 5 commits into from
May 16, 2022
Merged

Tick-tock Export Headers #249

merged 5 commits into from
May 16, 2022

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented May 4, 2022

This is related to: gazebo-tooling/release-tools#711

Description

ign-cmake dynamically configures an Export.hh file in the public headers of a package. This Export.hh file actually wraps and comments on a lower level dynamically generated detail/Export.hh file that is generated using GenerateExportHeader.

The fact we're using an external CMake library to generate the detail/Export.hh makes it hard to ticktock the lower-level CMake macros.

So instead, this PR defines IGNITION_ prefixed export headers in the configured Export.hh if a target is prefixed with gz that allow downstream users to use the IGNITION_ prefixed versions of the export macros, while leaving those in detail/Export.hh alone (except for IGN_DEPRECATED_ALL_VERSIONS, which doesn't seem to be used anywhere (at least in the core libraries.)

If the library target is ignition prefixed, then no extra macros generate.

This PR replaces all IGNITION_/IGN_ prefixed macros with GZ_ prefixed versions in Export.hh and detail/Export.hh. And provisions IGNITION_ prefixed and the IGN_DEPRECATED macros in the public Export.hh alongside the GZ_ ones, so you can use both!

This means that detail/Export.hh defined macros aren't tick-tocked though! But since they're ostensibly private, it shouldn't be an issue.

Notes

This does nothing new at the moment, but once we start using GZ prefixed macro names, everything will be 🌞 🌈 because of this.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label May 4, 2022
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label May 4, 2022
@methylDragon methylDragon force-pushed the ch3/export-tick-tock branch 2 times, most recently from 62e26f8 to 5f72b77 Compare May 4, 2022 21:06
@methylDragon
Copy link
Contributor Author

Should I just make every EXPORT_BASE point to a GZ_ prefix, and remove the logic?

@methylDragon methylDragon force-pushed the ch3/export-tick-tock branch 3 times, most recently from 815e592 to d0c212a Compare May 4, 2022 21:29
@methylDragon
Copy link
Contributor Author

methylDragon commented May 4, 2022

Should I just make every EXPORT_BASE point to a GZ_ prefix, and remove the logic?

I decided to do that. 20748d4

Note that this means every detail/Export.hh will be hard migrated to GZ_ prefixes, with no ticktocking!
I don't see any uses of the macros defined there though, so we should be fine 🤞

@methylDragon methylDragon force-pushed the ch3/export-tick-tock branch 2 times, most recently from f0a0d22 to 20748d4 Compare May 4, 2022 21:37
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
#ifndef IGN_DEPRECATED
// TODO(CH3): Remove on ticktock
// NOTE(CH3): This has no docstring since we'll make GZ_DEPRECATED the default
#define IGN_DEPRECATED(version) GZ_DEPRECATED_ALL_VERSIONS
Copy link
Member

Choose a reason for hiding this comment

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

do we want to define IGNITION_ versions of _VISIBLE and _HIDDEN as well?

if so, we should create an ign_export_base variable to use when configuring this file

Copy link
Contributor Author

@methylDragon methylDragon May 5, 2022

Choose a reason for hiding this comment

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

They're defined in here already, using the _ign_export_base variable! 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

thanks; I could have done a better job reading the pull request changes before making comments like that. I was a little confused on first glance to see all the Doxygen for the redirection macros; I think we could remove them. See my suggested changes in ee17946

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes incorporated!

methylDragon and others added 4 commits May 13, 2022 18:56
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@methylDragon methylDragon force-pushed the ch3/export-tick-tock branch from 64bad56 to fb53df0 Compare May 14, 2022 01:59
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon merged commit 98e431a into main May 16, 2022
@methylDragon methylDragon deleted the ch3/export-tick-tock branch May 16, 2022 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden ign to gz Renaming Ignition to Gazebo.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants