-
Notifications
You must be signed in to change notification settings - Fork 342
Add support for loading System plugin from static plugin registry #2950
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
Conversation
Signed-off-by: Shameek Ganguly <shameek@intrinsic.ai>
Signed-off-by: Shameek Ganguly <shameek@intrinsic.ai>
Signed-off-by: Shameek Ganguly <shameek@intrinsic.ai>
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.
looks good, just minor style suggestions
add_library(TestStaticModelSystem STATIC TestStaticModelSystem.cc) | ||
target_link_libraries(TestStaticModelSystem PRIVATE | ||
gz-plugin${GZ_PLUGIN_VER}::register | ||
gz-transport${GZ_TRANSPORT_VER}::gz-transport${GZ_TRANSPORT_VER} |
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.
I think gz-transport is not needed here
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.
I believe it is needed since the plugin implementation in the header file test/plugins/TestModelSystem.hh
exposes a gz transport service.
Signed-off-by: Shameek Ganguly <shameek@intrinsic.ai>
🎉 New feature
Summary
Gz Plugin added support for loading plugins from statically linked plugin libraries in gazebosim/gz-plugin@a49b9f4.
This PR builds on top of this feature to enable System plugins to be loaded from the static plugin registry.
To indicate that a plugin should be loaded from the static registry, users can specify the sdf
plugin@filename
attribute asstatic://<fully qualified plugin class name>
orstatic://<registered plugin alias>
. (Theplugin@name
attribute is unused in this case and can conventionally use the same plugin name or alias stem as theplugin@filename
attribute).I updated the
SystemLoader::InstantiateSystemPlugin
method to check if the plugin filename conforms to the above pattern and attempt to instantiate the plugin from the static registry in this case. Loading fails if the plugin either cannot be found in the static registry or it does not implement theSystem
interface.I also added an integration test in
test/integration/load_system_static_registry.cc
to verify this feature (runs both in cmake and bazel build). This test uses theTestModelSystem
test System class that I registered with the static plugin registry intest/plugins/TestStaticModelSystem.cc
. Note that theTestStaticModelSystem
static library needs to be linked with special flags into the test binary target in cmake to ensure that the plugin class symbols are not stripped out by the linker (similar to this test target in gz plugin).Test it
bazel:
cmake:
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
andGenerated-by
messages.