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

Resolves potential linking error in python bindings #9820

Merged

Conversation

SeanCurtis-TRI
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI commented Oct 26, 2018

Move the static world frame id into function static -- this eliminates ODR problems based on how python bindings are articulated and combined.

(Came up in PR 9796).


This change is Reviewable

Move the static world frame id into function static -- this eliminates
ODR problems based on how python bindings are articulated and combined.

(Came up in PR 9796).
Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@RussTedrake

/cc @jwnimmer-tri (With the requested never destroyed).

Reviewable status: all discussions resolved, platform LGTM missing

@jwnimmer-tri
Copy link
Collaborator

FYI The reason this code is better is because it removes the global variable's non-trivial constructor. (That is, now the initialization happens when first needed, instead of when the object code is loaded.) It does not make ODR compliance any better or worse -- but perhaps just less likely for ODR violations to cause problems in practice.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, did this error occur at interpreter shutdown time, for any process that imported stuff from
pydrake.examples.manipulation_station? (which violates ODR by how it's linked in, relevant to #9645 and #9646)

Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, platform LGTM missing

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error was a runtime error. Basically the class-static member ended up getting duplicated twice. So, there were in fact two world frame ids running around in Russ's example. All of the code assumes that there's only a single and it led to lots of inscrutable bugs. @jwnimmer-tri seems to have triaged it already here: #9796 (comment)

Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, platform LGTM missing

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR Given the simplicity of this PR, I'd accept a feature review from anyone who has weighed in and commented. Hint, hint. :)

Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, platform LGTM missing

@jwnimmer-tri jwnimmer-tri self-assigned this Oct 26, 2018
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: feature and platform. I'll leave it up to you if you'd like to have a second pair of eyes also stamp it.

Reviewed 2 of 2 files at r1.
Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, platform LGTM from [jwnimmer-tri]

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jwnimmer-tri , I'm going to call it good. -@RussTedrake

Reviewable status: all discussions resolved, LGTM missing from assignee russtedrake, platform LGTM from [jwnimmer-tri]

@SeanCurtis-TRI SeanCurtis-TRI merged commit f3b68ef into RobotLocomotion:master Oct 26, 2018
@SeanCurtis-TRI SeanCurtis-TRI deleted the PR_move_SG_world_frame branch October 26, 2018 16:21
@RussTedrake
Copy link
Contributor

You guys aren't very patient. I was testing.

joint_teleop was broken this morning on master.
now that this has landed, it is STILL broken.
if I use the fix in my branch (which @SeanCurtis-TRI and I worked on together) instead of this fix, it resolves the problem.

@RussTedrake
Copy link
Contributor

if it's useful, interesting... we could probably bisect and figure out when joint_teleop broke in the first place. and I'm remiss that I forgot to add the test_rule. Will do that on my next manipulation_station PR.

@jwnimmer-tri
Copy link
Collaborator

Russ and I discussed this f2f. His problem remains ODR linking with //examples/manipulation_station, not with this PR.

@SeanCurtis-TRI
Copy link
Contributor Author

Thanks @jwnimmer-tri.

@jwnimmer-tri
Copy link
Collaborator

The real problem is https://github.com/RussTedrake/drake/blob/57697231447b9d19a430dfa4fe54b69f82a2be58/bindings/pydrake/examples/BUILD.bazel#L66-L67 (ala #8911). In cc_deps, you can't list things that use elements from libdrake.so, unless you link them via libdrake.so. Those lines should be removed, and instead add the manipulation_station to https://github.com/RobotLocomotion/drake/blob/master/tools/install/libdrake/build_components.bzl on your feature branch for now.

@jwnimmer-tri
Copy link
Collaborator

@RussTedrake I believe that this patch will, for your feature branch, solve the ODR issues for manipulation_station, by pulling examples/manipulation_station (as well as some of examples/kuka_iiwa, and geometry/dev, and sensors/dev, and ...) into libdrake.so. You'd apply this patch, and then run ./tools/install/libdrake/build_components_refresh.py --force to update the list of components.

jwnimmer@call-cps:~/jwnimmer-tri/drake$ git diff
diff --git a/bindings/pydrake/examples/BUILD.bazel b/bindings/pydrake/examples/BUILD.bazel
index af320ac..297c12b 100644
--- a/bindings/pydrake/examples/BUILD.bazel
+++ b/bindings/pydrake/examples/BUILD.bazel
@@ -63,8 +63,6 @@
     name = "manipulation_station_py",
     cc_deps = [
         "//bindings/pydrake:documentation_pybind",
-        "//examples/manipulation_station",
-        "//examples/manipulation_station:manipulation_station_hardware_interface",  # noqa
     ],
     cc_srcs = ["manipulation_station_py.cc"],
     package_info = PACKAGE_INFO,
diff --git a/tools/install/libdrake/build_components_refresh.py b/tools/install/libdrake/build_components_refresh.py
index 1a0aea1..8a208c1 100755
--- a/tools/install/libdrake/build_components_refresh.py
+++ b/tools/install/libdrake/build_components_refresh.py
@@ -44,7 +44,11 @@ kind("cc_library", visible("//tools/install/libdrake:libdrake.so", "//..."))
     except(attr("testonly", "1", "//..."))
     except("//:*")
     except("//bindings/pydrake/...")
-    except("//examples/...")
+    except(
+      "//examples/..." except(
+        "//examples/manipulation_station/..."
+      )
+    )
     except("//lcmtypes/...")
     except("//tools/install/libdrake:*")
     except(attr(tags, "exclude_from_libdrake", //...))

@RussTedrake
Copy link
Contributor

it does indeed. thank you @jwnimmer-tri .

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

Successfully merging this pull request may close these issues.

4 participants