Skip to content

Commit

Permalink
[build] Use py_cc_toolchain for configuring pybind11
Browse files Browse the repository at this point in the history
The py_cc_toolchain is a rules_python concept that bridges the Python
and C++ toolchains by providing cc_library targets for the headers and
libraries that allow compiling C code against the Python interpreter
selected by the Python toolchain.

This commit changes our MODULE and WORKSPACE to add that new kind of
toolchain and our pydrake bindings to use it.

Therefore, it's no longer necessary for our prior approach (the
'@python' repository) to be public, so that repository is now
deprecated for removal.

To simplify this transition, we also revise how pydrake module shared
libraries are named in our binary packages. The importlib.machinery
package defines many possible values for EXTENSION_SUFFIXES (i.e., the
filename extension used for native code), e.g., for example on Ubuntu
22.04 the possible values are ['.cpython-310-x86_64-linux-gnu.so',
'.abi3.so', '.so']. Previously we used the first choice (with the
python version baked in) but this is awkward, so now we use the last
option (plain '.so'). The distinction only matters in case we package
multiple python versions into the same release artifact at once, which
we never do. It will also be moot once we switch to using ABI 3 in the
future (in which case we'll use the middle choice).

Ditto for lcm-python's shared library.

Details:

We now call our python_repository rule twice: once to create "python"
(which is deprecated and no longer unused by Drake) and once to create
"python_internal". The internal flavor provides fewer constants since
many are no longer needed with the py_cc_toolchain.

We provide new labels in //tools/workspace/python as a single point of
control for depending on Python. The comments in python/repository.bzl
provide an overview of how the pieces all fit together.

The PYTHON_SITE_PACKAGES is no longer used as an opaque constant; its
correct value is trivially derivable from the Python version number so
we'll just use the longhand anywhere we need such a path.
  • Loading branch information
jwnimmer-tri committed Dec 22, 2024
1 parent a023679 commit 00f2983
Show file tree
Hide file tree
Showing 21 changed files with 251 additions and 103 deletions.
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,9 @@ endfunction()
set(BAZEL_WORKSPACE_EXTRA)
set(BAZEL_WORKSPACE_EXCLUDES)

# Our cmake/WORKSPACE.bzlmod always provides @python_internal.
list(APPEND BAZEL_WORKSPACE_EXCLUDES "python_internal")

macro(override_repository NAME)
set(repo "${CMAKE_CURRENT_BINARY_DIR}/external/workspace/${NAME}")
string(APPEND BAZEL_WORKSPACE_EXTRA
Expand Down
1 change: 1 addition & 0 deletions MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use_repo(cc_configure, "local_config_cc")
register_toolchains(
"//tools/py_toolchain:toolchain",
"//tools/py_toolchain:exec_tools_toolchain",
"//tools/py_toolchain:py_cc_toolchain",
)

# TODO(#20731) Move all of our dependencies from WORKSPACE.bzlmod into this
Expand Down
2 changes: 1 addition & 1 deletion bindings/pydrake/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ install(
":pydrake_pyi",
":pydrake_typed",
],
data_dest = "@PYTHON_SITE_PACKAGES@",
data_dest = "lib/python@PYTHON_VERSION@/site-packages",
visibility = ["//visibility:public"],
deps = get_drake_py_installs(PY_LIBRARIES_WITH_INSTALL) + [
# These three modules are a special case.
Expand Down
5 changes: 3 additions & 2 deletions cmake/WORKSPACE.bzlmod.in
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ load("//tools/workspace:default.bzl", "add_default_workspace")
# Use Drake's python repository rule to interrogate the interpreter chosen by
# the CMake find_program stanza, in support of compiling our C++ bindings.
python_repository(
name = "python",
name = "python_internal",
linux_interpreter_path = "@Python_EXECUTABLE@",
macos_interpreter_path = "@Python_EXECUTABLE@",
requirements_flavor = "build",
is_drake_internal = True,
)

# Custom repository rules injected by CMake.
Expand All @@ -21,6 +22,6 @@ _BAZEL_WORKSPACE_EXCLUDES = split_cmake_list("@BAZEL_WORKSPACE_EXCLUDES@")

# For anything not already overridden, use Drake's default externals.
add_default_workspace(
repository_excludes = ["python"] + _BAZEL_WORKSPACE_EXCLUDES,
repository_excludes = _BAZEL_WORKSPACE_EXCLUDES,
bzlmod = True,
)
2 changes: 1 addition & 1 deletion tools/install/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ drake_py_library(
srcs = ["install_test_helper.py"],
data = [
"//:install",
"@python//:venv_bin",
"@python_internal//:venv_bin",
],
imports = ["."],
deps = [
Expand Down
11 changes: 6 additions & 5 deletions tools/install/install.bzl
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
load("@python//:version.bzl", "PYTHON_SITE_PACKAGES_RELPATH", "PYTHON_VERSION")
load("@python_internal//:version.bzl", "PYTHON_VERSION")
load("@rules_license//rules:providers.bzl", "LicenseInfo")
load("//tools/skylark:cc.bzl", "CcInfo")
load("//tools/skylark:drake_java.bzl", "MainClassInfo")
Expand Down Expand Up @@ -122,7 +122,7 @@ def _install_action(

dest_replacements = (
("@WORKSPACE@", _workspace(ctx)),
("@PYTHON_SITE_PACKAGES@", PYTHON_SITE_PACKAGES_RELPATH),
("@PYTHON_VERSION@", PYTHON_VERSION),
)
for old, new in dest_replacements:
if old in dest:
Expand Down Expand Up @@ -590,7 +590,9 @@ _install_rule = rule(
"runtime_strip_prefix": attr.string_list(),
"java_dest": attr.string(default = "share/java"),
"java_strip_prefix": attr.string_list(),
"py_dest": attr.string(default = "@PYTHON_SITE_PACKAGES@"),
"py_dest": attr.string(
default = "lib/python@PYTHON_VERSION@/site-packages",
),
"py_strip_prefix": attr.string_list(),
"rename": attr.string_dict(),
"install_tests": attr.label_list(
Expand Down Expand Up @@ -636,8 +638,7 @@ Destination paths may include the following placeholders:
* ``@WORKSPACE@``, replaced with ``workspace`` (if specified) or the name of
the workspace which invokes ``install``.
* ``@PYTHON_SITE_PACKAGES``, replaced with the Python version-specific path of
"site-packages".
* ``@PYTHON_VERSION@``, replaced with the Python major.minor version.
Note:
By default, headers and resource files to be installed must be explicitly
Expand Down
2 changes: 1 addition & 1 deletion tools/install/install_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def _run_one_command(self, test_command):
# are installed system-wide (without a venv).
if sys.platform == "darwin":
manifest = runfiles.Create()
python = manifest.Rlocation("python/bin/python3")
python = manifest.Rlocation("python_internal/bin/python3")
else:
python = install_test_helper.get_python_executable()

Expand Down
11 changes: 2 additions & 9 deletions tools/install/libdrake/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@ load(
"@drake//tools/workspace:cmake_configure_file.bzl",
"cmake_configure_file",
)
load(
"@python//:version.bzl",
"PYTHON_SITE_PACKAGES_RELPATH",
"PYTHON_VERSION",
)
load("@python_internal//:version.bzl", "PYTHON_VERSION")
load(
"//third_party:com_github_bazelbuild_rules_cc/whole_archive.bzl",
"cc_whole_archive_library",
Expand Down Expand Up @@ -62,10 +58,7 @@ cmake_configure_file(
out = "drake-config.cmake",
atonly = True,
cmakelists = ["stamp_version.cmake"],
defines = [
"PYTHON_SITE_PACKAGES_RELPATH=" + PYTHON_SITE_PACKAGES_RELPATH,
"PYTHON_VERSION=" + PYTHON_VERSION,
],
defines = ["PYTHON_VERSION=" + PYTHON_VERSION],
)

install_cmake_config(
Expand Down
2 changes: 1 addition & 1 deletion tools/install/libdrake/drake-config.cmake.in
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ set(${CMAKE_FIND_PACKAGE_NAME}_VERSION_PATCH "@DRAKE_VERSION_PATCH@")
set(${CMAKE_FIND_PACKAGE_NAME}_VERSION_TWEAK "@DRAKE_VERSION_TWEAK@")


set(${CMAKE_FIND_PACKAGE_NAME}_PYTHON_DIR "${${CMAKE_FIND_PACKAGE_NAME}_IMPORT_PREFIX}/@PYTHON_SITE_PACKAGES_RELPATH@")
set(${CMAKE_FIND_PACKAGE_NAME}_PYTHON_DIR "${${CMAKE_FIND_PACKAGE_NAME}_IMPORT_PREFIX}/lib/python@PYTHON_VERSION@/site-packages")
# Allow users to easily check Drake's expected CPython version.
set(${CMAKE_FIND_PACKAGE_NAME}_PYTHON_VERSION "@PYTHON_VERSION@")

Expand Down
2 changes: 1 addition & 1 deletion tools/jupyter/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ drake_jupyter_py_binary(

drake_py_unittest(
name = "jupyter_notebook_test",
data = ["@python//:venv_bin"],
data = ["@python_internal//:venv_bin"],
deps = [
"@rules_python//python/runfiles",
],
Expand Down
2 changes: 1 addition & 1 deletion tools/jupyter/test/jupyter_notebook_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def test_help(self):
"""Ensures that `jupyter notebook` is installed (#12042)."""
manifest = runfiles.Create()
if sys.platform == "darwin":
jupyter = manifest.Rlocation("python/bin/jupyter")
jupyter = manifest.Rlocation("python_internal/bin/jupyter")
else:
jupyter = "jupyter"

Expand Down
24 changes: 18 additions & 6 deletions tools/py_toolchain/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
load("@python//:version.bzl", "PYTHON_BIN_PATH")
load("@rules_python//python:defs.bzl", "py_runtime", "py_runtime_pair")
load(
"@rules_python//python:py_exec_tools_toolchain.bzl",
"py_exec_tools_toolchain",
)
load("@python_internal//:version.bzl", "PYTHON_BIN_PATH", "PYTHON_VERSION")
load("@rules_python//python:py_exec_tools_toolchain.bzl", "py_exec_tools_toolchain") # noqa
load("@rules_python//python:py_runtime.bzl", "py_runtime")
load("@rules_python//python:py_runtime_pair.bzl", "py_runtime_pair")
load("@rules_python//python/cc:py_cc_toolchain.bzl", "py_cc_toolchain")
load("//tools/lint:lint.bzl", "add_lint_tests")

py_runtime(
Expand Down Expand Up @@ -34,4 +33,17 @@ toolchain(
toolchain_type = "@rules_python//python:exec_tools_toolchain_type",
)

py_cc_toolchain(
name = "py_cc",
headers = "@python_internal//:cc_headers",
libs = "@python_internal//:cc_libs",
python_version = PYTHON_VERSION,
)

toolchain(
name = "py_cc_toolchain",
toolchain = ":py_cc",
toolchain_type = "@rules_python//python/cc:toolchain_type",
)

add_lint_tests()
32 changes: 32 additions & 0 deletions tools/py_toolchain/defs.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
load("@rules_cc//cc:defs.bzl", "cc_common")
load("//tools/skylark:cc.bzl", "CcInfo")

_PY_CC_TOOLCHAIN_TYPE = "@rules_python//python/cc:toolchain_type"

def _current_py_cc_libpython(ctx):
py_cc_toolchain = ctx.toolchains[_PY_CC_TOOLCHAIN_TYPE].py_cc_toolchain
linkopts = ["-lpython{}".format(py_cc_toolchain.python_version)]
return [
CcInfo(
compilation_context = None,
linking_context = cc_common.create_linking_context(
linker_inputs = depset(
direct = [
cc_common.create_linker_input(
owner = ctx.label,
user_link_flags = depset(linkopts),
),
],
),
),
),
]

current_py_cc_libpython = rule(
implementation = _current_py_cc_libpython,
toolchains = [_PY_CC_TOOLCHAIN_TYPE],
provides = [CcInfo],
doc = """Caclulates the linker flags for how to link a C/C++ exectuable
that embeds a Python interpreter (e.g., for unit testing), based on the
current Python toolchain.""",
)
2 changes: 1 addition & 1 deletion tools/skylark/py.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ load(
# doesn't use a requirements.txt, then the file will be empty (and thus inert).

def _add_requirements(data):
return (data or []) + ["@python//:requirements.txt"]
return (data or []) + ["@drake//tools/workspace/python:requirements"]

def py_binary(name, *, data = None, **kwargs):
_py_binary(
Expand Down
7 changes: 3 additions & 4 deletions tools/skylark/pybind.bzl
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
load("@cc//:compiler.bzl", "COMPILER_ID")
load("@python//:version.bzl", "PYTHON_EXTENSION_SUFFIX")
load("//tools/install:install.bzl", "install")
load("//tools/skylark:cc.bzl", "CcInfo", "cc_binary")
load("//tools/skylark:drake_cc.bzl", "drake_cc_binary", "drake_cc_googletest")
Expand Down Expand Up @@ -57,7 +56,7 @@ def pybind_py_library(

# TODO(eric.cousineau): See if we can keep non-`*.so` target name, but
# output a *.so, so that the target name is similar to what is provided.
cc_so_target = cc_so_name + PYTHON_EXTENSION_SUFFIX
cc_so_target = cc_so_name + ".so"

# Add C++ shared library.
cc_binary_rule(
Expand Down Expand Up @@ -232,7 +231,7 @@ def get_pybind_package_info(base_package, sub_package = None):
package_info = _get_package_info(base_package, sub_package)
return struct(
py_imports = [package_info.base_path_rel],
py_dest = "@PYTHON_SITE_PACKAGES@/{}".format(
py_dest = "lib/python@PYTHON_VERSION@/site-packages/{}".format(
package_info.sub_path_rel,
),
)
Expand Down Expand Up @@ -288,8 +287,8 @@ def drake_pybind_cc_googletest(
deps = cc_deps + [
"//:drake_shared_library",
"//bindings/pydrake:pydrake_pybind",
"//tools/workspace/python:cc_libpython",
"@pybind11",
"@python//:python_direct_link",
],
copts = cc_copts,
use_default_main = False,
Expand Down
5 changes: 5 additions & 0 deletions tools/workspace/default.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,8 @@ def add_default_repositories(
pycodestyle_repository(name = "pycodestyle", mirrors = mirrors)
if "python" not in excludes:
python_repository(name = "python")
if "python_internal" not in excludes:
python_repository(name = "python_internal", is_drake_internal = True)
if "qdldl_internal" not in excludes:
qdldl_internal_repository(name = "qdldl_internal", mirrors = mirrors)
if "qhull_internal" not in excludes:
Expand Down Expand Up @@ -401,6 +403,9 @@ def add_default_toolchains(
native.register_toolchains(
"//tools/py_toolchain:exec_tools_toolchain",
)
native.register_toolchains(
"//tools/py_toolchain:py_cc_toolchain",
)
if "rust" not in excludes:
register_rust_toolchains()

Expand Down
8 changes: 5 additions & 3 deletions tools/workspace/lcm/package.BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ load(
"generate_export_header",
)
load("@drake//tools/workspace:generate_file.bzl", "generate_file")
load("@python//:version.bzl", "PYTHON_EXTENSION_SUFFIX")

licenses([
"notice", # BSD-3-Clause
Expand Down Expand Up @@ -179,6 +178,8 @@ cc_binary(
],
)

PYTHON_EXTENSION_SUFFIX = ".so"

cc_binary(
name = "_lcm" + PYTHON_EXTENSION_SUFFIX,
srcs = [
Expand All @@ -196,7 +197,8 @@ cc_binary(
visibility = ["//visibility:private"],
deps = [
":lcm",
"@python",
"@drake//tools/workspace/python:cc_headers",
"@drake//tools/workspace/python:cc_libs",
],
)

Expand Down Expand Up @@ -308,7 +310,7 @@ install(
":_lcm" + PYTHON_EXTENSION_SUFFIX,
":lcm-python-upstream",
],
library_dest = "@PYTHON_SITE_PACKAGES@/lcm",
library_dest = "lib/site-packages/python@PYTHON_VERSION@/lcm",
py_strip_prefix = ["lcm-python"],
)

Expand Down
3 changes: 2 additions & 1 deletion tools/workspace/pybind11/package.BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ cc_library(
includes = ["include"],
deps = [
"@eigen",
"@python",
"@drake//tools/workspace/python:cc_headers",
"@drake//tools/workspace/python:cc_libs",
],
)

Expand Down
39 changes: 36 additions & 3 deletions tools/workspace/python/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,6 +1,39 @@
# This file exists to make our directory into a Bazel package, so that our
# neighboring *.bzl file can be loaded elsewhere.

load("//tools/lint:lint.bzl", "add_lint_tests")
load("//tools/py_toolchain:defs.bzl", "current_py_cc_libpython")

# All of Drake's Python code should depend on our requirements.txt pin. This
# filegroup provides a single point of control for any targets in Drake that
# need to depend on changes to our requirements file. If this particular build
# doesn't use a requirements.txt, then the file will be empty (and thus inert).
filegroup(
name = "requirements",
srcs = [
"@python_internal//:requirements.txt",
],
visibility = ["//visibility:public"],
)

# Provides a single point of control within Drake for how to compile a native
# C/C++ Python module (e.g., for pybind11).
alias(
name = "cc_headers",
actual = "@rules_python//python/cc:current_py_cc_headers",
visibility = ["//visibility:public"],
)

# Provides a single point of control within Drake for how to link a native
# C/C++ Python module (e.g., for pybind11).
alias(
name = "cc_libs",
actual = "@rules_python//python/cc:current_py_cc_libs",
visibility = ["//visibility:public"],
)

# Provides a single point of control within Drake for how to link a C/C++
# exectuable that embeds a Python interpreter (e.g., for unit testing).
current_py_cc_libpython(
name = "cc_libpython",
visibility = ["//visibility:public"],
)

add_lint_tests()
Loading

0 comments on commit 00f2983

Please sign in to comment.