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

Restore message generation pipeline and generate message libraries #368

Merged
merged 23 commits into from
Aug 28, 2023

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Jul 27, 2023

This it the final rework of the message generation pipeline for Harmonic.

After trying multiple approaches, we decided to keep the messages in one location and generate the library as part of gz-msgs to derisk the long term supported Harmonic release.

This PR adds the new message generation pipeline, but uses it internally to keep the API similar to previous releases.

Also needs: gazebosim/gz-sensors#375
Also needs: gazebosim/gz-gui#563

cmake/gz_msgs_generate.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #368 (c1fea4b) into main (a8b2d84) will increase coverage by 0.17%.
The diff coverage is 98.13%.

❗ Current head c1fea4b differs from pull request most recent head 7a062af. Consider uploading reports for the commit 7a062af to get more accurate results

@@            Coverage Diff             @@
##             main     #368      +/-   ##
==========================================
+ Coverage   97.03%   97.21%   +0.17%     
==========================================
  Files           9       27      +18     
  Lines        1081     1150      +69     
==========================================
+ Hits         1049     1118      +69     
  Misses         32       32              
Files Changed Coverage Δ
core/generator/generator_main.cc 100.00% <ø> (ø)
...re/include/gz/msgs/detail/PointCloudPackedUtils.hh 94.80% <ø> (ø)
core/src/gz.cc 90.47% <ø> (ø)
core/src/DynamicFactory.cc 87.09% <87.09%> (ø)
core/src/Factory.cc 87.50% <87.50%> (ø)
...re/include/gz/msgs/convert/SphericalCoordinates.hh 92.85% <92.85%> (ø)
core/src/MessageFactory.cc 94.44% <94.44%> (ø)
core/generator/Generator.cc 92.64% <100.00%> (ø)
core/include/gz/msgs/Factory.hh 100.00% <100.00%> (ø)
core/include/gz/msgs/MessageFactory.hh 100.00% <100.00%> (ø)
... and 17 more

@mjcarroll
Copy link
Contributor Author

@j-rivero is there any way to run PR jobs against jammy+? In this case, the protobuf version is missing some APIs in focal

@azeey
Copy link
Contributor

azeey commented Aug 4, 2023

You can ignore the Focal builds. The Jammy job is already in the PR( gz-msgs-ci-pr_any-jammy-amd64) . I don't think we can remove the Focal jobs from an open PR, but new PRs should only run on Jammy on Jenkins.

@mjcarroll mjcarroll force-pushed the mjcarroll/message_library_generation branch from 7030b00 to 3e8971d Compare August 17, 2023 13:48
@mjcarroll mjcarroll marked this pull request as ready for review August 17, 2023 13:49
@mjcarroll mjcarroll requested a review from caguero as a code owner August 17, 2023 13:49
@mjcarroll mjcarroll force-pushed the mjcarroll/message_library_generation branch from 3e8971d to fb8b36f Compare August 17, 2023 13:54
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll force-pushed the mjcarroll/message_library_generation branch from fb8b36f to 7e7e0b4 Compare August 17, 2023 14:59
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll force-pushed the mjcarroll/message_library_generation branch from 7aa4667 to f879398 Compare August 21, 2023 20:48
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll force-pushed the mjcarroll/message_library_generation branch from 56b88be to aeafe56 Compare August 21, 2023 21:44
@mjcarroll mjcarroll force-pushed the mjcarroll/message_library_generation branch from 74de63c to 8c5d9f0 Compare August 22, 2023 13:44
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/target_link_messages.cmake Outdated Show resolved Hide resolved
cmake/gz_msgs_generate.cmake Show resolved Hide resolved
core/src/DynamicFactory.cc Show resolved Hide resolved
core/src/DynamicFactory.cc Outdated Show resolved Hide resolved
core/src/DynamicFactory.hh Show resolved Hide resolved
core/src/gz.cc Outdated Show resolved Hide resolved
core/src/gz.cc Outdated Show resolved Hide resolved
core/CMakeLists.txt Show resolved Hide resolved
examples/generating_custom_msgs/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few more minor comments.

core/include/gz/msgs/convert/SphericalCoordinates.hh Outdated Show resolved Hide resolved
core/src/DynamicFactory.cc Show resolved Hide resolved
core/src/DynamicFactory.hh Show resolved Hide resolved
core/CMakeLists.txt Show resolved Hide resolved
gz-msgs-extras.cmake.in Show resolved Hide resolved
examples/using_gz_msgs/README.md Outdated Show resolved Hide resolved
gz-msgs-extras.cmake.in Outdated Show resolved Hide resolved
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for iterating.

gazebosim/gz-sim#2087 probably needs to be merged before we merge this.

@mjcarroll
Copy link
Contributor Author

gazebosim/gz-sim#2087 probably needs to be merged before we merge this.

2087 depends on this one being merged/released.

@azeey
Copy link
Contributor

azeey commented Aug 28, 2023

gazebosim/gz-sim#2087 probably needs to be merged before we merge this.

2087 depends on this one being merged/released.

Okay, let's merge this then.

@azeey azeey merged commit f41e1e2 into main Aug 28, 2023
6 checks passed
@azeey azeey deleted the mjcarroll/message_library_generation branch August 28, 2023 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants