feat: updates to the py_extension rule for building C extensionsPy extension#3875
feat: updates to the py_extension rule for building C extensionsPy extension#3875rsartor-cmd wants to merge 37 commits into
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>
…nfirm py_extension behaves similarly to cc_shared_library.
…ry and cc_shared_library targets.
| ) | ||
|
|
||
| def _csl_dynamic_deps_test_impl(env, target): | ||
| env.expect.that_target(target).has_provider(CcSharedLibraryInfo) |
There was a problem hiding this comment.
This test seems a little light. Don't we need to check more to ensure it actually linked everything correctly? Same for the others
There was a problem hiding this comment.
Yes, I can add tests for that. These tests were in the earlier stages, when I was working out what providers were necessary to return.
| py_extension( | ||
| name = "ext_limited", | ||
| py_limited_api = "3.8", | ||
| static_deps = [":ext_limited_impl"], |
There was a problem hiding this comment.
please rename static_deps to deps (can be done separate pr)
| imports = depset([import_path]), | ||
| ), | ||
| propagated_cc_info, | ||
| csl_target[CcSharedLibraryInfo], |
There was a problem hiding this comment.
Why do we propagate CcSharedLibraryInfo? It's so that the py extension can be given as a dynamic dep to another cc shared library? I'm not sure that makes sense -- lets discuss in chat.
| ], | ||
| # An extension that also depends on a data file | ||
| name = "ext_with_data", | ||
| data = ["test_symbols.h"], |
There was a problem hiding this comment.
Lets use a non-code type of file for a data dependency, i.e. somedata.txt
| ##### | ||
|
|
||
| py_extension( | ||
| # An extension with its PyInit_* function in a dynamic dependency |
There was a problem hiding this comment.
Don't support this case, or rather, don't add a test for it -- it doesn't make any sense. The PyInit symbol is supposed to be provided by the foo.so module being loaded. If its provided by a shared library, that would imply that others are supposed to load that shared library and also get that symbol -- but that symbol is supposed to be unique to the loaded foo.so module
| "_constraints": lambda: attrb.LabelList( | ||
| default = sorted({ | ||
| c: None | ||
| for info in PLATFORMS.values() |
There was a problem hiding this comment.
The PLATFORMs global is legacy and shouldn't be used. The main problem is it requires every platform to be defined in rules_python, but that doesn't scale.
|
|
||
| # 4. Create the underlying cc_shared_library | ||
| csl_name = "_" + name + "_csl" | ||
| csl_kwargs = {} |
There was a problem hiding this comment.
There's a small collection of attributes (visiblity, compatible_with, some others) that need to be copied into depended-upon generated targets. I think there's a helper function in utils for this; a todo for now is fine.
| files = depset([py_dso]), | ||
| runfiles = runfiles, | ||
| ), | ||
| PyInfo( |
There was a problem hiding this comment.
Also set the "has shared libraries" field (I forget its exact name)
| user_link_flags.append("-Wl,--allow-shlib-undefined") | ||
| import_path = repo_name | ||
| if ctx.label.package: | ||
| import_path = repo_name + "/" + ctx.label.package |
There was a problem hiding this comment.
Setting import path here is incorrect. It's going to add the directory the .so file is in to the python sys.path search, which makes it directly importable. However, this is wrong for two reasons:
- A foo.so file can be part of package. In whih case,
import pkg.foois how to import it, andimport foowould be incorrect - If it is part of package, all py files in the directory become directly importable as top-level modules. e.g
foo/bar.pybecomes importable asimport bar. This is incorrect, as it breaks the identify modules have, causes duplicate code loading, and can shadow other top-level imports.
|
|
||
| abi_tag = ctx.attr.abi_tag | ||
| if not abi_tag: | ||
| # Derive default: cpython-XX |
There was a problem hiding this comment.
This calculation is incomplete: it also needs to take into account freethreadedness.
You can copy how py_runtime computes it. I think it has an abiflags attribute? I can't remember exactly.
| kwargs["data"] = data | ||
|
|
||
| # 6. Wrap with py_extension_wrapper for PEP 3149 naming & PyInfo | ||
| py_extension_wrapper( |
There was a problem hiding this comment.
Since we now have an internal-only wrapper rule we can pass extra arbitrary internal-use-only args, I think we can simplify how the platform info is passed in: have os and arch attributes, then use select. This also makes it easy to map bazel's conditions to the necessary python value
os = select({
"@platforms//os:windows": "win",
"@platforms//os:osx": "darwin",
})
arch = select({
"@platforms//cpu:x86_64": ...,
...
})
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