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

initial support for rules_python #64

Closed
junyer opened this issue Nov 27, 2023 · 5 comments
Closed

initial support for rules_python #64

junyer opened this issue Nov 27, 2023 · 5 comments

Comments

@junyer
Copy link
Contributor

junyer commented Nov 27, 2023

See bazelbuild/bazel#8674 (comment) for background.

For historical reasons, this logic enforces that python_version is "2", "3" or "". My current thinking is to extend that to permit "3.<minor>" and "DEFAULT" and "resolve" such versions to Labels via INTERPRETER_LABELS (and DEFAULT_PYTHON_VERSION) as provided by @pythons_hub//:interpreters.bzl.

Ping (again), @rickeylev and @rwgk. :)

@mutongx
Copy link

mutongx commented Nov 28, 2023

Thanks @junyer! I was previously unsure about where to ask for help so I ended up with that workaround in my company internal repo. Very happy to see that pybind11_bazel is initiating support for rules_python, and please let me know if you need any testers. I will be here to help.

@junyer
Copy link
Contributor Author

junyer commented Nov 28, 2023

Glad to hear that you are keen, @mutongx. #65 isn't going to fly, unfortunately, but I do want to solve this problem in the not too distant future, so... back to the drawing board. :)

@rickeylev
Copy link
Collaborator

I just wanted to drop a couple sketches of how I see this likely working. This is based on how Google makes pybind work internally, where the C++ integration is better with the Python rules.

Fundamentally, what we want to to is define a cc_library that depends on targets that provide the headers, copts, and linkopts we need. Where do we get that information? From "the toolchain", specifically, the @rules_python//python/cc:toolchain_type toolchain.

The end result would look something like this...

def pybind11_library(name, ...):
  py_library(name=name, deps=[name + "_cc"], ...)
  cc_library(
    name = name + "_cc",
    deps = [
      "@rules_python//python/cc:current_py_cc_headers",
      "@rules_python//python/cc:current_py_dsos",
      "@rules_python//python/cc:current_py_linkopts",
      "@rules_python//python/cc:current_py_copts",
    ]
)

I broke out separate current_py_* targets just for illustrative purposes. I'm not entirely sure the granularity that is needed. In any case, think of those targets "magic" cc_library targets that rules_python defines that know how to get the right values. e.g., they conceptually look like this:

# rules_python/python/cc/BUILD
cc_library(name = "current_py_cc_headers", srcs=["Python.h"])
cc_library(name = "current_py_dsos", srcs="libpython310.so"])
cc_library(name = "current_py_linkopts", linkopts=["-lm", ...], ...)

(except they use toolchain stuff to fill the values in).

@junyer
Copy link
Contributor Author

junyer commented Dec 1, 2023

SGTM. Thanks! :D

@junyer
Copy link
Contributor Author

junyer commented Feb 9, 2024

Closing this issue in favour of issue #71 (and PR #72).

@junyer junyer closed this as completed Feb 9, 2024
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 a pull request may close this issue.

3 participants