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

Support extra_deps arg for rosidl both cc and py #345

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ali-bdai
Copy link

@ali-bdai ali-bdai commented Apr 18, 2024

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

@EricCousineau-TRI
Copy link
Collaborator

#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
Copy link
Collaborator

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

Copy link
Collaborator

@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.

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.


@ali-bdai
Copy link
Author

#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
Copy link
Author

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.

@jwnimmer-tri
Copy link
Contributor

jwnimmer-tri commented May 19, 2024

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.

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.

3 participants