pybind / pybind11_bazel

Bazel wrapper around the pybind11 repository
Other
100 stars 54 forks source link

Avoid PYBIND_DEPS to be appended by default #68

Closed hofbi closed 2 months ago

hofbi commented 8 months ago

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.

junyer commented 8 months ago

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?

rickeylev commented 8 months ago

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"]
)
hofbi commented 5 months ago

The should we implement the solution with the implicit deps?

junyer commented 5 months ago

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

hofbi commented 2 months ago

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