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 27, 2024
1 parent e37610a commit 7d91e3e
Show file tree
Hide file tree
Showing 18 changed files with 287 additions and 107 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
33 changes: 22 additions & 11 deletions tools/install/install.bzl
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
load("@python//:version.bzl", "PYTHON_SITE_PACKAGES_RELPATH", "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 @@ -48,6 +47,16 @@ def _depset_to_list(x):
iter_list = x.to_list() if type(x) == "depset" else x
return iter_list

#------------------------------------------------------------------------------

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

def _python_version(ctx):
"""Returns a string a containing the major.minor version number of the
current Python toolchain."""
py_cc_toolchain = ctx.toolchains[_PY_CC_TOOLCHAIN_TYPE].py_cc_toolchain
return py_cc_toolchain.python_version

#------------------------------------------------------------------------------
def _output_path(ctx, input_file, strip_prefix = [], ignore_errors = False):
"""Compute output path (without destination prefix) for install action.
Expand Down Expand Up @@ -120,13 +129,10 @@ def _install_action(
else:
dest = dests

dest_replacements = (
("@WORKSPACE@", _workspace(ctx)),
("@PYTHON_SITE_PACKAGES@", PYTHON_SITE_PACKAGES_RELPATH),
)
for old, new in dest_replacements:
if old in dest:
dest = dest.replace(old, new)
if "@WORKSPACE@" in dest:
dest = dest.replace("@WORKSPACE@", _workspace(ctx))
if "@PYTHON_VERSION@" in dest:
dest = dest.replace("@PYTHON_VERSION@", _python_version(ctx))

if type(strip_prefixes) == "dict":
strip_prefix = strip_prefixes.get(
Expand Down Expand Up @@ -590,7 +596,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 All @@ -609,6 +617,10 @@ _install_rule = rule(
},
executable = True,
implementation = _install_impl,
toolchains = [
# Used to discern the major.minor site-packages path to install into.
_PY_CC_TOOLCHAIN_TYPE,
],
)

def install(tags = [], **kwargs):
Expand Down Expand Up @@ -636,8 +648,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
19 changes: 10 additions & 9 deletions tools/install/libdrake/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,6 @@ load(
"@drake//tools/workspace:cmake_configure_file.bzl",
"cmake_configure_file",
)
load(
"@python//:version.bzl",
"PYTHON_SITE_PACKAGES_RELPATH",
"PYTHON_VERSION",
)
load(
"//third_party:com_github_bazelbuild_rules_cc/whole_archive.bzl",
"cc_whole_archive_library",
Expand All @@ -33,6 +28,13 @@ load(":header_lint.bzl", "cc_check_allowed_headers")

package(default_visibility = ["//visibility:private"])

genrule(
name = "python_version_cmake",
srcs = ["//tools/workspace/python:python_version.txt"],
outs = ["python_version.cmake"],
cmd = """echo "set(PYTHON_VERSION \\"$$(cat $<)\\")" > $@""",
)

genrule(
name = "stamp_version",
outs = ["stamp_version.txt"],
Expand Down Expand Up @@ -61,10 +63,9 @@ cmake_configure_file(
src = "drake-config.cmake.in",
out = "drake-config.cmake",
atonly = True,
cmakelists = ["stamp_version.cmake"],
defines = [
"PYTHON_SITE_PACKAGES_RELPATH=" + PYTHON_SITE_PACKAGES_RELPATH,
"PYTHON_VERSION=" + PYTHON_VERSION,
cmakelists = [
":python_version.cmake",
":stamp_version.cmake",
],
)

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
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()
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/python@PYTHON_VERSION@/site-packages/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
32 changes: 29 additions & 3 deletions tools/workspace/python/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,6 +1,32 @@
# 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(":internal.bzl", "current_py_cc_libpython", "python_version_txt")

package(default_visibility = ["//visibility:public"])

# Provides a text file containing the major.minor version number of the current
# Python toolchain, without any newlines.
python_version_txt(
name = "python_version.txt",
)

# 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",
)

# 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",
)

# 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",
)

add_lint_tests()
Loading

0 comments on commit 7d91e3e

Please sign in to comment.