Bazelize drake_ros_tf2 #200

sloretz · 2022-12-02T23:39:35Z

This change is

sloretz

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion

a discussion (no related file):

Current issue:

test_tf_broadcaster_py can't import from drake_ros_core.

Traceback (most recent call last):
  File "/home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/sandbox/linux-sandbox/1/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/test_tf_broadcaster_py.runfiles/drake_ros_tf2/test/test_tf_broadcaster.py", line 19, in <module>
    from drake_ros_core import RosInterfaceSystem
ImportError: cannot import name 'RosInterfaceSystem' from 'drake_ros_core' (/home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/sandbox/linux-sandbox/1/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/test_tf_broadcaster_py.runfiles/drake_ros_core/__init__.py)

I think the issue is the runfiles create a folder called drake_ros_core with an empty __init__.py, then create a src/drake_ros_core/ folder which has the real __init__.py. I'm trying to see how I can trick or modify pybind_py_library to create the correct structure.

$ tree /home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/sandbox/linux-sandbox/1/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/test_tf_broadcaster_py.runfiles/drake_ros_core/
/home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/sandbox/linux-sandbox/1/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/test_tf_broadcaster_py.runfiles/drake_ros_core/
├── _drake_ros_core.cpython-38-x86_64-linux-gnu.so -> /home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/external/drake_ros_core/_drake_ros_core.cpython-38-x86_64-linux-gnu.so
├── __init__.py
├── __pycache__
│   └── __init__.cpython-38.pyc
└── src
    ├── drake_ros_core
    │   └── __init__.py -> /home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/execroot/drake_ros_tf2/external/drake_ros_core/src/drake_ros_core/__init__.py
    └── __init__.py

EricCousineau-TRI

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion

a discussion (no related file):

Previously, sloretz (Shane Loretz) wrote…

Current issue:

test_tf_broadcaster_py can't import from drake_ros_core.

Traceback (most recent call last):
  File "/home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/sandbox/linux-sandbox/1/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/test_tf_broadcaster_py.runfiles/drake_ros_tf2/test/test_tf_broadcaster.py", line 19, in <module>
    from drake_ros_core import RosInterfaceSystem
ImportError: cannot import name 'RosInterfaceSystem' from 'drake_ros_core' (/home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/sandbox/linux-sandbox/1/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/test_tf_broadcaster_py.runfiles/drake_ros_core/__init__.py)

I think the issue is the runfiles create a folder called drake_ros_core with an empty __init__.py, then create a src/drake_ros_core/ folder which has the real __init__.py. I'm trying to see how I can trick or modify pybind_py_library to create the correct structure.

$ tree /home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/sandbox/linux-sandbox/1/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/test_tf_broadcaster_py.runfiles/drake_ros_core/
/home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/sandbox/linux-sandbox/1/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/test_tf_broadcaster_py.runfiles/drake_ros_core/
├── _drake_ros_core.cpython-38-x86_64-linux-gnu.so -> /home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/execroot/drake_ros_tf2/bazel-out/k8-fastbuild/bin/external/drake_ros_core/_drake_ros_core.cpython-38-x86_64-linux-gnu.so
├── __init__.py
├── __pycache__
│   └── __init__.cpython-38.pyc
└── src
    ├── drake_ros_core
    │   └── __init__.py -> /home/osrf/.cache/bazel/_bazel_osrf/5c02e3f1f40a737d073de695c12319b8/execroot/drake_ros_tf2/external/drake_ros_core/src/drake_ros_core/__init__.py
    └── __init__.py

Working: Will provide pointers on the oddities of Bazel's externals, shared libs and relative RPATH's, etc.

Signed-off-by: Shane Loretz <[email protected]>

sloretz

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Will provide pointers on the oddities of Bazel's externals, shared libs and relative RPATH's, etc.

I ended up tracing this issue to bazelbuild/bazel#7386. Using legacy_create_init = False in the test resolves the issue.

EricCousineau-TRI · 2022-12-06T11:48:29Z

Nice! I was going to say maybe RobotLocomotion/drake#8811 is related, but if this fixes it, then awesome!
(It could be the workaround is no longer relevant)

EricCousineau-TRI · 2022-12-06T11:52:04Z

Er, sorry - must have fat-fingered the closing!

EricCousineau-TRI

First pass done, PTAL!

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @sloretz)

a discussion (no related file):

Previously, sloretz (Shane Loretz) wrote…

I ended up tracing this issue to bazelbuild/bazel#7386. Using legacy_create_init = False in the test resolves the issue.

Sweet!

drake_ros_tf2/BUILD.bazel line 79 at r2 (raw file):

    name = "drake_ros_tf2_py",
    cc_deps = [
        ":drake_ros_tf2",

This will (or at least should) cause an ODR issue between static Drake cc deps and the shared library Drake cc deps (that may come in through py deps).
Specifically, it should come in when using global counters (e.g. geometry ids, etc.).

For an example of this see what I linked to in #151

Possible options to move forward:

Huge TODO saying this is probably wrong.
Fixing it here and now.

drake_ros_tf2/WORKSPACE line 39 at r2 (raw file):

# Depend on Drake
DRAKE_TAG = "v1.10.0"

Working: Synchronizing this (and other components in this WORKSPACE) among separate repositories will quickly get away from us.
Filing issue...

EricCousineau-TRI

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sloretz)

drake_ros_tf2/WORKSPACE line 39 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Synchronizing this (and other components in this WORKSPACE) among separate repositories will quickly get away from us.
Filing issue...

Done. Filed #201.

sloretz

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

drake_ros_tf2/BUILD.bazel line 79 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This will (or at least should) cause an ODR issue between static Drake cc deps and the shared library Drake cc deps (that may come in through py deps).
Specifically, it should come in when using global counters (e.g. geometry ids, etc.).

For an example of this see what I linked to in #151

Possible options to move forward:

Huge TODO saying this is probably wrong.

Fixing it here and now.

ah, meaning using :drake_ros_tf2 is equivalent to linking statically? That is if I made two pybind libraries foo_pybind and bar_pybind, and both depended on :drake_ros_tf2, followed by a pure python library baz_py then ODR would be violated? Is the solution to create a shared library target that depends on :drake_ros_tf2?

                                                                ┌─────────────┐
                                                                │drake_ros_tf2│
                                                                └──┬──────────┘
                                                                   │
                                                                   │:drake_ros_tf2
                                                                   │
              ┌─────────────┐                            ┌─────────▼──────────────┐
              │drake_ros_tf2│                            │drake_ros_tf2_shared_lib│
              └─┬───────┬───┘                            └───────┬───────┬────────┘
                │       │                                        │       │
                │       │                                        │       │
       ┌────────┘       └──────────┐                    ┌────────┘       └──────────┐
       │     :drake_ros_tf2        │                    │ :drake_ros_tf2_shared_lib │
       │                           │                    │                           │
       │                           │                    │                           │
┌──────▼───┐                 ┌─────▼────┐        ┌──────▼───┐                 ┌─────▼────┐
│foo_pybind│                 │bar_pybind│        │foo_pybind│                 │bar_pybind│
└─────┬────┘                 └─────┬────┘        └─────┬────┘                 └─────┬────┘
      │                            │                   │                            │
      │                            │                   │                            │
      │:foo_pybind                 │:bar_pybind        │:foo_pybind                 │:bar_pybind
      │                            │                   │                            │
      │                            │                   │                            │
      └───────────┐      ┌─────────┘                   └───────────┐      ┌─────────┘
                  │      │                                         │      │
                  │      │  [violates ODR]                         │      │    [OK because definitions
                 ┌▼──────▼┐                                       ┌▼──────▼┐    in one shared lib]
                 │ baz_py │                                       │ baz_py │
                 └────────┘                                       └────────┘

Or is it more complicated than that? I see the example shared library depends on Drake's shared library. Do we need to use shared libraries all the way back up the dependency tree so that ODR won't be violated by depending any transitive dependency twice?

EricCousineau-TRI · 2022-12-06T21:10:21Z

Yup, that's correct!

Is the solution to create a shared library target that depends on :drake_ros_tf2? [...] Or is it more complicated than that?

Depends! However, I think we should (and must) service both for Anzu, because Anzu itself uses both.

Can I ask if you can look at the ODR example I posted? It has an additional solution:
#151
More specifically:
https://github.com/EricCousineau-TRI/repro/tree/1bc74a62/drake_stuff/drake_downstream_pybind
This has (a version of) the solution that we're employing in Anzu itself.

EricCousineau-TRI · 2022-12-06T21:10:47Z

That being said, I think we can move forward on this PR, just with a big TODO that says "this works here, but may break in real usage" ?

sloretz

Added TODO in 1f838a9

Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

EricCousineau-TRI

, thanks!

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sloretz)

drake_ros_tf2/BUILD.bazel line 79 at r2 (raw file):

Previously, sloretz (Shane Loretz) wrote…

ah, meaning using :drake_ros_tf2 is equivalent to linking statically? That is if I made two pybind libraries foo_pybind and bar_pybind, and both depended on :drake_ros_tf2, followed by a pure python library baz_py then ODR would be violated? Is the solution to create a shared library target that depends on :drake_ros_tf2?

                                                                ┌─────────────┐
                                                                │drake_ros_tf2│
                                                                └──┬──────────┘
                                                                   │
                                                                   │:drake_ros_tf2
                                                                   │
              ┌─────────────┐                            ┌─────────▼──────────────┐
              │drake_ros_tf2│                            │drake_ros_tf2_shared_lib│
              └─┬───────┬───┘                            └───────┬───────┬────────┘
                │       │                                        │       │
                │       │                                        │       │
       ┌────────┘       └──────────┐                    ┌────────┘       └──────────┐
       │     :drake_ros_tf2        │                    │ :drake_ros_tf2_shared_lib │
       │                           │                    │                           │
       │                           │                    │                           │
┌──────▼───┐                 ┌─────▼────┐        ┌──────▼───┐                 ┌─────▼────┐
│foo_pybind│                 │bar_pybind│        │foo_pybind│                 │bar_pybind│
└─────┬────┘                 └─────┬────┘        └─────┬────┘                 └─────┬────┘
      │                            │                   │                            │
      │                            │                   │                            │
      │:foo_pybind                 │:bar_pybind        │:foo_pybind                 │:bar_pybind
      │                            │                   │                            │
      │                            │                   │                            │
      └───────────┐      ┌─────────┘                   └───────────┐      ┌─────────┘
                  │      │                                         │      │
                  │      │  [violates ODR]                         │      │    [OK because definitions
                 ┌▼──────▼┐                                       ┌▼──────▼┐    in one shared lib]
                 │ baz_py │                                       │ baz_py │
                 └────────┘                                       └────────┘

Or is it more complicated than that? I see the example shared library depends on Drake's shared library. Do we need to use shared libraries all the way back up the dependency tree so that ODR won't be violated by depending any transitive dependency twice?

OK Resolved in scope of immediate PR per TODO and tracked via #151

sloretz added 2 commits December 2, 2022 11:44

Add rmw_isolation

8ce20d6

Add BUILD.bazel and WORKSPACE for drake_ros_tf2

fb06bfc

sloretz commented Dec 2, 2022

View reviewed changes

EricCousineau-TRI requested changes Dec 5, 2022

View reviewed changes

sloretz added 3 commits December 5, 2022 17:27

Use legacy_create_init to workaround bazelbuild/bazel#7386

aec9df8

Signed-off-by: Shane Loretz <[email protected]>

No underscore in module name

ad7eace

Fix bazel linting

c2b642d

sloretz commented Dec 6, 2022

View reviewed changes

sloretz changed the title ~~[WIP] Bazelize drake_ros_tf2~~ Bazelize drake_ros_tf2 Dec 6, 2022

sloretz marked this pull request as ready for review December 6, 2022 02:14

EricCousineau-TRI closed this Dec 6, 2022

EricCousineau-TRI reopened this Dec 6, 2022

EricCousineau-TRI requested changes Dec 6, 2022

View reviewed changes

EricCousineau-TRI reviewed Dec 6, 2022

View reviewed changes

sloretz commented Dec 6, 2022

View reviewed changes

sloretz added 2 commits December 6, 2022 15:44

Add TODO about ODR violations

1f838a9

Use C++17

80f0027

sloretz commented Dec 6, 2022

View reviewed changes

EricCousineau-TRI approved these changes Dec 7, 2022

View reviewed changes

EricCousineau-TRI merged commit ba4e7cf into RobotLocomotion:main Dec 7, 2022

sloretz deleted the sloretz__bazelize__drake_ros_tf2 branch December 7, 2022 16:22

sloretz mentioned this pull request Jan 4, 2023

Add BUILD.bazel files for drake_ros_viz and drake_ros_tf2 #158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bazelize drake_ros_tf2 #200

Bazelize drake_ros_tf2 #200

sloretz commented Dec 2, 2022 •

edited by EricCousineau-TRI

Loading

sloretz left a comment

EricCousineau-TRI left a comment

sloretz left a comment

EricCousineau-TRI commented Dec 6, 2022

EricCousineau-TRI commented Dec 6, 2022

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

sloretz left a comment

EricCousineau-TRI commented Dec 6, 2022 •

edited

Loading

EricCousineau-TRI commented Dec 6, 2022

sloretz left a comment

EricCousineau-TRI left a comment

Bazelize drake_ros_tf2 #200

Bazelize drake_ros_tf2 #200

Conversation

sloretz commented Dec 2, 2022 • edited by EricCousineau-TRI Loading

sloretz left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

sloretz left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Dec 6, 2022

EricCousineau-TRI commented Dec 6, 2022

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

sloretz left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Dec 6, 2022 • edited Loading

EricCousineau-TRI commented Dec 6, 2022

sloretz left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

sloretz commented Dec 2, 2022 •

edited by EricCousineau-TRI

Loading

EricCousineau-TRI commented Dec 6, 2022 •

edited

Loading