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

ign -> gz Header Migration : ign-msgs #249

Merged
merged 4 commits into from
May 10, 2022
Merged

ign -> gz Header Migration : ign-msgs #249

merged 4 commits into from
May 10, 2022

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Apr 28, 2022

See gazebo-tooling/release-tools#711

Notes:

  • proto message definitions for gz and ignition will be installed alongside each other, but ONLY THE gz proto messages will result in code generation for sources and headers.
  • The entirety of the ignition headers are instantiated as redirection headers pointing to the gz version
  • The gz headers use the ignition namespace, pending namespace migration

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Apr 28, 2022
@methylDragon methylDragon marked this pull request as draft April 28, 2022 23:55
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label May 2, 2022
@methylDragon methylDragon marked this pull request as ready for review May 3, 2022 00:45
@methylDragon methylDragon force-pushed the header_migration branch 3 times, most recently from e64a884 to 1ca5aa6 Compare May 3, 2022 02:40
@methylDragon methylDragon marked this pull request as draft May 3, 2022 03:05
@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #249 (48a8297) into main (e61edda) will decrease coverage by 1.82%.
The diff coverage is n/a.

❗ Current head 48a8297 differs from pull request most recent head d512175. Consider uploading reports for the commit d512175 to get more accurate results

@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
- Coverage   15.90%   14.08%   -1.83%     
==========================================
  Files         382      764     +382     
  Lines       71409   142630   +71221     
==========================================
+ Hits        11361    20088    +8727     
- Misses      60048   122542   +62494     
Impacted Files Coverage Δ
include/gz/msgs/boxgeom.pb.h 5.71% <0.00%> (ø)
include/gz/msgs/world_modify.pb.cc 14.17% <0.00%> (ø)
include/gz/msgs/laserscan.pb.cc 9.11% <0.00%> (ø)
include/gz/msgs/sonar.pb.h 0.00% <0.00%> (ø)
include/gz/msgs/tactile.pb.cc 14.17% <0.00%> (ø)
include/gz/msgs/imu_sensor.pb.cc 12.96% <0.00%> (ø)
include/gz/msgs/subscribe.pb.cc 13.86% <0.00%> (ø)
include/gz/msgs/uint64.pb.cc 19.23% <0.00%> (ø)
include/gz/msgs/cmd_vel2d.pb.cc 17.94% <0.00%> (ø)
include/gz/msgs/world_stats.pb.h 1.94% <0.00%> (ø)
... and 372 more

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 e61edda...d512175. Read the comment docs.

@methylDragon methylDragon marked this pull request as ready for review May 3, 2022 04:00
@methylDragon
Copy link
Contributor Author

All green!

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.

It looks like the import calls inside downstream messages are broken with this PR. I tried to compile gz-sim's main branch on top of this PR and it fails with:

/home/chapulina/dev_focal/ws_garden/build/ignition-gazebo7/src/msgs/peer_control.pb.cc:23:8: error: _PROTOBUF_INTERNAL_EXPORT_protobuf_ignit
ion_2fmsgs_2fheader_2eproto_ does not name a type; did you mean _PROTOBUF_INTERNAL_EXPORT_protobuf_gz_2fmsgs_2fheader_2eproto_?             
   23 | extern PROTOBUF_INTERNAL_EXPORT_protobuf_ignition_2fmsgs_2fheader_2eproto ::google::protobuf::internal::SCCInfo<2> scc_info_Header;       |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                           
      |        PROTOBUF_INTERNAL_EXPORT_protobuf_gz_2fmsgs_2fheader_2eproto
/home/chapulina/dev_focal/ws_garden/build/ignition-gazebo7/src/msgs/peer_control.pb.cc:50:51: error: _scc_info_Header_ is not a member of _p
rotobuf_ignition_2fmsgs_2fheader_2eproto_                                                                                                   
   50 |       &protobuf_ignition_2fmsgs_2fheader_2eproto::scc_info_Header.base,}};                                                                |                                                   ^~~~~~~~~~~~~~~                                       
/home/chapulina/dev_focal/ws_garden/build/ignition-gazebo7/src/msgs/peer_control.pb.cc: In function _void protobuf_peer_5fcontrol_2eproto::A
ddDescriptorsImpl()_:                                                                                                                       
/home/chapulina/dev_focal/ws_garden/build/ignition-gazebo7/src/msgs/peer_control.pb.cc:105:48: error: _AddDescriptors_ is not a member of _p
rotobuf_ignition_2fmsgs_2fheader_2eproto_                                                                                                   
  105 |   ::protobuf_ignition_2fmsgs_2fheader_2eproto::AddDescriptors();                                                                    
      |                                                ^~~~~~~~~~~~~~ 

I believe that's coming from here.

include/gz/msgs/SuppressWarning.hh Outdated Show resolved Hide resolved
include/ignition/msgs/detail/PointCloudPackedUtils.hh Outdated Show resolved Hide resolved
conf/CMakeLists.txt Outdated Show resolved Hide resolved
methylDragon added a commit that referenced this pull request May 3, 2022
methylDragon added a commit that referenced this pull request May 3, 2022
methylDragon added a commit that referenced this pull request May 3, 2022
methylDragon added a commit that referenced this pull request May 3, 2022
Signed-off-by: methylDragon <[email protected]>
methylDragon added a commit that referenced this pull request May 4, 2022
methylDragon added a commit that referenced this pull request May 4, 2022
methylDragon added a commit that referenced this pull request May 4, 2022
Signed-off-by: methylDragon <[email protected]>
methylDragon added a commit that referenced this pull request May 4, 2022
methylDragon added a commit that referenced this pull request May 4, 2022
methylDragon added a commit that referenced this pull request May 6, 2022
methylDragon added a commit that referenced this pull request May 6, 2022
methylDragon added a commit that referenced this pull request May 6, 2022
Signed-off-by: methylDragon <[email protected]>
@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@e61edda). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #249   +/-   ##
=======================================
  Coverage        ?   15.90%           
=======================================
  Files           ?      382           
  Lines           ?    71409           
  Branches        ?        0           
=======================================
  Hits            ?    11361           
  Misses          ?    60048           
  Partials        ?        0           

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 e61edda...5b97333. Read the comment docs.

methylDragon added a commit that referenced this pull request May 7, 2022
Signed-off-by: methylDragon <[email protected]>
methylDragon added a commit that referenced this pull request May 7, 2022
methylDragon added a commit that referenced this pull request May 7, 2022
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Copy link
Contributor Author

Going with the deferred hard-tock option (B) with gz headers directing to ignition for now, and ignition being the "canonical" message definitions at the moment.

image

Gazebo builds.

Once we're ready, and namespaces have been migrated, we will then move forward with hard-tocking every message (and message header) to gz.

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.

LGTM, this shouldn't break any downstream libraries as it is.

We'll come back for the hard tock later. I'd like to at least make the C++ redirection headers work, because that's what most people are using.

The proto namespaces are only used by people creating custom messages or doing some other kind of advanced usage of the messages. Ideally we'd tick-tick the change, but it's proving difficult, so I think those advanced users will have to deal with a hard-tock. Fortunately, the migration only requires a quick search-and-replace on their end.

@@ -1,5 +1,5 @@
/*
* Copyright (C) 2016 Open Source Robotics Foundation
* Copyright (C) 2017 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! 684b316

I'll merge once CI is green 🙌

@methylDragon methylDragon merged commit 03a7434 into main May 10, 2022
@methylDragon methylDragon deleted the header_migration branch May 10, 2022 23:48
methylDragon added a commit that referenced this pull request May 10, 2022
methylDragon added a commit that referenced this pull request May 10, 2022
methylDragon added a commit that referenced this pull request May 10, 2022
WilliamLewww pushed a commit to WilliamLewww/ign-msgs that referenced this pull request May 25, 2022
WilliamLewww pushed a commit to WilliamLewww/ign-msgs that referenced this pull request May 25, 2022
Signed-off-by: methylDragon <[email protected]>
Signed-off-by: William Lew <[email protected]>
WilliamLewww pushed a commit to WilliamLewww/ign-msgs that referenced this pull request May 25, 2022
Signed-off-by: methylDragon <[email protected]>
Signed-off-by: William Lew <[email protected]>
WilliamLewww pushed a commit to WilliamLewww/ign-msgs that referenced this pull request May 25, 2022
Signed-off-by: methylDragon <[email protected]>
Signed-off-by: William Lew <[email protected]>
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.

2 participants