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

Proposal: Add inline namespaces to DART and maintain ABI stability between minor releases #1026

Open
mxgrey opened this issue Mar 8, 2018 · 4 comments
Assignees
Labels
tag: feature request Indicates new feature requests
Milestone

Comments

@mxgrey
Copy link
Member

mxgrey commented Mar 8, 2018

This is semi-related to #917

Future versions of Gazebo intend to use a plugin-based architecture in order to make the Gazebo simulations more modular and customizable.

One danger that is often faced is that multiple plugin libraries might depend on different versions of the same dependencies. Trying to use these multiple plugins simultaneously in one application can easily result in ABI collisions, due to DART's current approach to ABI management.

An easy and effective way to avoid symbol collision is to use inline namespaces which contain the major version of the library. However, this is only effective for avoiding symbol collisions between different major versions.

If the DART ABI gets changed between minor versions (e.g. adding a new member variable to a class), then simultaneously using two plugins that were built against two different minor versions will cause the application to crash.

The most popular and effective strategy for maintaining ABI stability is the PIMPL pattern, but DART uses a considerable amount of template metaprogramming, to the extent that I'm skeptical whether we could reasonably apply the PIMPL pattern to DART in any normal way.

One mitigation strategy is to defer any ABI-breaking changes until a major version increment. I think our ABI-breaking changes are rare enough that this would be an acceptable solution (at least to me).

Alternatively, I might be able to come up with a way to use the Composite class to give us a PIMPL-like way to delay ABI breakages. I'm not entirely sure how plausible this is, though.

I think this is particularly important because of DART's use as a toolkit. It's very likely that different plugins will want to use DART independently of each other for various reasons (e.g. a planning plugin might want to use the forward/inverse kinematics, while a controller plugin might want to use the inverse dynamics, and a physics engine plugin uses the forward dynamics). These plugins might be built and distributed against different versions of DART, especially in the case that a developer needs to make a plugin that uses a new feature of DART that wasn't available in the version that the other plugins were built and distributed for.

The easiest mitigation strategy would be to include the minor version number in the inline namespace, and to install headers to directories that are specific to major+minor version. I think this would be too blunt of an instrument, and it would prevent plugins that are built using different minor versions of DART from communicating using DART symbols.

@mxgrey mxgrey added tag: feature request Indicates new feature requests type: maintenance labels Mar 8, 2018
@mxgrey mxgrey added this to the DART 7.0.0 milestone Mar 8, 2018
@mxgrey mxgrey self-assigned this Mar 8, 2018
@scpeters
Copy link
Collaborator

I know this is still just a proposal, but I wanted to note that I think dart's ABI was broken by 68aef6b#diff-b521b0929fd5887e9bd58eebad913cef (conversion of createShapeNode functions from library to template).

@mxgrey
Copy link
Member Author

mxgrey commented May 21, 2019

Related to this concern, an ABI breakage caused some headache here: https://bitbucket.org/ignitionrobotics/ign-physics-release/pull-requests/5/

One thing that might help this issue is allowing minor versions of dartsim to be installed side-by-side.

@jslee02
Copy link
Member

jslee02 commented May 21, 2019

One mitigation strategy is to defer any ABI-breaking changes until a major version increment. I think our ABI-breaking changes are rare enough that this would be an acceptable solution (at least to me).

This consensus is realistic to me. We might need a tool to check the ABI compatibility for every change within the same minor version.

One thing that might help this issue is allowing minor versions of dartsim to be installed side-by-side.

I'm a bit hesitated to this solution because it could be too verbose.

@mxgrey
Copy link
Member Author

mxgrey commented May 21, 2019

I'm a bit hesitated to this solution because it could be too verbose.

Yeah, that's understandable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: feature request Indicates new feature requests
Projects
None yet
Development

No branches or pull requests

3 participants