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

Deprecate LOCAL2 in SphericalCoordinates #450

Closed
wants to merge 1 commit into from

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Aug 19, 2024

🦟 Bug fix

Summary

cc: @peci

With the introduction of gazebosim/gz-math#616. We will be deprecating LOCAL2 as a work around in favor or LOCAL. As part of this change, its probably a good idea to update gz-msgs as well.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

With the introduction of gazebosim/gz-math#616.
We will be deprecating `LOCAL2` as a work around in favor or `LOCAL`. As
part of this change, its probably a good idea to update `gz-msgs` as
well.

Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 requested a review from caguero as a code owner August 19, 2024 07:53
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Aug 19, 2024
@azeey azeey added the beta Targeting beta release of upcoming collection label Aug 19, 2024
@iche033
Copy link
Contributor

iche033 commented Aug 20, 2024

there is one gcc deprecation warning left: https://build.osrfoundation.org/job/gz_msgs-ci-pr_any-noble-amd64/7/gcc/

@arjo129
Copy link
Contributor Author

arjo129 commented Aug 20, 2024

Yeah, I'm not exactly sure how to solve this as the warning is coming from the generated protobuf. It seems like the Proto compiler reuses the highest value in the enum. There are several options. Perhaps I could introduce a DO_NOT_USE as the highest proto enum value?

@Crola1702
Copy link

This change in gazebosim/gz-math#616 triggered a regression in gz-transport14-debbuilder:

Reference build:

Log output:

/usr/lib/x86_64-linux-gnu/pkgconfig/../../../include/gz/msgs11/gz/msgs/convert/SphericalCoordinates.hh:80:54: warning: ‘gz::math::v8::SphericalCoordinates::LOCAL2’ is deprecated [-Wdeprecated-declarations]
   80 |   return math::SphericalCoordinates::CoordinateType::LOCAL2;
      |                                                      ^~~~~~
/usr/lib/x86_64-linux-gnu/pkgconfig/../../../include/gz/math8/gz/math/SphericalCoordinates.hh:82:15: note: declared here
   82 |               LOCAL2 GZ_LOCAL2_DEPRECATED = 5
      |               ^~~~~~
autopkgtest [11:18:31]: @@@@@@@@@@@@@@@@@@@@ summary
build                FAIL stderr: In file included from /usr/lib/x86_64-linux-gnu/pkgconfig/../../../include/gz/msgs11/gz/msgs/Utility.hh:27,
autopkgtest [11:18:31]: Binaries: resetting testbed apt configuration

@arjo129 can you please take a look?

@peci1
Copy link
Contributor

peci1 commented Aug 20, 2024

@Crola1702 Those errors should be fixed by this PR.

Copy link
Contributor

@peci1 peci1 left a comment

Choose a reason for hiding this comment

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

This PR looks good, but there still remain the warnings from protobuf-generated files. I fixed that in #451.

@arjo129
Copy link
Contributor Author

arjo129 commented Aug 22, 2024

Thanks @peci1! Will use that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants