Use HIDE_SYMBOLS_BY_DEFAULT, fix MovingWindowFilter by j-rivero · Pull Request #566 · gazebosim/gz-math

j-rivero · 2023-11-03T15:17:58Z

🦟 Bug fix

As part of the works of defining hidden by default in gazebosim/gz-cmake#392, this PR moves the whole MovingWindowFilter class to be header only. Reason for this is that GCC on Jammy seems buggy to me (clang11 on Jammy worked fine, also clang on Mac and MSVC on Windows deal with it just fine) when it needs to generate the symbols needed for the specialization of the class done for <gz::math::Vector3i>. See efforts in #565.

Summary

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.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

codecov · 2023-11-03T15:18:31Z

Codecov Report

Merging #566 (c94ebba) into main (beac11e) will not change coverage.
The diff coverage is 100.00%.

❗ Current head c94ebba differs from pull request most recent head 17970d7. Consider uploading reports for the commit 17970d7 to get more accurate results

@@           Coverage Diff           @@
##             main     #566   +/-   ##
=======================================
  Coverage   96.18%   96.18%           
=======================================
  Files         143      142    -1     
  Lines        9826     9826           
=======================================
  Hits         9451     9451           
  Misses        375      375

Files	Coverage Δ
include/gz/math/MovingWindowFilter.hh	`100.00% <100.00%> (ø)`

azeey · 2023-11-03T16:02:27Z

Since gz-cmake already has HIDDEN_SYMBOLS_BY_DEFAULT, maybe we should use that in all the libraries first, make sure all CI passes and then merge gazebosim/gz-cmake#392. Thoughts?

scpeters · 2023-11-04T00:08:49Z

Since gz-cmake already has HIDDEN_SYMBOLS_BY_DEFAULT, maybe we should use that in all the libraries first, make sure all CI passes and then merge gazebosim/gz-cmake#392. Thoughts?

this sounds like you are suggesting the following:

Pass HIDE_SYMBOLS_BY_DEFAULT to the gz_configure_build calls in every gz-* package
Test and verify that CI / nightly builds are happy
Remove HIDE_SYMBOLS_BY_DEFAULT from the gz_configure_build calls in every gz-* package
Merge Use visibility hidden by default gz-cmake#392

If we merge gazebosim/gz-cmake#392 before removing the HIDE_SYMBOLS_BY_DEFAULT parameters, I think it may introduce cmake deprecation warnings. I guess if we split out the deprecation of HIDE_SYMBOLS_BY_DEFAULT, then we could do things in a slightly different order, but it would require an extra gz-cmake pull request

scpeters · 2023-11-04T00:14:09Z

Since gz-cmake already has HIDDEN_SYMBOLS_BY_DEFAULT, maybe we should use that in all the libraries first, make sure all CI passes and then merge gazebosim/gz-cmake#392. Thoughts?

this sounds like you are suggesting the following:

Pass HIDE_SYMBOLS_BY_DEFAULT to the gz_configure_build calls in every gz-* package

Test and verify that CI / nightly builds are happy

Remove HIDE_SYMBOLS_BY_DEFAULT from the gz_configure_build calls in every gz-* package

Merge Use visibility hidden by default gz-cmake#392

If we merge gazebosim/gz-cmake#392 before removing the HIDE_SYMBOLS_BY_DEFAULT parameters, I think it may introduce cmake deprecation warnings. I guess if we split out the deprecation of HIDE_SYMBOLS_BY_DEFAULT, then we could do things in a slightly different order, but it would require an extra gz-cmake pull request

it sounds a little like overkill, but I think it would be the most thorough approach since it would run CI on every single package

azeey · 2023-11-06T17:32:06Z

it sounds a little like overkill, but I think it would be the most thorough approach since it would run CI on every single package

Maybe a little overkill, but my gut feeling is that it won't be straightforward to get all the packages to build and test properly with symbols hidden by default, so I'd like CI to run on the PRs rather than just local testing. If we can do that without using the HIDE_SYMBOLS_BY_DEFAULT approach, I'm all for it.

scpeters · 2023-11-06T18:07:16Z

it sounds a little like overkill, but I think it would be the most thorough approach since it would run CI on every single package

Maybe a little overkill, but my gut feeling is that it won't be straightforward to get all the packages to build and test properly with symbols hidden by default, so I'd like CI to run on the PRs rather than just local testing. If we can do that without using the HIDE_SYMBOLS_BY_DEFAULT approach, I'm all for it.

I opened gazebosim/gz-utils#115 for example. I think it will take some additional development from our infrastructure team to support testing all packages from source on Ubuntu in CI

Part of testing gazebosim/gz-cmake#392 Signed-off-by: Steve Peters <scpeters@openrobotics.org> Signed-off-by: Steve Peters <scpeters@openrobotics.org>

azeey · 2023-11-06T21:41:44Z

include/gz/math/MovingWindowFilter.hh

+  }
+}
+
+template class MovingWindowFilter<int>;


Do we still need these if it's a header-only?

You are right, I don't think so. 56f145f

…' into jrivero/visibility_fixes_linux_single_header

scpeters · 2023-11-06T23:38:28Z

I opened #567 to set HIDE_SYMBOLS_BY_DEFAULT confirmed that CI fails, then I merged the changes into this PR

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

azeey · 2023-11-07T17:08:19Z

@j-rivero Looks like there are two homebrew jobs? And both of them are failing trying to install either gz-math or ign-math8, neither of which are available.

scpeters · 2023-11-07T17:58:21Z

@j-rivero Looks like there are two homebrew jobs? And both of them are failing trying to install either gz-math or ign-math8, neither of which are available.

I think it's due to gazebo-tooling/release-tools#1054

Signed-off-by: Steve Peters <scpeters@openrobotics.org> Signed-off-by: Steve Peters <scpeters@openrobotics.org>

scpeters · 2023-11-07T23:22:15Z

@j-rivero Looks like there are two homebrew jobs? And both of them are failing trying to install either gz-math or ign-math8, neither of which are available.

I pushed a whitespace change (17970d7) in order to re-run CI, and it's happy now

scpeters · 2023-11-07T23:23:03Z

I'll wait for @azeey to approve

Use single header for MovingWindowFilter

55fbcc6

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

j-rivero requested review from adityapande-1995 and scpeters as code owners November 3, 2023 15:17

j-rivero mentioned this pull request Nov 6, 2023

Library symbols should be hidden by default gazebosim/gz-cmake#166

Closed

Use HIDE_SYMBOLS_BY_DEFAULT

75a19c7

Part of testing gazebosim/gz-cmake#392 Signed-off-by: Steve Peters <scpeters@openrobotics.org> Signed-off-by: Steve Peters <scpeters@openrobotics.org>

scpeters mentioned this pull request Nov 6, 2023

Use HIDE_SYMBOLS_BY_DEFAULT #567

Closed

9 tasks

azeey reviewed Nov 6, 2023

View reviewed changes

Merge remote-tracking branch 'origin/scpeters/hide_symbols_by_default…

082cda1

…' into jrivero/visibility_fixes_linux_single_header

Remove specialization types

56f145f

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

adjust whitespace

17970d7

Signed-off-by: Steve Peters <scpeters@openrobotics.org> Signed-off-by: Steve Peters <scpeters@openrobotics.org>

azeey approved these changes Nov 7, 2023

View reviewed changes

scpeters changed the title ~~Change MovingWindowFilter to be a header only class~~ Use HIDE_SYMBOLS_BY_DEFAULT, fix MovingWindowFilter Nov 8, 2023

scpeters merged commit ba26674 into main Nov 8, 2023

scpeters deleted the jrivero/visibility_fixes_linux_single_header branch November 8, 2023 03:00

Conversation

j-rivero commented Nov 3, 2023

🦟 Bug fix

Summary

Checklist

Uh oh!

codecov bot commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

azeey commented Nov 3, 2023

Uh oh!

scpeters commented Nov 4, 2023

Uh oh!

scpeters commented Nov 4, 2023

Uh oh!

azeey commented Nov 6, 2023

Uh oh!

scpeters commented Nov 6, 2023

Uh oh!

azeey Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

j-rivero Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

scpeters commented Nov 6, 2023

Uh oh!

azeey commented Nov 7, 2023

Uh oh!

scpeters commented Nov 7, 2023

Uh oh!

scpeters commented Nov 7, 2023

Uh oh!

scpeters commented Nov 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 3, 2023 •

edited

Loading