gazebosim / gazebo-classic

Gazebo classic. For the latest version, see https://github.com/gazebosim/gz-sim
http://classic.gazebosim.org/
Other
1.21k stars 484 forks source link

Fix Instance() method of Singleton classes #3269

Closed traversaro closed 1 year ago

traversaro commented 2 years ago

In particular, make sure that all the Instance() methods of the same class always use the same instantiation.

🦟 Bug fix

Fixes https://github.com/conda-forge/gazebo-feedstock/issues/148 .

Summary

This should fix all the cases in which multiple instances of a Singleton classes were observed, as for example in conda-forge/macOS build of Classic Gazebo (see https://github.com/conda-forge/gazebo-feedstock/issues/148#issuecomment-1249496093). The fix was inspired from https://github.com/gazebosim/gz-sim/issues/1725 and from the related PRs:

Thanks a lot to @azeey for suggesting the correct fix in https://github.com/conda-forge/gazebo-feedstock/pull/152#issuecomment-1275198706 .

Checklist

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.

traversaro commented 1 year ago

What is your opinion regarding merging this @scpeters @azeey ? We just hit a regression involving this patch in conda-forge (see https://github.com/conda-forge/gazebo-feedstock/issues/161), to avoid this in the future it would be great if we could get this eventually in an official release.

traversaro commented 1 year ago

I'm rerunning Ubuntu CI and then it will be ready for merge, though I may need to do some testing with ROS before releasing this

Sure, we can also just wait for the ROS tests before even merging this PR, I am not particulary in a hurry on this one. I am just afraid that at some point the problem that affected conda-forge macOS builds also started to affect other builds (for example homebrew builds as soon as a new version of Apple Clang is released).