-
Notifications
You must be signed in to change notification settings - Fork 287
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
Remove static for maps from Factory.hh #635
Conversation
Signed-off-by: Shameek Ganguly <[email protected]>
Thank you for the pull request. This is causing CI to fail, does it work locally for you?
|
Ping @shameekganguly , I think you also meant to remove |
Signed-off-by: Shameek Ganguly <[email protected]>
Yep, sorry about that! Thanks for merging the fix in! |
Should be able to pass all tests with the new fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks ABI and can't be merged into a stable release. Can it be retargeted to main
(Edifice)?
See the ABI test results: https://build.osrfoundation.org/job/ignition_gazebo-abichecker-any_to_any-ubuntu_auto-amd64/3271/API_5fABI_20report/
Discussed offline, |
@osrf-jenkins run tests again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better late than never, thanks for the PR!
🦟 Bug fix
Fixes an issue where dlopen can fail with segfault due to static map variables in Factory.hh.
Fix is to simply remove the static qualifiers. This does not have any significant usage impact and a minor perf impact as the Factory class is already a singleton.
Note to maintainers: Remember to use Squash-Merge