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

Add OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION cmake option #4012

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adamkewley
Copy link
Contributor

@adamkewley adamkewley commented Feb 6, 2025

This is a patch I've been applying downstream in opensim-creator that I figured might be useful for opensim-core.

This adds an OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION cmake option that can be toggled ON to build OpenSim without the static-init-time instantiator classes.

The motivation for doing this is:

  • It lets downstream applications/libraries decide if/when to register OpenSim types. There are several reasons why downstream might want this:
    • The application wants to set up logging, log sinks, etc. before registration, so that all logged registration errors appear in a runtime-defined location (e.g. a UI).
    • The application wants to lazily-load parts of OpenSim
    • A bug has appeared in the application and it would be useful to debug step registration, skip registering something, etc. without having to recompile OpenSim.
  • It lets downstream application/libraries decide load-order. The reason you'd want to do this is:
    • OpenSim is being loaded as one library/application, so the instantiators cannot lean on the fact that the dynamic loader is loading libraries in use-order. Right now, the only reason the static initializers work is because osimActuators.dll depends on osimCommon.dll, so the dynamic loader just happens to load one before the other - this may fail if they are merged into one binary. It may also fail if (e.g.) the log is statically initialized after a static registration function that writes to the log (a segfault I encountered with OpenSim Creator).

Brief summary of changes

  • Added OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION to CMakeLists.txt
  • Used the resulting compile define to control whether the static initializer is emitted or not

Testing I've completed

  • Similar pattern works on OpenSim Creator, which has its own application log (shown to the user via a UI) and statically compiles OpenSim into its executable.

Looking for feedback on...

CHANGELOG.md (choose one)

  • updated TODO

This change is Reviewable

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

LGTM, but requesting a review from @aymanhab since (I think) he wrote the instantiators originally and may have some comments.

@nickbianco nickbianco requested a review from aymanhab February 11, 2025 17:28
Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

Thanks @adamkewley that looks reasonable, especially that the default behavior is unchanged.
:lgtm:

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @adamkewley)


CMakeLists.txt line 193 at r1 (raw file):

on using by calling `RegisterTypes_osimLIBRARY` (e.g. `RegisterTypes_osimActuators`),
or by manually registering each type (e.g. `Object::registerType(PhysicalOffsetFrame());`)." OFF)
mark_as_advanced(OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION)

Will this show in cmake popup? Great explanation but maybe too verbose for a popup? Maybe something like turn it on only if you'll load the libraries explicitly in your code? but your call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants