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.

Sign up for GitHub

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

Jump to bottom

ign -> gz Header Migration : ign-cmake #243

Closed
wants to merge 4 commits into from
Closed

ign -> gz Header Migration : ign-cmake #243

wants to merge 4 commits into from

Conversation

methylDragon
Copy link
Contributor

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Apr 27, 2022
@methylDragon methylDragon mentioned this pull request Apr 27, 2022
Closed
@methylDragon methylDragon force-pushed the header_migration branch 4 times, most recently from 5e6ab09 to eb3b748 Compare April 27, 2022 23:39
@methylDragon
Copy link
Contributor Author

methylDragon commented Apr 28, 2022 •
edited
Loading

Note

This PR will require a lot of manual patching for REPLACE_IGNITION_INCLUDE_PATH for the examples because the examples have a bunch of subdirectories/subprojects.

I won't script that because there's a lot of variance between how the examples are done for the different packages.

@scpeters
Copy link
Member

Note

This PR will require a lot of manual patching for REPLACE_IGNITION_INCLUDE_PATH for the examples because the examples have a bunch of subdirectories/subprojects.

I won't script that because there's a lot of variance between how the examples are done for the different packages.

I think the examples in this repository can wait if they take extra work. They can switch when we update the default include path in the ign-cmake macros themselves

@methylDragon methylDragon force-pushed the header_migration branch 10 times, most recently from 8f2b06e to 4999e15 Compare April 28, 2022 07:11
@methylDragon methylDragon marked this pull request as draft April 28, 2022 23:55
@methylDragon methylDragon marked this pull request as ready for review April 29, 2022 00:32
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label May 2, 2022
chapulina
chapulina suggested changes May 3, 2022
View reviewed changes
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.

Looking at this PR now, I think gz-cmake is a special case where we don't need to migrate any includes to gz because they're all deprecated (#171). So I think we can close this PR and call the gz-cmake header migration done.

include/gz/utilities/ExtraTestMacros.hh

#include <gz/utilities/detail/ExtraTestMacros.hh>

#pragma message("ign-cmake (utilities) ExtraTestMacros is deprecated, use ign-utils")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is deprecated anyway, how about we don't add gz aliases for them? That is, users of ignition/utilities shouldn't migrate to gz/utilities, but to gz/utils, which is in another library.

@methylDragon
Copy link
Contributor Author

Looking at this PR now, I think gz-cmake is a special case where we don't need to migrate any includes to gz because they're all deprecated (#171). So I think we can close this PR and call the gz-cmake header migration done.

🤔 We might want to consider migrating the examples when we're ready then, but I'll close this for now.

@scpeters
Copy link
Member

scpeters commented May 4, 2022

Looking at this PR now, I think gz-cmake is a special case where we don't need to migrate any includes to gz because they're all deprecated (#171). So I think we can close this PR and call the gz-cmake header migration done.

🤔 We might want to consider migrating the examples when we're ready then, but I'll close this for now.

we can migrate the #include <ignition/* statements in the examples when we change the default PROJECT_INCLUDE_PATH path to gz/

@chapulina chapulina deleted the header_migration branch May 10, 2022 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina requested changes

@mxgrey mxgrey Awaiting requested review from mxgrey

@j-rivero j-rivero Awaiting requested review from j-rivero j-rivero is a code owner

Assignees
No one assigned
Labels
🌱 garden Ignition Garden ign to gz Renaming Ignition to Gazebo.
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@methylDragon @scpeters @chapulina