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

ogre: Do not assume that ogre plugins have lib prefix on macOS #454

Merged
merged 1 commit into from Oct 7, 2021

Conversation

traversaro
Copy link
Contributor

🦟 Bug fix

Fix most of the test suite when running against ogre with conda-forge-provided dependencies on macOS.

Summary

By default, ogre plugins (the one that are installed under lib\OGRE\<..> and that are loaded at runtime) are not built with the lib prefix on any platform. However, for legacy reasons (see comment in https://github.com/osrf/homebrew-simulation/blame/f27e070d08600f77933f5608df8d413bc99b2206/Formula/ogre1.9.rb#L105) the ogre1.9 contained both the deprecated lib\OGRE\libRenderSystem_GL.dylib and the recommended to use lib\OGRE\RenderSystem_GL.dylib. Furthermore, the ignition-rendering ogre plugin only looked for the library starting with lib on macOS, creating runtime problems on macOS distributions in which ogre did not had the plugin that started with lib, such as conda-forge, but I imagine that on vcpkg on macOS you could get a similar problem.

This PR switches also igniton-rendering to only look for ogre plugins without the lib prefix. It is basically the backport of the gazebo commit gazebosim/gazebo-classic@2dd748d#diff-ed2a31885765b0b34495723b06e6ce939e978bb42f5c082a9b93b9fec20257a2 from gazebo8/2017 that was never forward ported to ignition-rendering.

As the homebrew bottles provide ogre plugins without the lib prefix since at least 5 years (see comment https://github.com/osrf/homebrew-simulation/blame/f27e070d08600f77933f5608df8d413bc99b2206/Formula/ogre1.9.rb#L105) I think there is a low risk of creating problems (even because if anyone has a system in which gazebo classic works fine, then also ignition-rendering will work fine after this PR).

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

@traversaro
Copy link
Contributor Author

(I will fix the DCO as soon as I am back to my laptop : ) )

@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #454 (25abfaa) into ign-rendering3 (9ed136b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           ign-rendering3     #454      +/-   ##
==================================================
- Coverage           53.35%   53.35%   -0.01%     
==================================================
  Files                 131      131              
  Lines               12036    12035       -1     
==================================================
- Hits                 6422     6421       -1     
  Misses               5614     5614              
Impacted Files Coverage Δ
ogre/src/OgreRenderEngine.cc 68.38% <100.00%> (-0.10%) ⬇️

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 9ed136b...25abfaa. Read the comment docs.

Signed-off-by: Silvio <silvio@traversaro.it>
@traversaro
Copy link
Contributor Author

(I will fix the DCO as soon as I am back to my laptop : ) )

Done.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

thanks, yeah looks like we just didn't port these changes over from gazebo.

Looks good to me.

Core development automation moved this from Inbox to In review Oct 7, 2021
@iche033 iche033 merged commit 19e1843 into gazebosim:ign-rendering3 Oct 7, 2021
Core development automation moved this from In review to Done Oct 7, 2021
@traversaro traversaro deleted the patch-3 branch October 7, 2021 09:40
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
WilliamLewww pushed a commit to WilliamLewww/ign-rendering that referenced this pull request Dec 7, 2021
…osim#454)

Signed-off-by: Silvio <silvio@traversaro.it>
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

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

3 participants