Use HIDE_SYMBOLS_BY_DEFAULT, fix MovingWindowFilter

[j-rivero]

🦟 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.

[codecov]

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]

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]

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:

1. Pass HIDE_SYMBOLS_BY_DEFAULT to the gz_configure_build calls in every gz-* package
2. Test and verify that CI / nightly builds are happy
3. Remove HIDE_SYMBOLS_BY_DEFAULT from the gz_configure_build calls in every gz-* package
4. 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]

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:

1. Pass HIDE_SYMBOLS_BY_DEFAULT to the gz_configure_build calls in every gz-* package
2. Test and verify that CI / nightly builds are happy
3. Remove HIDE_SYMBOLS_BY_DEFAULT from the gz_configure_build calls in every gz-* package
4. 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]

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]

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

[azeey]

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

[j-rivero]

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

[scpeters]

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

[azeey]

@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]

@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

[scpeters]

@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]

I'll wait for @azeey to approve