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

pybind: Should restore fork's latest commit in workspace pending mac fix #14047

Closed
2 tasks done
EricCousineau-TRI opened this issue Sep 11, 2020 · 8 comments · Fixed by #14048
Closed
2 tasks done

pybind: Should restore fork's latest commit in workspace pending mac fix #14047

EricCousineau-TRI opened this issue Sep 11, 2020 · 8 comments · Fixed by #14048
Assignees
Labels
component: pydrake Python API and its supporting Starlark macros priority: high unused team: manipulation

Comments

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Sep 11, 2020

See: #14046 (comment)

This was reproducible on our macsim machine.

@jamiesnape I will try this briefly. If I can't make this work today, can I ask you to put this at the top of your pydrake queue for the next two weeks?


UPDATE: Plan of attack:

@jamiesnape
Copy link
Contributor

So this issue is restoring the fork or fixing the underlying Mac issue?

@EricCousineau-TRI
Copy link
Contributor Author

Fixing the underlying Mac issue for building in CMake that causes the RTTI mismatch, most likely due to some visibility changes that came from the fork updates.
Not sure if it requires a change to our fork, or just a change in downstream.

(RobotLocomotion/pybind11 itself is unchanged at present, and I don't think we should revert anything there)

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Sep 11, 2020

FTR On macsim, I am using this drake-external-examples setup:
https://github.com/RobotLocomotion/drake-external-examples/blob/8a53b4c8df534fcc02f557d1579636dd16795151/drake_cmake_installed/src/simple_bindings/CMakeLists.txt

I confirmed that drake @ 690a7ea works (post-revert), and drake @ 0cc01f0 (pre-revert) reprodues the error:

cd .../drake
bazel run //:install -- /tmp/drake

cd .../bazel-external-examples/drake_cmake_installed
mkdir build && cd build
cmake -DCMAKE_PREFIX_PATH=/tmp/drake ..
cd src/simple_bindings/
make && ctest -V -R

@jamiesnape
Copy link
Contributor

Ok, can you update the title to match?

@jamiesnape jamiesnape self-assigned this Sep 11, 2020
@EricCousineau-TRI EricCousineau-TRI changed the title pybind: Should restore fork update pending mac fix pybind: Should restore fork's latest commit in workspace pending mac fix Sep 11, 2020
@EricCousineau-TRI
Copy link
Contributor Author

Done.

My money's on some change from upstream's CMake in pybind11Tools or pybind11Config:
RobotLocomotion/pybind11@58a368ea8#diff-8cb32abb960df57289d52b400092e31d

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Sep 11, 2020

Yuppers. It looks like pybind/pybind11@97aa54fefa is the culprit.
I had stripped it out (accidentally?) before, and restored it when I carte-blanch took in upstream's changes.

Here's the patch-fix for drake-external-examples:

diff --git a/drake_cmake_installed/src/simple_bindings/CMakeLists.txt b/drake_cmake_installed/src/simple_bindings/CMakeLists.txt
index 1fde04b..d8530c3 100644
--- a/drake_cmake_installed/src/simple_bindings/CMakeLists.txt
+++ b/drake_cmake_installed/src/simple_bindings/CMakeLists.txt
@@ -33,7 +33,14 @@
 find_package(pybind11 CONFIG REQUIRED)
 
 pybind11_add_module(simple_bindings MODULE simple_bindings.cc)
-target_link_libraries(simple_bindings PUBLIC drake::drake pybind11::module)
+target_link_libraries(simple_bindings PUBLIC drake::drake)
+# N.B. `pybind11_add_module` normally sets the default visibility to "hidden"
+# to avoid warnings. However, we generally take care in Drake to avoid mixing
+# hidden and public symbols, so we should we confidently re-enable this
+# setting.
+# For more information, see:
+# https://github.com/RobotLocomotion/drake/issues/14047
+set_target_properties(simple_bindings PROPERTIES CXX_VISIBILITY_PRESET "default")
 
 add_test(NAME simple_bindings_test COMMAND
     "${PYTHON_EXECUTABLE}" "${CMAKE_CURRENT_SOURCE_DIR}/simple_bindings_test.py")

My desire is to leave upstream's CMake logic as untouched as possible. I think just propogating this specific CXX_VISIBILITY_PRESET post-fix should be fine.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Sep 11, 2020

Confirmed that this patch works on both drake @ 690a7ea and drake @ 0cc01f0. Will update overview with plan of attack.

EDIT: Done.

@EricCousineau-TRI
Copy link
Contributor Author

Filed upstream issue to pybind11: pybind/pybind11#2479

@jamiesnape jamiesnape removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pydrake Python API and its supporting Starlark macros priority: high unused team: manipulation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants