Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Prototype gz header migration #51

Closed
wants to merge 3 commits into from
Closed

Conversation

scpeters
Copy link
Member

🎉 New feature

Prototype part of gazebo-tooling/release-tools#698

Summary

The big diff is not so easy to read, so I recommend going commit-by-commit:

  • b2e1098: move the current ignition header folder to gz
  • 7c6e7e8: make several changes needed to support the gz headers
  • 6fb3171: add dummy ignition/* headers that simply redirect to their gz/ counterpart

This takes a very minimal approach, just to change the include path without changing any namespaces or even the IGNITION_* variables in config.hh. The only macros changed are for header guards of the new gz/* headers.

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
* REPLACE_IGNITION_INCLUDE_PATH gz/utils
* Include from gz/* in header files
* Update header guards to use GZ_*
* Add gz subdirectory in CMakeLists.txt

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters requested a review from chapulina April 18, 2022 20:08
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Apr 18, 2022
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label Apr 18, 2022
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks, @scpeters , this approach looks good to me. Some questions / comments:

  • How do you think we should do the deprecation warnings? Using a #pragma message() in every ignition header?
  • Did you use a script or just prototyped it manually?
  • It would be good to move the files from ignition to gz so we keep their commit history.
  • When this PR is ready for merging, we should update examples and tests

@scpeters
Copy link
Member Author

  • How do you think we should do the deprecation warnings? Using a #pragma message() in every ignition header?

I'm not sure if #pragma message() works with gcc. I think we could define a macro that uses different directives depending on the compiler and include that in each ignition/* header just before the redirect include of the corresponding gz/ header.

  • Did you use a script or just prototyped it manually?

I did this manually, but it shouldn't be too hard to script

  • It would be good to move the files from ignition to gz so we keep their commit history.

I did a git mv command in b2e1098 and you can confirm with a git blame that the history is still there:

It doesn't show up because github is a little confused, but I think our history is preserved with this approach.

  • When this PR is ready for merging, we should update examples and tests

we can decide how to split the work up into phases, but I believe this approach can be merged independently without breaking anything. We could optionally update the #include <ignition/*> calls in src, test, and examples as you say, but I was trying to see what's the minimal amount that needs to be changed for this first PR.

Some possible steps:

  • Move header files to gz/* and add redirect headers to ignition/* (included in this PR)
  • Replace all #include <ignition/*> statements in header files with #include <gz/*> (included in this PR)
  • Replace all #include <ignition/*> statements in src, test, and examples source code with #include <gz/*> (not included in this PR)
  • Replace all #include <ignition/*> statements in downstream code
  • Change ign-cmake3's default value of PROJECT_INCLUDE_DIR from ignition/${IGN_DESIGNATION} to gz/${IGN_DESIGNATION} (or use ${GZ_DESIGNATION} if that has been defined) in IgnConfigureProject.cmake
  • Remove the REPLACE_IGNITION_INCLUDE_PATH parameter override once its no longer needed
  • Add using namespace ignition to the ignition/ headers
  • Add portable #warning equivalent to ignition/ headers

Because GitHub gets confused by the diff in this pull request, I like making a narrow PR like this to add the gz/ headers in a backwards-compatible way. This allows flexibility in deciding when to change the c++ namespaces and add deprecation warnings

@scpeters
Copy link
Member Author

we could arguably backport this to fortress since I think it's backwards compatible, but I'm not sure if it's worth it

@scpeters scpeters requested a review from methylDragon April 21, 2022 18:43
@scpeters scpeters marked this pull request as ready for review April 21, 2022 19:23
@scpeters scpeters requested a review from azeey as a code owner April 21, 2022 19:23
@chapulina
Copy link
Contributor

we could arguably backport this to fortress

Something to keep in mind for later 👍🏽

*
*/

#include <gz/utils.hh>
Copy link
Contributor

@methylDragon methylDragon Apr 25, 2022

Choose a reason for hiding this comment

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

Is this meant to be gz/utils/utils.hh?
(I'll write the script assuming that it's utils/utils.hh, but it'll be a trivial change to change it back.)

Copy link
Member Author

Choose a reason for hiding this comment

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

in general, it should only have one utils, so gz/utils.hh is correct (look at the installed paths of any ign-* package to confirm)

the one exception is libsdformat, which has sdf/sdf.hh but that one is a bit different anyway

@chapulina
Copy link
Contributor

I'm not sure if #pragma message() works with gcc.

It prints a message for me, not sure what's missing? I think the only downside to it is that it's not an actual warning.

I did a git mv command in b2e1098 and you can confirm with a git blame that the history is still there:

Ah perfect, indeed GitHub gets confused in the PR diff

we can decide how to split the work up into phases

I'm fine with that 👍🏽 We just have to make sure we don't introduce warnings in the intermediate steps.

@scpeters scpeters closed this Apr 28, 2022
@scpeters scpeters deleted the scpeters/gz_prototype branch April 28, 2022 21:29
@scpeters
Copy link
Member Author

#53 was merged instead

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🌱 garden Ignition Garden ign to gz Renaming Ignition to Gazebo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants