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

Avoid symbol redefition to fix armel builds #457

Merged
merged 6 commits into from Oct 13, 2021

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Oct 7, 2021

🦟 Bug fix

Summary

armel builds (aka armhf) are particularly picky with the c++ standard due to the number of optimizations that they do. armhf build of ign-rendering6 failed with the following:

[ 17%] Linking CXX shared library ../lib/libignition-rendering6.so
cd /var/lib/jenkins/workspace/ign-rendering6-debbuilder/build/ignition-rendering-6.0.1/obj-arm-linux-gnueabihf/src && /usr/bin/cmake -E cmake_link_script CMakeFiles/ignition-rendering6.dir/link.txt --verbose=1
/usr/bin/c++ -fPIC -g -O2 -fdebug-prefix-map=/var/lib/jenkins/workspace/ign-rendering6-debbuilder/build/ignition-rendering-6.0.1=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -O2 -g -DNDEBUG  -Wall -Wextra -Wno-long-long -Wno-unused-value -Wfloat-equal -Wshadow -Winit-self -Wswitch-default -Wmissing-include-dirs -pedantic -UNDEBUG -Wl,-Bsymbolic-functions -Wl,-z,relro -shared -Wl,-soname,libignition-rendering6.so.6 -o ../lib/libignition-rendering6.so.6.0.1 CMakeFiles/ignition-rendering6.dir/BoundingBox.cc.o CMakeFiles/ignition-rendering6.dir/GaussianNoisePass.cc.o CMakeFiles/ignition-rendering6.dir/HeightmapDescriptor.cc.o CMakeFiles/ignition-rendering6.dir/Image.cc.o CMakeFiles/ignition-rendering6.dir/LidarVisual.cc.o CMakeFiles/ignition-rendering6.dir/Marker.cc.o CMakeFiles/ignition-rendering6.dir/MeshDescriptor.cc.o CMakeFiles/ignition-rendering6.dir/MoveToHelper.cc.o CMakeFiles/ignition-rendering6.dir/OrbitViewController.cc.o CMakeFiles/ignition-rendering6.dir/OrthoViewController.cc.o CMakeFiles/ignition-rendering6.dir/PixelFormat.cc.o CMakeFiles/ignition-rendering6.dir/RenderEngineManager.cc.o CMakeFiles/ignition-rendering6.dir/RenderEnginePlugin.cc.o CMakeFiles/ignition-rendering6.dir/RenderPassSystem.cc.o CMakeFiles/ignition-rendering6.dir/RenderingIface.cc.o CMakeFiles/ignition-rendering6.dir/ShaderParam.cc.o CMakeFiles/ignition-rendering6.dir/ShaderParams.cc.o CMakeFiles/ignition-rendering6.dir/ShaderType.cc.o CMakeFiles/ignition-rendering6.dir/TransformController.cc.o CMakeFiles/ignition-rendering6.dir/Utils.cc.o CMakeFiles/ignition-rendering6.dir/WireBox.cc.o CMakeFiles/ignition-rendering6.dir/base/BaseObject.cc.o CMakeFiles/ignition-rendering6.dir/base/BaseRenderEngine.cc.o CMakeFiles/ignition-rendering6.dir/base/BaseScene.cc.o  -lX11 /usr/lib/arm-linux-gnueabihf/libignition-common4-graphics.so.4.3.0 /usr/lib/arm-linux-gnueabihf/libignition-utils1.so.1.1.0 /usr/lib/arm-linux-gnueabihf/libignition-common4-events.so.4.3.0 /usr/lib/arm-linux-gnueabihf/libignition-common4.so.4.3.0 /usr/lib/arm-linux-gnueabihf/libuuid.so /usr/lib/arm-linux-gnueabihf/libuuid.so -lpthread /usr/lib/arm-linux-gnueabihf/libignition-math6.so.6.9.1 /usr/lib/arm-linux-gnueabihf/libignition-plugin1-loader.so.1.2.1 /usr/lib/arm-linux-gnueabihf/libignition-plugin1.so.1.2.1 
/usr/bin/ld: CMakeFiles/ignition-rendering6.dir/OrbitViewController.cc.o:(.rodata+0x0): multiple definition of `typeinfo name for ignition::rendering::v6::Store<ignition::rendering::v6::Scene>'; CMakeFiles/ignition-rendering6.dir/MoveToHelper.cc.o:(.rodata+0x0): first defined here
/usr/bin/ld: CMakeFiles/ignition-rendering6.dir/OrbitViewController.cc.o:(.data.rel.ro+0x0): multiple definition of `typeinfo for ignition::rendering::v6::Store<ignition::rendering::v6::Scene>'; CMakeFiles/ignition-rendering6.dir/MoveToHelper.cc.o:(.data.rel.ro+0x0): first defined here
/usr/bin/ld: CMakeFiles/ignition-rendering6.dir/OrbitViewController.cc.o:(.rodata+0x2c): multiple definition of `typeinfo name for ignition::rendering::v6::Store<ignition::rendering::v6::Node>'; CMakeFiles/ignition-rendering6.dir/MoveToHelper.cc.o:(.rodata+0x2c): first defined here
/usr/bin/ld: CMakeFiles/ignition-rendering6.dir/OrbitViewController.cc.o:(.data.rel.ro+0x8): multiple definition of `typeinfo for ignition::rendering::v6::Store<ignition::rendering::v6::Node>'; CMakeFiles/ignition-rendering6.dir/MoveToHelper.cc.o:(.data.rel.ro+0x8): first defined here
/usr/bin/ld: CMakeFiles/ignition-rendering6.dir/OrbitViewController.cc.o:(.rodata+0x58): multiple definition of `typeinfo name for ignition::rendering::v6::Store<ignition::rendering::v6::Light>'; CMakeFiles/ignition-rendering6.dir/MoveToHelper.cc.o:(.rodata+0x58): first defined here
/usr/bin/ld: CMakeFiles/ignition-rendering6.dir/OrbitViewController.cc.o:(.data.rel.ro+0x10): multiple definition of `typeinfo for ignition::rendering::v6::Store<ignition::rendering::v6::Light>'; CMakeFiles/ignition-rendering6.dir/MoveToHelper.cc.o:(.data.rel.ro+0x10): first defined here
/usr/bin/ld: CMakeFiles/ignition-rendering6.dir/OrbitViewController.cc.o:(.rodata+0x84): multiple definition of `typeinfo name for 
... (snipped)

Looking at the code I found some template class declarations that somehow looked to me superflous. I tried removing them and the whole thing compiled in my local machine (amd64). When I ported the changes to debbuilder (via patch in debian) things seems to work.

armhf for Focal fixes with this patch here: Build Status

I did not look more for the original declaration/definitions of these symbols or if there were other reasons to make these declarations in these places. Let's see if the rest of the platforms are happy with the changes.

Checklist

  • Signed all commits for DCO

@j-rivero j-rivero requested a review from iche033 as a code owner October 7, 2021 21:15
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Oct 7, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Oct 7, 2021
@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #457 (88ca80d) into ign-rendering3 (43f53aa) will not change coverage.
The diff coverage is n/a.

❗ Current head 88ca80d differs from pull request most recent head 7244aaa. Consider uploading reports for the commit 7244aaa to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-rendering3     #457   +/-   ##
===============================================
  Coverage           53.35%   53.35%           
===============================================
  Files                 131      131           
  Lines               12035    12035           
===============================================
  Hits                 6421     6421           
  Misses               5614     5614           
Impacted Files Coverage Δ
include/ignition/rendering/Storage.hh 25.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43f53aa...7244aaa. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Oct 7, 2021

no related test failures but ABI checker is complaining though:
https://build.osrfoundation.org/job/ignition_rendering-abichecker-any_to_any-ubuntu_auto-amd64/738/API_5fABI_20report/

@chapulina chapulina moved this from Inbox to In review in Core development Oct 7, 2021
j-rivero and others added 4 commits October 12, 2021 22:33
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: GitHub <noreply@github.com>

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
This reverts commit 6e25442.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@j-rivero
Copy link
Contributor Author

no related test failures but ABI checker is complaining though:

I just changed to only remove the symbols on armhf. The code was released without them, should be fine to remove them in that platform.

@iche033
Copy link
Contributor

iche033 commented Oct 13, 2021

one minor codecheck error, otherwise looks good:

/github/workspace/ogre/include/ignition/rendering/ogre/OgreStorage.hh:37:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@j-rivero j-rivero merged commit e61ff3c into gazebosim:ign-rendering3 Oct 13, 2021
Core development automation moved this from In review to Done Oct 13, 2021
iche033 added a commit that referenced this pull request Oct 19, 2021
* ogre: Do not assume that ogre plugins have lib prefix on macOS (#454)

Signed-off-by: Silvio <silvio@traversaro.it>

* Fix compilation against Ogre 1.10.12 (#390)

Signed-off-by: GitHub <noreply@github.com>

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>

* Avoid symbol redefition to fix armel builds (#457)

* Avoid symbol redefiniition on armel builds

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

* Fix logic on warning for ogre versions different than 1.9.x (#465)

* Fix logic on warning for ogre versions different than 1.9.x

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

* Fix context attributes of glXCreateContextAttribsARB. (#460)

Signed-off-by: Hill Ma <hillma@google.com>

Co-authored-by: Silvio Traversaro <silvio@traversaro.it>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Co-authored-by: Hill Ma <mahiuchun@users.noreply.github.com>
srmainwaring pushed a commit to srmainwaring/gz-rendering that referenced this pull request Oct 21, 2021
* Avoid symbol redefiniition on armel builds

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
WilliamLewww pushed a commit to WilliamLewww/ign-rendering that referenced this pull request Dec 7, 2021
* Avoid symbol redefiniition on armel builds

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants