Skip to content

Fix compatibility with protobuf 22#405

Merged
scpeters merged 1 commit intogazebosim:ign-transport8from
traversaro:fix404
Jun 10, 2023
Merged

Fix compatibility with protobuf 22#405
scpeters merged 1 commit intogazebosim:ign-transport8from
traversaro:fix404

Conversation

@traversaro
Copy link
Contributor

@traversaro traversaro commented Jun 6, 2023

🎉 New feature

Fix #404 by adding compatibility with protobuf >= 22 .

Summary

Similar to gazebosim/gz-msgs#346, but with an additional fix to change the removed google::protobuf::down_cast to google::protobuf::internal::DownCast . It may not be ideal to use a function declared in an internal:: namepsace, but that how actually the method was called in protobuf 2 when it was first used.

Test it

Install protobuf 22 or 23, and compile gz-citadel against it.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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.

@traversaro traversaro requested a review from caguero as a code owner June 6, 2023 15:23
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jun 6, 2023
@traversaro traversaro force-pushed the fix404 branch 2 times, most recently from 360e8fe to a6162eb Compare June 6, 2023 15:46
caguero
caguero previously requested changes Jun 7, 2023

#if GOOGLE_PROTOBUF_VERSION > 2999999
#if GOOGLE_PROTOBUF_VERSION >= 4022000
auto msgReq =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Remove extra space at the end of the line to make the style checker happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Signed-off-by: Silvio Traversaro <silvio@traversaro.it>
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #405 (2b80785) into ign-transport8 (cb1c95b) will decrease coverage by 0.05%.
The diff coverage is 88.10%.

❗ Current head 2b80785 differs from pull request most recent head e35a697. Consider uploading reports for the commit e35a697 to get more accurate results

@@                Coverage Diff                 @@
##           ign-transport8     #405      +/-   ##
==================================================
- Coverage           83.61%   83.56%   -0.05%     
==================================================
  Files                  51       51              
  Lines                5035     5039       +4     
==================================================
+ Hits                 4210     4211       +1     
- Misses                825      828       +3     
Impacted Files Coverage Δ
include/gz/transport/Packet.hh 0.00% <0.00%> (ø)
...og/include/gz/transport/log/detail/QueryOptions.hh 100.00% <ø> (ø)
log/src/Batch.cc 89.28% <ø> (ø)
log/src/Descriptor.cc 89.65% <ø> (ø)
log/src/Descriptor.hh 100.00% <ø> (ø)
log/src/Log.cc 78.72% <ø> (ø)
log/src/Message.cc 100.00% <ø> (ø)
log/src/MsgIter.cc 80.43% <ø> (ø)
log/src/QualifiedTime.cc 97.81% <ø> (ø)
log/src/QueryOptions.cc 100.00% <ø> (ø)
... and 40 more

@scpeters scpeters merged commit 3d68f46 into gazebosim:ign-transport8 Jun 10, 2023
scpeters pushed a commit that referenced this pull request Jun 10, 2023
Signed-off-by: Silvio Traversaro <silvio@traversaro.it>
scpeters pushed a commit that referenced this pull request Jun 15, 2023
Signed-off-by: Silvio Traversaro <silvio@traversaro.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏰 citadel Ignition Citadel

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants