pybind11_bazel icon indicating copy to clipboard operation
pybind11_bazel copied to clipboard

Avoid PYBIND_DEPS to be appended by default

Open hofbi opened this issue 1 year ago • 4 comments

For the pybind_library and similar targets, the PYBIND_DEPS are added by default to the deps list, see https://github.com/pybind/pybind11_bazel/blob/8889d39b2b925b2a47519ae09402a96f00ccf2b4/build_defs.bzl#L78C9-L78C35. Now if a user includes @pybind or @pybind//:pybind as a dependency, it errors out.

Ideally you could solve this by turning this list into a set, adding the list to it then creating a list from it again to avoid duplicates. If you turn all of these into sets, you should not get duplicates anymore.

hofbi avatar Jan 18 '24 13:01 hofbi

I don't think I have ever seen any Starlark that does that, but it could simply be that I haven't seen a lot of Starlark... What do you reckon, @rickeylev?

junyer avatar Jan 18 '24 13:01 junyer

It would mostly work, but I don't think it's a complete solution. I think if you pass in a select() expression (which can't be inspected) it can result in the same error. e.g., I think this results in an error:

deps = ["//foo"] + select({"//conditions:default": ["//foo"]})

The solution is to move the implicit deps behind another target e.g.

def pybind_library(deps, ...):
  py_library(
    deps = deps + ["//pybind:implicit_deps"]
  )

# pybind/BUILD
py_library(
  name = "implicit_deps",
  deps = ["//foo"]
)

rickeylev avatar Jan 18 '24 17:01 rickeylev

The should we implement the solution with the implicit deps?

hofbi avatar Apr 10 '24 19:04 hofbi

Is it still worth doing given that commit https://github.com/pybind/pybind11_bazel/commit/a5d4fd632ddf8253b1741884d0a05a51af550b68 turned @pybind11 into an internal dependency?

junyer avatar Apr 11 '24 14:04 junyer

I think this was fixed by https://github.com/pybind/pybind11_bazel/pull/73 as well

hofbi avatar Jul 24 '24 13:07 hofbi