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
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