feat: updates to the py_extension rule for building C extensions#3851
Conversation
…y from the platform/cpu/arch.
…mited API in our extension.
…have compatible settings for Py_LIMITED_API.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Python Limited API in the py_extension rule, including validation logic to ensure C++ dependencies are binary-compatible with the targeted Limited API version. It also transitions dynamic dependency handling to use CcSharedLibraryInfo and adds comprehensive tests. The feedback highlights several important improvements for py_extension_rule.bzl: fixing a bug where standard Windows extensions are incorrectly named with a .so extension, optimizing performance by lazily flattening the headers depset, avoiding trailing slashes in import_path when the package is empty, reusing the cc_toolchain variable, using Starlark integer division // instead of float division, and stripping trailing C integer suffixes from Py_LIMITED_API defines before parsing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Thanks! I'll do a post-merge review later |
|
|
||
| # Windows uses .pyd; Unix (Linux/macOS) uses .so for Python modules | ||
| target_name = cc_toolchain.target_gnu_system_name | ||
| is_windows = "windows" in target_name or "mingw" in target_name or "msvc" in target_name |
There was a problem hiding this comment.
Use platform constraints to detect the target platform instead of the cc toolchain. Grep around for target_platform_has_constraint. I think in attributes.bzl there's some shared constants for defining attributes to detect the platform, and helpers for accessing them in common.bzl (don't 100% recall)
There was a problem hiding this comment.
The pieces in attributes.bzl and common.bzl look like they are specifically for Apple platforms, and don't apply to the general case. It looks like it checks if any constraints match and then injects the requires-darwin tag. It's not re-usable for the case when we want to determine which specific platform we're compiling for.
There was a problem hiding this comment.
Ah here it is: see is_windows_platform in python/private/common.bzl -- its a helper specifically designed to detect if the target platform being built for is windows
…el-contrib#3851) This builds on the existing `py-extension` branch. It adds support for platform and abi tags in the resulting library filename. The `:py_extension_test`, `:py_extension_analysis_tests`, and `:py_limited_api_tests` test targets in `//tests/cc/py_extension` now build and pass. Relates to bazel-contrib#3283
…tension (#3875) Per feedback from #3851, this PR re-works the platform detection logic to rely on platform constraints instead of the Python runtime/toolchain. ### Description This PR contains updates to the experimental `py_extension` implementation. It re-works the platform tag detection to rely on modern platform constraints, and refactors the compilation and linking mechanism to delegate to Bazel's native `cc_shared_library` and `cc_library` rules. This PR targets the main repository's `py-extension` branch (not `main`). ### Key Changes #### 1. Platform Tag & ABI Tag Derivation * **Introduced `abi_tag` to `PyCcToolchainInfo`**: Added the `abi_tag` field to the `PyCcToolchainInfo` provider, populated by the `py_cc_toolchain` rule. It defaults to deriving the tag from `python_version` (e.g. `cpython-311`) for backward compatibility. * **Constraint-Based Detection**: Replaced legacy parsing of `cc_toolchain` CPU names with direct lookup in rules_python's central `PLATFORMS` registry using platform constraints. * **Limited API Configuration**: Updated how limited API (`.abi3.so`) suffixes are appended based on the toolchain configuration. #### 2. Compilation & Linking Delegation (Refactor to `cc_shared_library`) * **Macro Fusion**: Replaced the custom linking logic in the `py_extension` rule with a macro of the same name. It now accepts C/C++ source/header files directly (`srcs`, `hdrs`, `copts`, `defines`), implicitly wrapping them in a private `cc_library` under the hood. * **Consolidated Attributes**: Consolidated static linkage under standard `deps` (removing the redundant `static_deps` attribute) and aligned linker arguments with `cc_shared_library`'s `user_link_flags`. * **Wrapper Rule (`_py_extension_wrapper`)**: Added a lightweight, private rule that wraps the `cc_shared_library` output to: * Query the Python toolchain for platform/ABI tags to resolve PEP 3149 compliant filenames (e.g. `.cpython-311-x86_64-linux-gnu.so` or `.abi3.so`). * Create a cheap `symlink` from the CSL output to the PEP 3149 name. * Propagate `PyInfo` (for python rules) and `CcSharedLibraryInfo` (for dynamic C++ dependencies). * **ODR Validation**: Leveraging native `cc_shared_library` ensures strict analysis-time validation against One Definition Rule (ODR) violations (e.g., duplicate static linkage in dynamic chains). #### 3. Runfiles and Data Support * **Runfiles Propagation**: Re-implemented runfiles collection using the repository's standard `builders.RunfilesBuilder` to support runtime assets (`data` attribute) and dynamic library dependency propagation. ### Verification & Testing * Added parity test cases in `dependency_graph_tests.bzl` to verify dynamic dependency chains and static sharing behavior. * Added runfiles validation in `py_extension_tests.bzl` to verify data asset propagation. * Updated limited API tests to match CSL output structure. * All 14 tests in `//tests/cc/py_extension/...` are compiling and passing. ────── TAG=agy CONV=23f8a5e8-2d99-401a-9903-1256b7d42b0e --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
2 participants
This builds on the existing
py-extensionbranch. It adds support for platform and abi tags in the resulting library filename. The:py_extension_test,:py_extension_analysis_tests, and:py_limited_api_teststest targets in//tests/cc/py_extensionnow build and pass.Relates to #3283