ign -> gz Header Migration : ign-common #348

methylDragon · 2022-04-28T07:30:31Z

See gazebo-tooling/release-tools#711

methylDragon · 2022-04-29T22:53:00Z

This is pending the merge and nightly of ign-math

chapulina

LGTM with green CI. My comments below can be addressed in follow-up PRs if that's easier.

av/include/ignition/common/HWEncoder.hh

av/include/ignition/common/HWVideo.hh

chapulina · 2022-05-03T03:14:28Z

geospatial/include/CMakeLists.txt

@@ -0,0 +1,2 @@
+add_subdirectory(gz)
+install(DIRECTORY ignition DESTINATION ${IGN_INCLUDE_INSTALL_DIR_FULL})


geospatial is a new component from Garden, so we don't need to add ignition headers and can go straight to gz, because no downstream users should be using it yet anyway.

One caveat is that our libraries are already using it, so one strategy we could take would be to keep ignition/geospatial while we're updating all libraries, and then remove it afterwards.

Might be neater to remove it during the namespace (or maybe the source) migration? I'm not sure :o

I think any time after the header migration for all libraries should work, because none of the libraries should be using them anymore. So I'm fine either way. It's probably good to do it as a standalone PR for visibility. We just shouldn't forget to do it.

chapulina · 2022-05-03T03:16:24Z

include/gz/common/Plugin.hh

@@ -0,0 +1,196 @@
+/*


We're deprecating all the *Plugin* files from ign-common on Garden, in favor of using the ign-plugin library. So I think we don't need to add gz counterparts to it.

I'd be ok doing this in a separate PR, we don't even have IGN_DEPRECATED notices on them anyway.

Let's do this in a separate PR, so it can be tracked independently.

I'll track this in the issue here: #349
But I suspect I'll be focused on the other libraries.

chapulina · 2022-05-03T03:19:26Z

include/ignition/common/detail/Plugin.hh

@@ -15,98 +15,4 @@
 *
 */



Since the detail directories aren't part of the public API, I think we can just remove them.

If include/ignition/common/Plugin.hh isn't including include/ignition/common/detail/Plugin.hh, no one else is.

This is valid for all libraries.

Can I double check if this means that I delete all detail redirection headers across all libraries?

Since the detail directories aren't part of the public API, I think we can just remove them.

If include/ignition/common/Plugin.hh isn't including include/ignition/common/detail/Plugin.hh, no one else is.

This is valid for all libraries.

I disagree. These are installed header files, so they are part of the public API, and they are included from the header files not in the detail folder, though often from the bottom of the file so it is easy to miss:

https://github.com/ignitionrobotics/ign-math/blob/ignition-math6_6.10.0/include/ignition/math/Box.hh#L224

https://github.com/ignitionrobotics/ign-common/blob/ignition-common4_4.5.0/include/ignition/common/Plugin.hh#L194

I think it would be best to keep the redirection headers for all the contents of the detail subfolders

These are installed header files, so they are part of the public API,

I understand that we've never explicitly declared our public API, and we should. But the reason that the detail directories exist is that they're not meant to be directly used by downstream. They're installed because of technicalities of the language which require those templates to be installed, but that's an implementation detail, not our API. And as such, we don't need to tick-tock them.

ROS QL1 packages explicitly call this out. I ticketed this issue so we document this properly for Gazebo:

Declare our public API docs#248

I think it would be best to keep the redirection headers for all the contents of the detail subfolders

Having them there wouldn't break anything, but those should be dead files that no one is using. I'd say that keeping them there sends the wrong message that those are part of our public API and subject to tick-tock.

I'm getting a very different impression from your first and most recent comments. I think your points are reasonable, but the way they were initially phrased was a bit alarming to me.

Since the detail directories aren't part of the public API, I think we can just remove them.

I've gathered from your subsequent comment that the context is specifically about redirection headers in a detail subfolder, but on first glance it sounded to me like you were saying we can delete detail headers at any time from anywhere with no impact on API, which is absolutely not true. I now think you meant we don't need to tick-tock them, which is reasonable.

If include/ignition/common/Plugin.hh isn't including include/ignition/common/detail/Plugin.hh, no one else is.

Again, I now gather that this context is about redirection headers, not public headers in general, since gz/common/Plugin.hh does include gz/common/detail/Plugin.hh

This is valid for all libraries.

Nothing specific here but it added to my alarm! 😅

Ah sorry I should have been clearer. Yes, what I meant is that we can remove the ignition/*/detail/*.hh headers that just include gz/*/detail/*.hh ✅

I think downstream users are less likely to need the ignition/*/detail/* redirection headers. I would lean towards redirecting all headers for now until we've migrated headers for all the ignition libraries and then consider removing the detail redirects when we add the deprecation warnings to the primary redirection headers.

I think downstream users are less likely to need the ignition//detail/ redirection headers

If they're including them directly, that's a mistake on their part.

I would lean towards redirecting all headers for now until we've migrated headers

I'm ok with that if it makes things easier. We already have a long list of things to go back to, and long PRs to review. If the script could be updated to skip the detail folders automatically I think that would make our lives easier...

Done! gazebo-tooling/release-tools@7b17e47

profiler/CMakeLists.txt

chapulina · 2022-05-03T03:24:17Z

testing/include/ignition/common/testing/AutoLogFixture.hh

+ *
+ */
+
+#include <gz/common/testing/AutoLogFixture.hh>


The testing component is also new on Garden, just like geospatial. So we don't need to provide ignition/testing headers.

codecov · 2022-05-03T03:39:54Z

Codecov Report

Merging #348 (4d4366b) into main (fa6cd64) will not change coverage.
The diff coverage is n/a.

❗ Current head 4d4366b differs from pull request most recent head fcf8f68. Consider uploading reports for the commit fcf8f68 to get more accurate results

@@           Coverage Diff           @@
##             main     #348   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files           1        1           
  Lines          48       48           
=======================================
  Hits           44       44           
  Misses          4        4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa6cd64...fcf8f68. Read the comment docs.

Signed-off-by: methylDragon <[email protected]>

methylDragon requested a review from mjcarroll as a code owner April 28, 2022 07:30

github-actions bot added the 🌱 garden Ignition Garden label Apr 28, 2022

methylDragon mentioned this pull request Apr 28, 2022

C++ namespaces, directories and header guards from ignition to gz gazebo-tooling/release-tools#711

Closed

methylDragon force-pushed the header_migration branch from 9f27672 to 9bcab9a Compare April 28, 2022 07:37

chapulina added the ign to gz Renaming Ignition to Gazebo. label Apr 28, 2022

methylDragon marked this pull request as draft April 28, 2022 23:55

methylDragon marked this pull request as ready for review April 29, 2022 22:49

methylDragon force-pushed the header_migration branch 2 times, most recently from 9254bfa to 724125f Compare April 30, 2022 01:04

chapulina approved these changes May 3, 2022

View reviewed changes

methylDragon mentioned this pull request May 3, 2022

TODO: Deprecate/Remove Plugin from Garden #349

Closed

methylDragon added 4 commits May 2, 2022 20:57

Move header files with git mv

5f0e5c2

Signed-off-by: methylDragon <[email protected]>

Create redirection aliases

df2ddf0

Signed-off-by: methylDragon <[email protected]>

Migrate sources in src, test, examples, and include

0c593d4

Signed-off-by: methylDragon <[email protected]>

Migrate build files

fcf8f68

Signed-off-by: methylDragon <[email protected]>

methylDragon force-pushed the header_migration branch from 724125f to fcf8f68 Compare May 3, 2022 03:59

methylDragon merged commit 3aadb66 into main May 3, 2022

methylDragon deleted the header_migration branch May 3, 2022 05:05

scpeters mentioned this pull request May 3, 2022

Deprecate plugin in favor of gz-plugin #350

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ign -> gz Header Migration : ign-common #348

ign -> gz Header Migration : ign-common #348

methylDragon commented Apr 28, 2022

methylDragon commented Apr 29, 2022

chapulina left a comment

chapulina May 3, 2022

methylDragon May 3, 2022

chapulina May 3, 2022

chapulina May 3, 2022

methylDragon May 3, 2022

chapulina May 3, 2022

methylDragon May 3, 2022

scpeters May 3, 2022

chapulina May 3, 2022

scpeters May 3, 2022

chapulina May 3, 2022

scpeters May 3, 2022

chapulina May 3, 2022

methylDragon May 4, 2022 •

edited

Loading

chapulina May 3, 2022

codecov bot commented May 3, 2022 •

edited

Loading

		@@ -0,0 +1,2 @@
		add_subdirectory(gz)
		install(DIRECTORY ignition DESTINATION ${IGN_INCLUDE_INSTALL_DIR_FULL})

ign -> gz Header Migration : ign-common #348

ign -> gz Header Migration : ign-common #348

Conversation

methylDragon commented Apr 28, 2022

methylDragon commented Apr 29, 2022

chapulina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methylDragon May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 3, 2022 • edited Loading

Codecov Report

methylDragon May 4, 2022 •

edited

Loading

codecov bot commented May 3, 2022 •

edited

Loading