drake-ros icon indicating copy to clipboard operation
drake-ros copied to clipboard

Support `extra_deps` arg for `rosidl` both `cc` and `py`

Open ali-bdai opened this issue 2 years ago • 6 comments

This stems from a problem I ran into yesterday, where python_rules numpy wasn't providing the cpp headers and I would end up in a following scenario -

ali@bdai_docker:/workspaces/bdai/projects/dexterity$ bazel build //...
INFO: Options provided by the client:
  Inherited 'common' options: --isatty=1 --terminal_columns=103
INFO: Reading rc options for 'build' from /workspaces/bdai/projects/dexterity/.bazelrc:
  'build' options: --announce_rc --cxxopt=-std=c++20 --cxxopt=-Werror --cxxopt=-Wall --cxxopt=-Wextra --cxxopt=-Wdouble-promotion --cxxopt=-Wformat --cxxopt=-Wunused --cxxopt=-Wfloat-equal --cxxopt=-Wshadow --cxxopt=-Wconversion --cxxopt=-Winline --cxxopt=-Wno-variadic-macros --cxxopt=-Wnon-virtual-dtor --cxxopt=-Wpedantic --incompatible_strict_action_env --@aspect_rules_py//py:interpreter_version=3.10.12
INFO: Analyzed 59 targets (5 packages loaded, 10431 targets configured).
INFO: Found 59 targets...
ERROR: /workspaces/bdai/projects/dexterity/ws/src/mocap_print/BUILD:5:24: Compiling ws/src/mocap_print/mocap_generated_interfaces/py/mocap_generated_interfaces/msg/_marker_s.c failed: (Exit 1): gcc failed: error executing command (from target //ws/src/mocap_print:_mocap_generated_interfaces_py_c) /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -MD -MF ... (remaining 110 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
bazel-out/k8-fastbuild/bin/ws/src/mocap_print/mocap_generated_interfaces/py/mocap_generated_interfaces/msg/_marker_s.c:11:10: fatal error: numpy/ndarrayobject.h: No such file or directory
   11 | #include "numpy/ndarrayobject.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~
      
  

And unsurprisingly -

ali@bdai_docker:/workspaces/bdai/projects/dexterity$ bazelisk query "@pypi_numpy//:*" | grep ndarrayobject
Loading: 0 packages loaded
@pypi_numpy//:site-packages/numpy/core/include/numpy/ndarrayobject.h
Loading: 0 packages loaded

Via this patch users would be able to provide the cc dependency as an extra. I don't think it's a perfect solution, but it is a solution that doesn't involve touching python_rules themselves. An example to use this would be -

rosidl_interfaces_group(
    name = "mocap_generated_interfaces",
    interfaces = [
        "msg/RigidBody.msg",
        "msg/Marker.msg",
        "msg/RigidBodies.msg",
    ],
    visibility = ["//:__pkg__"],
    deps = [
        "@ros2//:builtin_interfaces",
        "@ros2//:geometry_msgs",
    ],
    extra_deps = [
        "@numpy_repo//:numpy_cc",
    ],
)

cc: thanks @dta-bdai for this patch :)


This change is Reviewable

ali-bdai avatar Apr 18 '24 18:04 ali-bdai

#include "numpy/ndarrayobject.h"

Hm... I'm a bit suspicious that either (a) we have a bug in drake-ros that we're not able to observe (i.e., we have system-installed numpy, and you do not), or (b) there is something misconfigured on your side that requires numpy when really it shouldn't be required.

Do you know if you have system-wide installed numpy, or is it purely local to Bazel?

EricCousineau-TRI avatar Apr 18 '24 18:04 EricCousineau-TRI

Also, can you reproduce this error in drake-ros itself, perhaps by adjusting the installed dependencies and running within a container?

EricCousineau-TRI avatar Apr 18 '24 18:04 EricCousineau-TRI

#include "numpy/ndarrayobject.h"

Hm... I'm a bit suspicious that either (a) we have a bug in drake-ros that we're not able to observe (i.e., we have system-installed numpy, and you do not), or (b) there is something misconfigured on your side that requires numpy when really it shouldn't be required.

Do you know if you have system-wide installed numpy, or is it purely local to Bazel?

  • We do have a broken version of systemwide numpy but we're going for a more aggressive hermetic environment where we don't depend on system deps where we don't absolutely have to. This seemed to have solved the problem in the docker container we're running into. On our end, it's much better to modify our bazel approach than to change dockerfiles as of now.

ali-bdai avatar Apr 18 '24 19:04 ali-bdai

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ali-bdai)

a discussion (no related file): nit The usage of extra_deps may be ambiguous w.r.t. what language this is intended for; you may get mixed providers (e.g. cc going where py should be or vice-versa).

If this approach is indeed needed, then the language type should be qualified.

Absolutely, that's why we wanted to push stuff upstream as well. Currently this works for us "as it is" as a temporary solution.

ali-bdai avatar Apr 18 '24 19:04 ali-bdai

If this approach is indeed needed, ..

As primary the maintainer of TRI's starlark rule set, I'll advocate that drake-ros should accept & merge something like this, even if it's not "needed" for numpy. Macros that wrap things like creating a library should always do their best to pass through options from higher layer callers above them down into the lower layer primitives like cc_library and py_library. It's not really feasible to argue that pass-throughs are never necessary, because we can't foresee every circumstance.

Certainly root causing and fixing the numpy thing the "right" way is ideal, but having an escape hatch is important for improving iteration speed and allowing for hotfixes during emergencies.

... the language type should be qualified.

I agree with this part. It should be named cc_extra_deps and py_extra_deps, or cc_deps_extra and py_deps_extra.

jwnimmer-tri avatar May 19 '24 14:05 jwnimmer-tri