cmake: Add example of custom bindings using drake + pydrake

[EricCousineau-TRI]

Requires:

• pybind11: Install headers and CMake config drake#8264
• pybind: Rebase + CMake updates for RobotLocomotion/drake-shambhala#98 drake#8860

Supersedes #95

Relates:
pybind/pybind11#1373
pybind/pybind11#1367

---

This change is 

[EricCousineau-TRI]

+@jamiesnape for feature review, please

---

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[RussTedrake]

Nice! This is going to be very useful for me. thanks!

---

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.

---

Comments from Reviewable

[jamiesnape]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

---

drake_cmake_external/CMakeLists.txt, line 36 at r1 (raw file):

# `ExternalProject_Add` here, and add `drake_shambhala` afterwards (also as a
# external project) which depends on these external projects, so that it can
# effectively build the external projects during the configuration stage.

Remove. Does not add anything and quite possibly confuses things.

---

drake_cmake_installed/CMakeLists.txt, line 46 at r1 (raw file):

# This must be specified before `drake`, so that `pybind11` does not find
# another version.
find_package(PythonInterp 2.7 EXACT MODULE REQUIRED)

This is exactly why you should not be throwing unnecessary dependencies into drake-config.cmake.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 8 at r1 (raw file):

# which is empty, relying on libraries to link `pybind11_LIBRARIES` to get the
# proper include directories; `pybind11_add_module` does not use
# `pybind11_LIBRARIES`.

Remove. You are misunderstanding how target_link_libraries works. The name is a legacy and you should never need to use a pybind11_INCLUDE_DIRS variable.

---

Comments from Reviewable

[jamiesnape]

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 2 at r1 (raw file):

find_package(pybind11 CONFIG REQUIRED)
include(pybind11Tools)

Remove. This is already included by the find_package call.

---

Comments from Reviewable

[jamiesnape]

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

---

drake_cmake_installed/src/simple_bindings/test_example_module.py, line 1 at r1 (raw file):

from example_module import SimpleAdder

Name the file example_module_test.py and add a short docstring to the top of the file. Also pylint is really not happy.

---

Comments from Reviewable

[jamiesnape]

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 1 at r1 (raw file):

find_package(pybind11 CONFIG REQUIRED)

Add the copyright and license header.

---

drake_cmake_installed/src/simple_bindings/example_module.cc, line 1 at r1 (raw file):

#include <pybind11/pybind11.h>

Add the copyright and license header.

---

drake_cmake_installed/src/simple_bindings/test_example_module.py, line 1 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Name the file example_module_test.py and add a short docstring to the top of the file. Also pylint is really not happy.

Also add the copyright and license header.

---

Comments from Reviewable

[jamiesnape]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks broke.

---

drake_cmake_installed/src/simple_bindings/example_module.cc, line 19 at r1 (raw file):

class SimpleAdder : public LeafSystem<T> {
 public:
  SimpleAdder(T add)

explicit

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 2 of 5 files reviewed at latest revision, 8 unresolved discussions.

---

drake_cmake_external/CMakeLists.txt, line 36 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Remove. Does not add anything and quite possibly confuses things.

I personally found the subproject structure confusing, and had to root around to figure out why we had this way.

Is there a link that I can point to, in this case? Or a better wording?

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 8 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Remove. You are misunderstanding how target_link_libraries works. The name is a legacy and you should never need to use a pybind11_INCLUDE_DIRS variable.

Done. Reworded, as it was unexpected to me that pybind11_add_module does not correctly include pybind11, which is because our pybind11-config diverges from the original.

---

drake_cmake_installed/src/simple_bindings/example_module.cc, line 1 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Add the copyright and license header.

Done.

---

drake_cmake_installed/src/simple_bindings/example_module.cc, line 19 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

explicit

Done.

---

drake_cmake_installed/CMakeLists.txt, line 46 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

This is exactly why you should not be throwing unnecessary dependencies into drake-config.cmake.

Done.

---

drake_cmake_installed/src/simple_bindings/test_example_module.py, line 1 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Also add the copyright and license header.

Done.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 2 of 7 files reviewed at latest revision, 8 unresolved discussions.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 1 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Add the copyright and license header.

Done.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 2 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Remove. This is already included by the find_package call.

Done.

---

Comments from Reviewable

[jamiesnape]

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

drake_cmake_external/CMakeLists.txt, line 36 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I personally found the subproject structure confusing, and had to root around to figure out why we had this way.

Is there a link that I can point to, in this case? Or a better wording?

I don't think so. The CMake documentation is sufficient and easy to find (if you think it is neither of these, please open an issue upstream and we will try to fix) and we do not annotate every CMake call with a link to its documentation.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 8 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done. Reworded, as it was unexpected to me that pybind11_add_module does not correctly include pybind11, which is because our pybind11-config diverges from the original.

Even if cps2cmake did define pybind11_INCLUDE_DIRS, you still would not use it. This is just how you use CMake these days.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 4 at r2 (raw file):

# vi: set ft=cmake :

# Copyright (c) 2017, Massachusetts Institute of Technology.

Probably Copyright (c) 2018, Toyota Research Institute. for all the new files?

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 41 at r2 (raw file):

target_link_libraries(example_module PUBLIC drake::drake pybind11::module)
# Revert visibilty preset (workaround for RobotLocomotion/drake#8540).
set_target_properties(example_module PROPERTIES CXX_VISIBILITY_PRESET "default")

Revert visibilty preset set by pybind11_add_module()? Also quotes are not needed around default.

---

Comments from Reviewable

[EricCousineau-TRI]

drake_cmake_external/CMakeLists.txt, line 36 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

I don't think so. The CMake documentation is sufficient and easy to find (if you think it is neither of these, please open an issue upstream and we will try to fix) and we do not annotate every CMake call with a link to its documentation.

Is there a better way to search for this as a non-expert? What is the best way to find an authoritative source on this exact structure?

My quick searches provide next to nil on finding a consistent mention of this; from what I see in the CMake docs for ExternalProject_Add, it shows the usage of the method, but does not appear to provide explicit / easily discoverable advice on how to structure your project which shows this structure.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 4 of 7 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 8 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Even if cps2cmake did define pybind11_INCLUDE_DIRS, you still would not use it. This is just how you use CMake these days.

Trying to follow the pybind11 docs with our setup from cps2cmake leads to a whole slew of ugly exceptions:
EricCousineau-TRI@8d032c6

I can use the interface library setup, but I don't like it as it's more verbose and deviates even further from pybind11s usage.
(Will create a Drake issue at some point.)

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 4 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Probably Copyright (c) 2018, Toyota Research Institute. for all the new files?

Done.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 41 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Revert visibilty preset set by pybind11_add_module()? Also quotes are not needed around default.

Done.

---

Comments from Reviewable

[jamiesnape]

Review status: 4 of 7 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 8 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Trying to follow the pybind11 docs with our setup from cps2cmake leads to a whole slew of ugly exceptions:
EricCousineau-TRI@8d032c6

I can use the interface library setup, but I don't like it as it's more verbose and deviates even further from pybind11s usage.
(Will create a Drake issue at some point.)

The pybind11 docs need updating then. They provide exactly the same INTERFACE targets.

---

Comments from Reviewable

[jamiesnape]

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 8 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

The pybind11 docs need updating then. They provide exactly the same INTERFACE targets.

Their docs are fine: https://github.com/pybind/pybind11/blob/master/docs/compiling.rst#advanced-interface-library-target

---

Comments from Reviewable

[jamiesnape]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

---

drake_cmake_installed/src/simple_bindings/CMakeLists.txt, line 8 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Their docs are fine: https://github.com/pybind/pybind11/blob/master/docs/compiling.rst#advanced-interface-library-target

What is different?

---

Comments from Reviewable

[EricCousineau-TRI]

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Getting the following error(s) when building and running off of current drake@master:

At one point:

4: Failure at /home/eacousineau/proj/tri/repo/branches/drake/master/drake/build_install/include/drake/systems/framework/system_base.h:560 in CreateInputPort(): condition 'port->get_index() 
== this->get_num_input_ports()' failed.

After a fresh rebuild:

4:     adder = builder.AddSystem(SimpleAdder(100.))
4: RuntimeError: Converting to an int. Type "drake::TypeSafeIndex<drake::systems::InputPortTag>", has an invalid value; it must lie in the range [0, 2³¹ - 1].

Seems to be some linking shenanigans?

Confirmed that this does not fail when the same code is run in drake Bazel land:
EricCousineau-TRI/drake@8f242ca

---

Comments from Reviewable

[EricCousineau-TRI]

May wait until #105 progresses, and then use the same code in this PR.

Per @fbudin69500's suggestion, may be due to not linking to the correct library?

[EricCousineau-TRI]

@RussTedrake Will you need this to be updated for future course teachings?

[RussTedrake]

It would be helpful for sure.

…

On Aug 15, 2018, at 7:29 PM, Eric Cousineau ***@***.***> wrote: @RussTedrake Will you need this to be updated for your course this fall? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[EricCousineau-TRI]

Pinging @jamiesnape

[jamiesnape]

+@m-chaturvedi is going to look into the outstanding issues.

[EricCousineau-TRI]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee m-chaturvedi, LGTM missing from assignee jamiesnape, platform LGTM missing

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Confirmed that this does not fail when the same code is run in drake Bazel land:
EricCousineau-TRI/drake@8f242ca

Using @m-chaturvedi's build script (uses packaged Drake), and my mod to it (using bazel run //:install from Drake), the segfaults seem to have disappeared. Marking this as resolved.

---

[jamiesnape]

Reviewed 3 of 3 files at r5.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee m-chaturvedi, LGTM missing from assignee jamiesnape, platform LGTM missing

[EricCousineau-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee m-chaturvedi, LGTM missing from assignee jamiesnape, platform LGTM missing

---

drake_cmake_external/CMakeLists.txt, line 36 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Is there a better way to search for this as a non-expert? What is the best way to find an authoritative source on this exact structure?

My quick searches provide next to nil on finding a consistent mention of this; from what I see in the CMake docs for ExternalProject_Add, it shows the usage of the method, but does not appear to provide explicit / easily discoverable advice on how to structure your project which shows this structure.

Just to follow up, is there a link that I can point to here? Or should I just leave it as-is?

[EricCousineau-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee m-chaturvedi, LGTM missing from assignee jamiesnape, platform LGTM missing

---

drake_cmake_external/CMakeLists.txt, line 36 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Just to follow up, is there a link that I can point to here? Or should I just leave it as-is?

Per vcon, I will make a GitHub issue on CMake; link to blog and/or issue:
https://gitlab.kitware.com/cmake/cmake/issues

[EricCousineau-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee m-chaturvedi, LGTM missing from assignee jamiesnape, platform LGTM missing

---

drake_cmake_external/CMakeLists.txt, line 36 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Per vcon, I will make a GitHub issue on CMake; link to blog and/or issue:
https://gitlab.kitware.com/cmake/cmake/issues

Sorry for the delay, submitted:
https://gitlab.kitware.com/cmake/cmake/issues/18336
Will link to this issue.

[EricCousineau-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee m-chaturvedi, LGTM missing from assignee jamiesnape, platform LGTM missing

---

drake_cmake_external/CMakeLists.txt, line 36 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Sorry for the delay, submitted:
https://gitlab.kitware.com/cmake/cmake/issues/18336
Will link to this issue.

Done.

[jamiesnape]

Reviewed 2 of 2 files at r6.
Reviewable status: all discussions resolved, LGTM missing from assignee m-chaturvedi, LGTM missing from assignee jamiesnape, platform LGTM missing

[jamiesnape]

Reviewable status: all discussions resolved, LGTM missing from assignee m-chaturvedi, platform LGTM from [jamiesnape]

[m-chaturvedi]

[m-chaturvedi]

Reviewable status: complete! all discussions resolved, platform LGTM from [jamiesnape]