search

RobotLocomotion / ros-drake-vendor

Maintainer scripts that package Drake in the ROS build farm
Other
1 stars 1 forks source link

Posible cleanup of the pybind files in the "make install" installation #5

Closed j-rivero closed 10 months ago

j-rivero commented 11 months ago

While installing Drake using make install I saw that it is installing some pybind11 headers and CMake artifacts:

-> ./lib/cmake/pybind11
FindPythonLibsNew.cmake  pybind11-config.cmake  pybind11-config-version.cmake  pybind11Tools.cmake
-> ./share/doc/pybind11
LICENSE
-> ./include/pybind11/pybind11
attr.h         cast.h    common.h   detail  eigen.h  eval.h    gil.h       numpy.h      options.h   pytypes.h  stl_bind.h  type_caster_pyobject_ptr.h
buffer_info.h  chrono.h  complex.h  eigen   embed.h  functional.h  iostream.h  operators.h  pybind11.h  stl    stl.h
-> ./include/pybind11/pybind11/pybind11.h
./include/pybind11/pybind11/pybind11.h

I was not able to find any other header or installed file using these headers or CMake artifacts. @jwnimmer-tri Can we remove them or there is an use case that needs them to be installed?

jwnimmer-tri commented 11 months ago

If the end user writes their own C++ code that uses Drake's C++ classes as part of its API, and wraps that in their own Python bindings that likewise use pydrake as part of its API, then they must use Drake's fork of pybind11 when compiling their bindings. Otherwise, they are not only violating ODR but probably will have broken code as a result.

That's why the headers (and CMake snippets) are installed. Ideally, the ROS package of Drake would provide the same capability.

Here's an example of that use case: https://github.com/RobotLocomotion/drake-ros/blob/b9b20c7ae2631e25478e6a6eaa05af8c3b4da07b/drake_ros/CMakeLists.txt#L16-L17

j-rivero commented 10 months ago

That's why the headers (and CMake snippets) are installed. Ideally, the ROS package of Drake would provide the same capability.

Here's an example of that use case: https://github.com/RobotLocomotion/drake-ros/blob/b9b20c7ae2631e25478e6a6eaa05af8c3b4da07b/drake_ros/CMakeLists.txt#L16-L17

Thanks for the information and the code. Since the Drake package is going to install files that are part of its dependencies (instead of relying on the other packages) I was wondering if we can "encapsulate" the files somehow to avoid using a path that could point people to the dependency being fully installed and ready to be consumed.

In this case, the path $PREFIX/include/pybind11/pybind11/pybind11.h could confuse people when a different/neutral pybind11 installation. It includes the pybind11/pybind11 twice which should bring protection against code using #include <pybind11/foo.h>.

It but would be great if we could move the files under the a drake specific path, something like: $PREFIX/include/drake/pybind11/ or $PREFIX/include/drake_pybind11/`.

@jwnimmer-tri would that work for the use case you detailed above? It will probably require code changes but for the final user we can probably encapsulate the path using CMake.

jwnimmer-tri commented 10 months ago

I'm having a bit of a difficult time wrapping my head around your question. Here's what I think would help:

In this build of Drake (the ROS package build), please explain the full paths where all of the files will end up.

Here's the full list directories that currently end up being installed:

$prefix/bin/...
$prefix/include/drake/...
$prefix/include/drake_lcmtypes/drake/...
$prefix/include/lcm/lcm/...
$prefix/include/pybind11/pybind11/...
$prefix/lib/cmake/drake/...
$prefix/lib/cmake/lcm/...
$prefix/lib/cmake/pybind11/...
$prefix/lib/...
$prefix/lib/python3.10/site-packages/drake/...
$prefix/lib/python3.10/site-packages/lcm/...
$prefix/lib/python3.10/site-packages/pydrake/...
$prefix/share/doc/...
$prefix/share/drake/...
$prefix/share/java/...

In this new package, will we simply have $prefix set to some value (if so, what value?) -- or instead will we be scattering those paths around so that there isn't a uniform $prefix but instead some customization of where things end up (if so, what is the mapping from those paths to the new paths)?

I'm guessing that your plan is maybe just prefix=/opt/ros/humble uniformly, but I'd like to be sure I have the right starting point before I reply.

j-rivero commented 10 months ago

In this build of Drake (the ROS package build), please explain the full paths where all of the files will end up.

All ROS packages created in the ROS Buildfarm use the following prefix /opt/ros/${ROS_DISTRO} . Given that our target is ROS Rolling, all the files should end up being installed under /opt/ros/rolling. That prefix acts as a chroot-like and it is populated with files under the usual FHS rules where you can find bin, lib, include, etc.:

/opt/ros/rolling ❯ ls
bin      local_setup.bash      local_setup.zsh  setup.zsh  tools
include  local_setup.sh        setup.bash       share
lib      _local_setup_util.py  setup.sh         src

In this new package, will we simply have $prefix set to some value (if so, what value?)

/opt/ros/rolling should act as the prefix. The Debian generator in the ROS Buildfarm will setup CMAKE_PREFIX_PATH with that value.

Clarifying my original post (sorry for the confusion) the include paths that I expect to be used by the Drake pybind installation would be currently: /opt/ros/rolling/include/pybind11/pybind11/ . I was wondering if we could modify it to be /opt/ros/rolling/include/drake/pybind11/ or /opt/ros/rolling/include/drake_pybind11/ so it is clear under the ROS filesystem that the pybind files belong to the Drake ROS package.

Hope it helps to understand my original question. Let me know if you need more details.

jwnimmer-tri commented 10 months ago

Given that our target is ROS Rolling ...

Just to double check -- we're aiming for Rolling for now because it's a useful and required stepping stone to reach Humble, yes? The ultimate goal of this SOW is to get Drake into both Humble and Rolling, so we will be pushing it down into Humble once we're happy with what it looks like in Rolling. Is that accurate?

(In this case, prefix would be either /opt/ros/rolling and/or /opt/ros/humble depending on which distro is being built.)

Hope it helps to understand my original question.

Yes, thanks. I'll need to check on a few things before I reply.

j-rivero commented 10 months ago

Just to double check -- we're aiming for Rolling for now because it's a useful and required stepping stone to reach Humble, yes? The ultimate goal of this SOW is to get Drake into both Humble and Rolling, so we will be pushing it down into Humble once we're happy with what it looks like in Rolling. Is that accurate?

Rolling is a good candidate to start with here because is consider as a "development" version and can fit well into our initial proposes of provide a testing package for Drake. It is not a requirement for Humble but yes, my plan is try to make the release in Rolling and release in Humble after that. Now both uses Ubuntu Jammy so the difference should be minimal.

(In this case, prefix would be either /opt/ros/rolling and/or /opt/ros/humble depending on which distro is being built.)

That is correct.

Hope it helps to understand my original question.

Yes, thanks. I'll need to check on a few things before I reply.

No problem, take the time you need, not a blocker by now.

j-rivero commented 10 months ago

Clarifying my original post (sorry for the confusion) the include paths that I expect to be used by the Drake pybind installation would be currently: /opt/ros/rolling/include/pybind11/pybind11/ . I was wondering if we could modify it to be /opt/ros/rolling/include/drake/pybind11/ or /opt/ros/rolling/include/drake_pybind11/ so it is clear under the ROS filesystem that the pybind files belong to the Drake ROS package.

After building the and installing drake using the ros_buildfarm installation flags in #6 (more relevant here is -DCMAKE_INSTALL_PREFIX=/opt/ros/rolling I see that the full prefix path used for drake is /opt/ros/rolling/opt/drake and not what I thought, the plain /opt/ros/rolling. The /opt/ros/rolling/opt/ (an opt/ inside an opt/) is actually used by ROS for the vendor packages so I think it is the right place for this Drake installation.

If this is true and we can encapsulate Drake in /opt/ros/rolling/opt/drake, my questions here about changing the pybind11 paths can decline easily since the installation prefix already 'encapsulate' the whole inside a drake subdirectory and it is not being mixed with other parts of the ROS ecosystem.

I think that we can close this by now.

jwnimmer-tri commented 10 months ago

The /opt/ros/rolling/opt/ (an opt/ inside an opt/) is actually used by ROS for the vendor packages so I think it is the right place for this Drake installation. ... I think that we can close this by now.

That's fine by me.

Let me just double-check that the outcome I care about is met ...

When a user wants to compile & link against Drake in their own ROS project code, the spelling for that is both succinct and obvious:

In particular, they do not need to manually type in the "double opt" path you mention above -- the details about the exact path layout /opt/ros/rolling/opt/drake/include and /opt/ros/rolling/opt/drake/lib etc are all handled automatically by the build system infrastructure. Is that right?

j-rivero commented 10 months ago

When a user wants to compile & link against Drake in their own ROS project code, the spelling for that is both succinct and obvious:

* maybe they need to list Drake in their `package.xml`;
* maybe they need to e.g. `sudo apt install ros-rolling-drake` (using whatever package name is appropriate);
* maybe they need to update their `CMakeLists.txt` to call `find_package(drake)` or `target_link_library ... drake` or something along those lines.

All three points are correct. Adding a find_package(drake) that finds the ROS package contained in this repository should provide that CMake code the ability to use the variables and configurations provided by the Drake drake-config.cmake.

In particular, they do not need to manually type in the "double opt" path you mention above -- the details about the exact path layout /opt/ros/rolling/opt/drake/include and /opt/ros/rolling/opt/drake/lib etc are all handled automatically by the build system infrastructure. Is that right?

That is my expectation and the code changes I'm currently working on.

jwnimmer-tri commented 10 months ago

Sounds great, thanks.