From 38d4428e5f878243e9fa129841fb2eb9377d423c Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Sun, 5 Jan 2025 11:57:57 -0800 Subject: [PATCH] [build] Use py_cc_toolchain for configuring pybind11 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 for compiling C code against the Python interpreter selected by the Python toolchain. This commit changes our python repository to define that new kind of toolchain and our pydrake bindings to use it. The toolchains are now encapsulated as `@python//:all`, moved from `//tools/py_toolchain`. Important note for future bzlmod users: because our python repository rule can fail when run on a system without our default interpreter, our MODULE.bazel will no longer register our default python toolchain automatically. Downstream Bazel projects using Drake as a module will need to opt-in to the local 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 legacy libraries `@python` and `@python//:python_direct_link` are deprecated. --- CMakeLists.txt | 3 + MODULE.bazel | 5 -- cmake/WORKSPACE.bzlmod.in | 5 +- cmake/bazel.rc.in | 3 + tools/bazel.rc | 3 + tools/install/install.bzl | 17 ++++- tools/install/libdrake/BUILD.bazel | 14 +++- tools/py_toolchain/BUILD.bazel | 37 ---------- tools/skylark/pybind.bzl | 2 +- tools/workspace/default.bzl | 10 +-- tools/workspace/lcm/package.BUILD.bazel | 3 +- tools/workspace/pybind11/package.BUILD.bazel | 3 +- tools/workspace/python/BUILD.bazel | 42 ++++++++++- tools/workspace/python/defs.bzl | 55 ++++++++++++++ tools/workspace/python/package.BUILD.bazel | 77 +++++++++++++++++--- tools/workspace/python/repository.bzl | 44 +++++------ 16 files changed, 220 insertions(+), 103 deletions(-) delete mode 100644 tools/py_toolchain/BUILD.bazel create mode 100644 tools/workspace/python/defs.bzl diff --git a/CMakeLists.txt b/CMakeLists.txt index 083c6cff135b..a2f336f3ce97 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -358,6 +358,9 @@ endfunction() set(BAZEL_WORKSPACE_EXTRA) set(BAZEL_WORKSPACE_EXCLUDES) +# Our cmake/WORKSPACE.bzlmod always provides @python. +list(APPEND BAZEL_WORKSPACE_EXCLUDES "python") + macro(override_repository NAME) set(repo "${CMAKE_CURRENT_BINARY_DIR}/external/workspace/${NAME}") string(APPEND BAZEL_WORKSPACE_EXTRA diff --git a/MODULE.bazel b/MODULE.bazel index 390b5780574a..983a7c83c987 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -23,11 +23,6 @@ cc_configure = use_extension( ) use_repo(cc_configure, "local_config_cc") -register_toolchains( - "//tools/py_toolchain:toolchain", - "//tools/py_toolchain:exec_tools_toolchain", -) - # TODO(#20731) Move all of our dependencies from WORKSPACE.bzlmod into this # file, so that downstream projects can consume Drake exclusively via bzlmod # (and so that we can delete our WORKSPACE files prior to Bazel 9 which drops diff --git a/cmake/WORKSPACE.bzlmod.in b/cmake/WORKSPACE.bzlmod.in index a3eaf1edee2e..6e95de09c039 100644 --- a/cmake/WORKSPACE.bzlmod.in +++ b/cmake/WORKSPACE.bzlmod.in @@ -16,11 +16,8 @@ python_repository( # Custom repository rules injected by CMake. @BAZEL_WORKSPACE_EXTRA@ -# The list of repositories already provided via BAZEL_WORKSPACE_EXTRA. -_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 = split_cmake_list("@BAZEL_WORKSPACE_EXCLUDES@"), bzlmod = True, ) diff --git a/cmake/bazel.rc.in b/cmake/bazel.rc.in index 01d375cc3c01..b3a139abc9b2 100644 --- a/cmake/bazel.rc.in +++ b/cmake/bazel.rc.in @@ -10,6 +10,9 @@ startup --output_base="@BAZEL_OUTPUT_BASE@" # Environment variables to be used in repository rules (if any). common @BAZEL_REPO_ENV@ +# Use the Python interpreter from our cmake/WORKSPACE.bzlmod.in. +build --extra_toolchains=@python//:all + # Disable the "convenience symlinks" intended for Bazel users; they only add # confusion for the CMake use case. build --symlink_prefix=/ diff --git a/tools/bazel.rc b/tools/bazel.rc index 403a1bb075f8..7c95058000d9 100644 --- a/tools/bazel.rc +++ b/tools/bazel.rc @@ -12,6 +12,9 @@ build -c opt build --strip=never build --strict_system_includes +# Use the host Python interpreter by default. +build --extra_toolchains=@python//:all + # Use C++20 by default. build --cxxopt=-std=c++20 build --host_cxxopt=-std=c++20 diff --git a/tools/install/install.bzl b/tools/install/install.bzl index bc8815770364..18a4f4168d5c 100644 --- a/tools/install/install.bzl +++ b/tools/install/install.bzl @@ -1,4 +1,3 @@ -load("@python//:version.bzl", "PYTHON_VERSION") load("@rules_license//rules:providers.bzl", "LicenseInfo") load("//tools/skylark:cc.bzl", "CcInfo") load("//tools/skylark:drake_java.bzl", "MainClassInfo") @@ -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. @@ -123,7 +132,7 @@ def _install_action( if "@WORKSPACE@" in dest: dest = dest.replace("@WORKSPACE@", _workspace(ctx)) if "@PYTHON_VERSION@" in dest: - dest = dest.replace("@PYTHON_VERSION@", PYTHON_VERSION) + dest = dest.replace("@PYTHON_VERSION@", _python_version(ctx)) if type(strip_prefixes) == "dict": strip_prefix = strip_prefixes.get( @@ -608,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): diff --git a/tools/install/libdrake/BUILD.bazel b/tools/install/libdrake/BUILD.bazel index 8a92c51cca5e..ce2367b9b01f 100644 --- a/tools/install/libdrake/BUILD.bazel +++ b/tools/install/libdrake/BUILD.bazel @@ -4,7 +4,6 @@ load( "@drake//tools/workspace:cmake_configure_file.bzl", "cmake_configure_file", ) -load("@python//:version.bzl", "PYTHON_VERSION") load( "//third_party:com_github_bazelbuild_rules_cc/whole_archive.bzl", "cc_whole_archive_library", @@ -29,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"], @@ -57,9 +63,9 @@ cmake_configure_file( src = "drake-config.cmake.in", out = "drake-config.cmake", atonly = True, - cmakelists = ["stamp_version.cmake"], - defines = [ - "PYTHON_VERSION=" + PYTHON_VERSION, + cmakelists = [ + ":python_version.cmake", + ":stamp_version.cmake", ], ) diff --git a/tools/py_toolchain/BUILD.bazel b/tools/py_toolchain/BUILD.bazel deleted file mode 100644 index ed37723e1d53..000000000000 --- a/tools/py_toolchain/BUILD.bazel +++ /dev/null @@ -1,37 +0,0 @@ -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("//tools/lint:lint.bzl", "add_lint_tests") - -py_runtime( - name = "runtime", - interpreter_path = PYTHON_BIN_PATH, - python_version = "PY3", -) - -py_runtime_pair( - name = "runtime_pair", - py3_runtime = ":runtime", -) - -toolchain( - name = "toolchain", - toolchain = ":runtime_pair", - toolchain_type = "@rules_python//python:toolchain_type", -) - -py_exec_tools_toolchain( - name = "exec_tools", - precompiler = "@rules_python//tools/precompiler:precompiler", -) - -toolchain( - name = "exec_tools_toolchain", - toolchain = ":exec_tools", - toolchain_type = "@rules_python//python:exec_tools_toolchain_type", -) - -add_lint_tests() diff --git a/tools/skylark/pybind.bzl b/tools/skylark/pybind.bzl index d40c80567e69..339d8cb40e31 100644 --- a/tools/skylark/pybind.bzl +++ b/tools/skylark/pybind.bzl @@ -289,8 +289,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, diff --git a/tools/workspace/default.bzl b/tools/workspace/default.bzl index e2b509ddda7c..b54374715e79 100644 --- a/tools/workspace/default.bzl +++ b/tools/workspace/default.bzl @@ -391,16 +391,12 @@ def add_default_toolchains( set this to True if you are using bzlmod. """ if bzlmod: - # All toolchains are in MODULE.bazel already. + # The cc toolchain is in MODULE.bazel already. + # The py toolchain is in tools/bazel.rc already. return if "py" not in excludes: - native.register_toolchains( - "//tools/py_toolchain:toolchain", - ) - native.register_toolchains( - "//tools/py_toolchain:exec_tools_toolchain", - ) + native.register_toolchains("@python//:all") if "rust" not in excludes: register_rust_toolchains() diff --git a/tools/workspace/lcm/package.BUILD.bazel b/tools/workspace/lcm/package.BUILD.bazel index 512f33e25ef6..193a18217c2b 100644 --- a/tools/workspace/lcm/package.BUILD.bazel +++ b/tools/workspace/lcm/package.BUILD.bazel @@ -195,7 +195,8 @@ cc_binary( visibility = ["//visibility:private"], deps = [ ":lcm", - "@python", + "@drake//tools/workspace/python:cc_headers", + "@drake//tools/workspace/python:cc_libs", ], ) diff --git a/tools/workspace/pybind11/package.BUILD.bazel b/tools/workspace/pybind11/package.BUILD.bazel index caedfd0e9e21..1d411564b312 100644 --- a/tools/workspace/pybind11/package.BUILD.bazel +++ b/tools/workspace/pybind11/package.BUILD.bazel @@ -61,7 +61,8 @@ cc_library( includes = ["include"], deps = [ "@eigen", - "@python", + "@drake//tools/workspace/python:cc_headers", + "@drake//tools/workspace/python:cc_libs", ], ) diff --git a/tools/workspace/python/BUILD.bazel b/tools/workspace/python/BUILD.bazel index b77b93ae0dbc..ab2f4b293c22 100644 --- a/tools/workspace/python/BUILD.bazel +++ b/tools/workspace/python/BUILD.bazel @@ -1,6 +1,42 @@ -# 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(":defs.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. This file may be used as srcs or data +# for any other rule whose action needs to know the current python version. +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) for the current Python toolchain. +# This may be used like it was a cc_library target that listed hdrs= and +# includes= for the current Python's header files. +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) for the current Python toolchain. +# This may be used like it was a cc_library target that listed linkopts= for +# any libraries use by a Python module. Note that this is intended for linking +# native modules, and does NOT link the libpython embedded runtime; for that, +# use cc_libpython below. +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++ +# executable that embeds the current Python toolchain's interpreter (e.g., for +# Python unit tests which are implemented as C++ programs). This alias may be +# used like it was a cc_library target that listed linkopts= for the libpython +# embedded runtime. +current_py_cc_libpython( + name = "cc_libpython", +) add_lint_tests() diff --git a/tools/workspace/python/defs.bzl b/tools/workspace/python/defs.bzl new file mode 100644 index 000000000000..07bb13ad297b --- /dev/null +++ b/tools/workspace/python/defs.bzl @@ -0,0 +1,55 @@ +load("@rules_cc//cc/common:cc_common.bzl", "cc_common") +load("//tools/skylark:cc.bzl", "CcInfo") + +_PY_CC_TOOLCHAIN_TYPE = "@rules_python//python/cc:toolchain_type" + +# These rules are intended for use only by our neighboring BUILD.bazel file. +# The comments in that file explain how and why to use these rules' output. + +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 = """Provides the linker flags for how to link a C/C++ executable + that embeds a Python interpreter (e.g., for unit testing), based on the + current Python toolchain. Use this rule like a cc_library.""", +) + +def _python_version_txt(ctx): + py_cc_toolchain = ctx.toolchains[_PY_CC_TOOLCHAIN_TYPE].py_cc_toolchain + output = ctx.actions.declare_file(ctx.label.name) + ctx.actions.write( + output = output, + content = py_cc_toolchain.python_version, + is_executable = False, + ) + return [DefaultInfo( + files = depset([output]), + data_runfiles = ctx.runfiles(files = [output]), + )] + +python_version_txt = rule( + implementation = _python_version_txt, + toolchains = [_PY_CC_TOOLCHAIN_TYPE], + doc = """Generates a text file containing the major.minor version number of + the current Python toolchain, without any newlines.""", +) diff --git a/tools/workspace/python/package.BUILD.bazel b/tools/workspace/python/package.BUILD.bazel index e85a1ed49d46..097d13e27fe0 100644 --- a/tools/workspace/python/package.BUILD.bazel +++ b/tools/workspace/python/package.BUILD.bazel @@ -1,19 +1,26 @@ # -*- bazel -*- +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( - "version.bzl", + ":version.bzl", + "PYTHON_BIN_PATH", "PYTHON_INCLUDES", - "PYTHON_LINKOPTS_EMBEDDED", - "PYTHON_LINKOPTS_MODULE", + "PYTHON_LINKOPTS", + "PYTHON_VERSION", ) +package(default_visibility = ["//visibility:private"]) + licenses(["notice"]) # Python-2.0 # Only include the first level of headers and specific second level headers # included from `python_repository`. This excludes some third-party C headers # that may be nested within `/usr/include/python`, such as `numpy`, # when installed via `apt` on Ubuntu. -headers = glob( +HDRS = glob( [ "include/*/*", "include/*/cpython/*", @@ -23,22 +30,70 @@ headers = glob( ) cc_library( - name = "python_headers", - hdrs = headers, + name = "cc_headers", + hdrs = HDRS, includes = PYTHON_INCLUDES, - visibility = ["//visibility:private"], +) + +cc_library( + name = "cc_libs", + linkopts = PYTHON_LINKOPTS, ) cc_library( name = "python", - linkopts = PYTHON_LINKOPTS_MODULE, + deps = [ + ":cc_headers", + ":cc_libs", + ], visibility = ["//visibility:public"], - deps = [":python_headers"], + deprecation = "DRAKE DEPRECATED: This target is deprecated and will be removed from Drake on or after 2025-05-01. As a replacement, use both @drake//tools/workspace/python:cc_headers and @drake//tools/workspace/python:cc_libs.", # noqa ) cc_library( name = "python_direct_link", - linkopts = PYTHON_LINKOPTS_EMBEDDED, visibility = ["//visibility:public"], - deps = [":python_headers"], + deps = ["@drake//tools/workspace/python:cc_libpython"], + deprecation = "DRAKE DEPRECATED: This target is deprecated and will be removed from Drake on or after 2025-05-01. As a replacement, use @drake//tools/workspace/python:cc_libpython.", # noqa +) + +py_runtime( + name = "runtime", + interpreter_path = PYTHON_BIN_PATH, + python_version = "PY3", +) + +py_runtime_pair( + name = "runtime_pair", + py3_runtime = ":runtime", +) + +toolchain( + name = "toolchain", + toolchain = ":runtime_pair", + toolchain_type = "@rules_python//python:toolchain_type", +) + +py_exec_tools_toolchain( + name = "exec_tools", + precompiler = "@rules_python//tools/precompiler:precompiler", +) + +toolchain( + name = "exec_tools_toolchain", + toolchain = ":exec_tools", + toolchain_type = "@rules_python//python:exec_tools_toolchain_type", +) + +py_cc_toolchain( + name = "py_cc", + headers = ":cc_headers", + libs = ":cc_libs", + python_version = PYTHON_VERSION, +) + +toolchain( + name = "py_cc_toolchain", + toolchain = ":py_cc", + toolchain_type = "@rules_python//python/cc:toolchain_type", ) diff --git a/tools/workspace/python/repository.bzl b/tools/workspace/python/repository.bzl index 09da29331301..ffbd70d80a38 100644 --- a/tools/workspace/python/repository.bzl +++ b/tools/workspace/python/repository.bzl @@ -4,13 +4,15 @@ This rule configures Python for use by Drake, in two parts: makes them available to be used as a C/C++ dependency. (b) macOS only: creates (or syncs) a virtual environment for dependencies. -For part (a) there are two available targets: +For part (a) our goal is to create a @python//:version.bzl file with the +details of what we found, which is loaded by package.BUILD.bazel to define +the @python//:all toolchains, which are loaded by our tools/bazel.rc. -(1) "@python//:python" for the typical case where we are creating loadable -dynamic libraries to be used as Python modules. - -(2) "@python//:python_direct_link" for the unusual case of linking the CPython -interpreter as a library into an executable with a C++ main() function. +Anywhere in Drake that needs to *consume* the python toolchain information +should use the aliases provided by //tools/workspace/python which always cite +the current toolchain (which might not be Drake's toolchain in case a user +configured a different toolchain). Nothing in Drake should ever refer to any +@python//:foo label directly; everything must always happen via toolchain. For part (b) the environment is used in all python rules and tests by default, because the python toolchain's interpreter is the venv python3 binary. @@ -159,35 +161,23 @@ def _impl(repo_ctx): includes = _get_includes(repo_ctx, python_config) linkopts = _get_linkopts(repo_ctx, python_config) - # Specialize the the linker options based on whether we're linking a - # loadable module or an embedded interpreter. (For details, refer to - # the docs for option (1) vs (2) atop this file.) - linkopts_embedded = list(linkopts) - linkopts_embedded.insert(0, "-lpython" + version) - linkopts_module = list(linkopts) + # On macOS, we need to tweak the linkopts slightly. if repo_ctx.os.name == "mac os x": - linkopts_module.insert(0, "-undefined dynamic_lookup") + linkopts.insert(0, "-undefined dynamic_lookup") # Set up (or sync) the venv. bin_path = _prepare_venv(repo_ctx, python) version_content = """ -# DO NOT EDIT: generated by python_repository() -# WARNING: Avoid using this macro in any repository rules which require -# `load()` at the WORKSPACE level. Instead, load these constants through -# `BUILD.bazel` or `package.BUILD.bazel` files. - -PYTHON_BIN_PATH = "{bin_path}" -PYTHON_VERSION = "{version}" +PYTHON_BIN_PATH = {bin_path} +PYTHON_VERSION = {version} PYTHON_INCLUDES = {includes} -PYTHON_LINKOPTS_EMBEDDED = {linkopts_embedded} -PYTHON_LINKOPTS_MODULE = {linkopts_module} +PYTHON_LINKOPTS = {linkopts} """.format( - bin_path = bin_path, - version = version, - includes = includes, - linkopts_module = linkopts_module, - linkopts_embedded = linkopts_embedded, + bin_path = repr(bin_path), + version = repr(version), + includes = repr(includes), + linkopts = repr(linkopts), ) repo_ctx.file( "version.bzl",