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

Reconsider the use of SWIG #4486

Closed
liangfok opened this issue Dec 13, 2016 · 14 comments · Fixed by #4949
Closed

Reconsider the use of SWIG #4486

liangfok opened this issue Dec 13, 2016 · 14 comments · Fixed by #4949

Comments

@liangfok
Copy link
Contributor

Problem Definition

Numerous oddities involving SWIG have cropped up:

Action Item

We should reconsider whether to continue using SWIG or switch to something else that is perhaps more target language specific. For example, for Python bindings, @jwnimmer-tri recommended Boost::Python and @jamiesnape recommended pybind11.

@jamiesnape
Copy link
Contributor

If we use Boost for anything else, we should use Boost::Python without hesitation. The two options above are very, very similar otherwise, so I have no real preference.

We have the long-standing Boost and MATLAB issues to consider, of course.

@rdeits
Copy link
Contributor

rdeits commented Dec 13, 2016

The additional issue is whether you want the Matlab bindings to be identical to the Python bindings and how much work you want to do to make that happen.

@jamiesnape
Copy link
Contributor

jamiesnape commented Dec 16, 2016

@mwoehlke-kitware is going to investigate this and post a proposal here for @david-german-tri, @rdeits, and others to review.

Initial thoughts are that MATLAB bindings remaining SWIG are fine, and whether we use Boost::Python depends on whether MATLAB needs to be simultaneously enabled with the Python bindings, and to a lesser extent whether we bring in Boost for other reasons, e.g., OMPL.

@david-german-tri
Copy link
Contributor

david-german-tri commented Jan 5, 2017

Bumping to high priority because it may be blocking #2482.

From f2f discussion: We believe that Boost::Python only needs to be linked into the Python bindings at the leaf of the build, and thus won't wind up linked into mex files. If that turns out to be incorrect, we can require that users build MATLAB or Python bindings but not both simultaneously. Thus, this is unblocked, and should move forward with Boost::Python.

@rdeits
Copy link
Contributor

rdeits commented Jan 5, 2017

@david-german-tri can you explain how it's blocking #2482? It's not obvious to me, since SWIG is unrelated to Pods.

@david-german-tri
Copy link
Contributor

@jamiesnape's belief is that the remaining few lines of https://github.com/RobotLocomotion/drake/blob/master/cmake/pods.cmake, which fiddle with RPATH, are necessary only to support the current SWIG approach. (I haven't actually tested that.)

@rdeits
Copy link
Contributor

rdeits commented Jan 5, 2017

I see. But surely SWIG can be used just fine without pods, since that's widely done in practice.

@jamiesnape
Copy link
Contributor

There are other issues. That is just the most pressing.

@jwnimmer-tri
Copy link
Collaborator

@mwoehlke-kitware -- #4771 has a question about the roadmap here. I believe there is additional information and decisions made but not yet scribed to the issue. Could you post a write-up?

@jamiesnape
Copy link
Contributor

We are going to use Boost::Python and the port will begin as soon as Director tests are up and running on CI, probably next week.

@mwoehlke-kitware
Copy link
Contributor

TBH, I wish Shiboken wasn't so in flux at the moment; it's the only binding generator I can find that doesn't require duplicating the entire API you want to expose.

On further investigation, between Boost::Python and pybind11, I'd actually rather use pybind11. The API is more modern and much less macro-heavy, the documentation seems easier to understand, how to integrate it into the build is much more obvious and better documented (Boost::Python basically says "use bjam or it will break"), and it just generally feels less brittle and better designed.

@jamiesnape
Copy link
Contributor

jamiesnape commented Jan 16, 2017

We have to introduce Boost for #3413, so Boost::Python would be preferable than adding the additional dependency unless we really have to.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Jan 16, 2017

TRI has Bazel build support for Boost::Python (or Boost::lots-of-stuff in general) that we can probably furnish to be open-sourced, if that's helpful. The code has a WORKSPACE entry for the Boost targz source release, then a small BUILD file that compiles a cc_library for Boost::Python, and then you just write your own cc file with the BOOST_PYTHON_MODULE call for the module you're going to export, as you would normally with any Boost::Python use no matter the build system.

@EricCousineau-TRI
Copy link
Contributor

Linking to #1267.

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

Successfully merging a pull request may close this issue.

7 participants