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

Remove static for maps from Factory.hh #635

Merged
merged 8 commits into from
Mar 19, 2021
Merged

Remove static for maps from Factory.hh #635

merged 8 commits into from
Mar 19, 2021

Conversation

shameekganguly
Copy link
Contributor

🦟 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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: Shameek Ganguly <[email protected]>
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Feb 18, 2021
@chapulina
Copy link
Contributor

Thank you for the pull request. This is causing CI to fail, does it work locally for you?

/var/lib/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/ign-gazebo/include/ignition/gazebo/components/Factory.hh:313:59: error: ‘namesById’ declared as an ‘inline’ field
     public: inline std::map<ComponentTypeId, std::string> namesById;
                                                           ^~~~~~~~~
/var/lib/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/ign-gazebo/include/ignition/gazebo/components/Factory.hh:318:9: error: ‘runtimeNamesById’ declared as an ‘inline’ field
         runtimeNamesById;
         ^~~~~~~~~~~~~~~~

@chapulina chapulina requested a review from mjcarroll February 22, 2021 19:40
@mjcarroll
Copy link
Contributor

Ping @shameekganguly , I think you also meant to remove inline here?

@shameekganguly
Copy link
Contributor Author

shameekganguly commented Feb 23, 2021

Yep, sorry about that! Thanks for merging the fix in!

@shameekganguly
Copy link
Contributor Author

Should be able to pass all tests with the new fix.

Copy link
Contributor

@chapulina chapulina left a 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/

@mjcarroll mjcarroll changed the base branch from ign-gazebo4 to main March 8, 2021 14:48
@mjcarroll
Copy link
Contributor

This breaks ABI and can't be merged into a stable release. Can it be retargeted to main (Edifice)?

Discussed offline, main is fine, retargeted.

@chapulina chapulina added 🏢 edifice Ignition Edifice and removed 🔮 dome Ignition Dome labels Mar 9, 2021
@chapulina
Copy link
Contributor

@osrf-jenkins run tests again

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021
Copy link
Contributor

@chapulina chapulina left a 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!

@chapulina chapulina merged commit 636bad5 into gazebosim:main Mar 19, 2021
@shameekganguly shameekganguly deleted the ign-gazebo4 branch June 6, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants