feat: updates to the py_extension rule for building C extensionsPy extension#3875
Conversation
…tions and may produce false-positives.
There was a problem hiding this comment.
Code Review
This pull request refactors the py_extension rule to use constraint-based platform tag derivation instead of C++ toolchain-based derivation, introduces abi_tag to PyCcToolchainInfo, and removes the _check_limited_api_compatibility validation logic. The review feedback highlights several critical issues: first, the new constraint-based platform detection is broken for musl Linux platforms because they share the same constraints as gnu platforms; second, changing the limited API check to bool(ctx.attr.py_limited_api) breaks backward compatibility for targets explicitly setting "none"; third, the hardcoded _constraints list should be dynamically derived from PLATFORMS to prevent future maintenance issues; and finally, the documentation for py_limited_api needs to be updated since the dependency validation logic was removed.
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.
|
The mypy CI failure appears to be caused by the check action using Python 3.9, but the newest mypy only supporting 3.10+ For what it's worth, I ran |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…using the obsolete PLATFORMS global.
96b6ba2 to
acf55b4
Compare
|
Attempting a rebase to fix the CI failures. |
acf55b4 to
aab3cee
Compare
|
Un-rebased |
|
@rickeylev Would it be okay if I rebased your py-extension branch (the base of this PR) onto main? That, together with rebasing my branch would fix most if not all of the CI failures, I think. I already did a dry-run locally and there was only a couple of conflicts which were basically nothing. |
rickeylev
left a comment
There was a problem hiding this comment.
just a couple small changes requested, but since we need to rebase atop main and this has been out awhile, lets just merge now. it ok if ci is failing in this WIP branch
| if "os" not in kwargs: | ||
| kwargs["os"] = select({ | ||
| "@platforms//os:macos": "macos", | ||
| "@platforms//os:windows": "windows", | ||
| "//conditions:default": "linux", | ||
| }) | ||
| if "cpu" not in kwargs: | ||
| kwargs["cpu"] = select({ | ||
| "@platforms//cpu:aarch64": "aarch64", | ||
| "@platforms//cpu:x86_32": "x86_32", | ||
| "//conditions:default": "x86_64", | ||
| }) |
There was a problem hiding this comment.
For os and cpu, lets expose the python-specific terminology and values for them on the toolchain (I think this is why PyRuntimeInfo does). There's several (os.name, platform.something, sys.platform, sysconfig values etc). I'm not sure which of them is most approrpriate, but we can defer that to another PR
…tension (bazel-contrib#3875) Per feedback from bazel-contrib#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>
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_extensionimplementation. It re-works the platform tag detection to rely on modern platform constraints, and refactors the compilation and linking mechanismto delegate to Bazel's native
cc_shared_libraryandcc_libraryrules.This PR targets the main repository's
py-extensionbranch (notmain).Key Changes
1. Platform Tag & ABI Tag Derivation
abi_tagtoPyCcToolchainInfo: Added theabi_tagfield to thePyCcToolchainInfoprovider, populated by thepy_cc_toolchainrule. It defaults to deriving the tag frompython_version(e.g.cpython-311) for backward compatibility.cc_toolchainCPU names with direct lookup in rules_python's centralPLATFORMSregistry using platform constraints..abi3.so) suffixes are appended based on the toolchain configuration.2. Compilation & Linking Delegation (Refactor to
cc_shared_library)py_extensionrule 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_libraryunder the hood.deps(removing the redundantstatic_depsattribute) and aligned linker arguments withcc_shared_library'suser_link_flags._py_extension_wrapper): Added a lightweight, private rule that wraps thecc_shared_libraryoutput to:.cpython-311-x86_64-linux-gnu.soor.abi3.so).symlinkfrom the CSL output to the PEP 3149 name.PyInfo(for python rules) andCcSharedLibraryInfo(for dynamic C++ dependencies).cc_shared_libraryensures strict analysis-time validation against One Definition Rule (ODR) violations (e.g., duplicate static linkage in dynamic chains).3. Runfiles and Data Support
builders.RunfilesBuilderto support runtime assets (dataattribute) and dynamic library dependencypropagation.
Verification & Testing
dependency_graph_tests.bzlto verify dynamic dependency chains and static sharing behavior.py_extension_tests.bzlto verify data asset propagation.//tests/cc/py_extension/...are compiling and passing.──────
TAG=agy
CONV=23f8a5e8-2d99-401a-9903-1256b7d42b0e