gazebosim / sdformat

Simulation Description Format (SDFormat) parser and description files.

http://sdformat.org

Apache License 2.0

169 stars 98 forks source link

Fix symbol checking test when compiled with debug symbols #1474

Closed j-rivero closed 3 months ago

j-rivero commented 3 months ago

🦟 Bug fix

Summary

The test that checks for prefixed binary symbols was broken when compiled with DebWithRelInfo since it was checking debugging symbols that broke the heuristics used.

The commit fixes it doing a couple of actions:

Include the length of the namespace sdf according to the mangling rules: 3sdf
Check only dynamic symbols being exported

Checklist

[x] 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.

j-rivero commented 3 months ago

Tested (pending):

scpeters commented 3 months ago

Tested (pending):

trying again with different branch parameters

https://build.osrfoundation.org/job/sdformat15-debbuilder/530/

azeey commented 3 months ago

I was also working on this and found that adding the --extern-only flag to nm fixes the issue. From what I can understand, the issue is that debbuilders enable LTO which causes some debug symbols to be added to the shared library. These symbols contain the file names, one of which is :

0000000000f9c8ce N sdformat15_get_install_prefix_impl.cc.b80149f8

This causes a false negative on the versioned symbol test.

I tested by adding set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE) in the root CMakeLists.txt file and was able to reproduce the test failure locally.

In general, it would be good to enable LTO on our libraries so these types of issues are caught at the PR stage. I think this is the third time this LTO stuff has bit us.

scpeters commented 3 months ago

In general, it would be good to enable LTO on our libraries so these types of issues are caught at the PR stage. I think this is the third time this LTO stuff has bit us.

should we do this in the action-gz-ci workflow?

azeey commented 3 months ago

In general, it would be good to enable LTO on our libraries so these types of issues are caught at the PR stage. I think this is the third time this LTO stuff has bit us.

should we do this in the action-gz-ci workflow?

I was thinking of enabling it via CMake, maybe in gz-cmake or on a per-project basis. That way local development will also use the same flags.

scpeters commented 3 months ago

@Mergifyio backport sdf15 sdf14

mergify[bot] commented 3 months ago

backport sdf15 sdf14

✅ Backports have been created

* [#1475 Fix symbol checking test when compiled with debug symbols (backport #1474)](https://github.com/gazebosim/sdformat/pull/1475) has been created for branch `sdf15` * [#1476 Fix symbol checking test when compiled with debug symbols (backport #1474)](https://github.com/gazebosim/sdformat/pull/1476) has been created for branch `sdf14`