Add more build-info functions to c-api#8216
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPublicizes build metadata constants in ChangesBuild metadata C API exposure
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/capi/src/pylifecycle.rs`:
- Around line 84-115: The four getter functions are duplicated and should be
refactored into a macro-based helper. Extract the shared CString/LazyLock/as_ptr
logic into a single reusable macro or helper and parameterize only the differing
sys constant, static identifier, and panic text for Py_GetBuildInfo,
Py_GetCompiler, Py_GetCopyright, and Py_GetPlatform. Keep the public extern "C"
function names unchanged and apply the same pattern consistently if
Py_GetVersion or similar getters in pylifecycle.rs use the same structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d582c5db-c613-42ea-a2b7-5f7d60708967
📒 Files selected for processing (2)
crates/capi/src/pylifecycle.rscrates/vm/src/stdlib/sys.rs
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn Py_GetBuildInfo() -> *const c_char { | ||
| static BUILD_INFO: LazyLock<CString> = LazyLock::new(|| { | ||
| CString::new(sys::BUILD_INFO).expect("build info must not contain interior NULs") | ||
| }); | ||
| BUILD_INFO.as_ptr() | ||
| } | ||
|
|
||
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn Py_GetCompiler() -> *const c_char { | ||
| static COMPILER: LazyLock<CString> = LazyLock::new(|| { | ||
| CString::new(sys::COMPILER).expect("compiler must not contain interior NULs") | ||
| }); | ||
| COMPILER.as_ptr() | ||
| } | ||
|
|
||
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn Py_GetCopyright() -> *const c_char { | ||
| static COPYRIGHT: LazyLock<CString> = LazyLock::new(|| { | ||
| CString::new(sys::COPYRIGHT).expect("copyright must not contain interior NULs") | ||
| }); | ||
| COPYRIGHT.as_ptr() | ||
| } | ||
|
|
||
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn Py_GetPlatform() -> *const c_char { | ||
| static PLATFORM: LazyLock<CString> = LazyLock::new(|| { | ||
| CString::new(sys::PLATFORM).expect("platform must not contain interior NULs") | ||
| }); | ||
| PLATFORM.as_ptr() | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Deduplicate the four near-identical build-metadata functions with a macro.
Py_GetBuildInfo, Py_GetCompiler, Py_GetCopyright, and Py_GetPlatform differ only in the source constant, static name, and panic message — everything else is copy-pasted.
♻️ Proposed macro-based refactor
+macro_rules! build_metadata_getter {
+ ($fn_name:ident, $const_name:path, $what:literal) => {
+ #[unsafe(no_mangle)]
+ pub extern "C" fn $fn_name() -> *const c_char {
+ static VALUE: LazyLock<CString> = LazyLock::new(|| {
+ CString::new($const_name).expect(concat!($what, " must not contain interior NULs"))
+ });
+ VALUE.as_ptr()
+ }
+ };
+}
+
+build_metadata_getter!(Py_GetBuildInfo, sys::BUILD_INFO, "build info");
+build_metadata_getter!(Py_GetCompiler, sys::COMPILER, "compiler");
+build_metadata_getter!(Py_GetCopyright, sys::COPYRIGHT, "copyright");
+build_metadata_getter!(Py_GetPlatform, sys::PLATFORM, "platform");
-#[unsafe(no_mangle)]
-pub extern "C" fn Py_GetBuildInfo() -> *const c_char {
- static BUILD_INFO: LazyLock<CString> = LazyLock::new(|| {
- CString::new(sys::BUILD_INFO).expect("build info must not contain interior NULs")
- });
- BUILD_INFO.as_ptr()
-}
-
-#[unsafe(no_mangle)]
-pub extern "C" fn Py_GetCompiler() -> *const c_char {
- static COMPILER: LazyLock<CString> = LazyLock::new(|| {
- CString::new(sys::COMPILER).expect("compiler must not contain interior NULs")
- });
- COMPILER.as_ptr()
-}
-
-#[unsafe(no_mangle)]
-pub extern "C" fn Py_GetCopyright() -> *const c_char {
- static COPYRIGHT: LazyLock<CString> = LazyLock::new(|| {
- CString::new(sys::COPYRIGHT).expect("copyright must not contain interior NULs")
- });
- COPYRIGHT.as_ptr()
-}
-
-#[unsafe(no_mangle)]
-pub extern "C" fn Py_GetPlatform() -> *const c_char {
- static PLATFORM: LazyLock<CString> = LazyLock::new(|| {
- CString::new(sys::PLATFORM).expect("platform must not contain interior NULs")
- });
- PLATFORM.as_ptr()
-}As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
#!/bin/bash
# Check whether Py_GetVersion (pre-existing) follows the same repeated pattern,
# and whether existing pyo3-based tests cover similar getters.
ast-grep run --pattern 'pub extern "C" fn Py_GetVersion() -> *const c_char { $$$ }' --lang rust crates/capi/src/pylifecycle.rs
rg -n -A5 -B2 '"Py_Get' crates/capi/src/pylifecycle.rs
rg -n 'fn test' -A10 crates/capi/src/pylifecycle.rs | rg -n 'Py_Get' -B5🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/capi/src/pylifecycle.rs` around lines 84 - 115, The four getter
functions are duplicated and should be refactored into a macro-based helper.
Extract the shared CString/LazyLock/as_ptr logic into a single reusable macro or
helper and parameterize only the differing sys constant, static identifier, and
panic text for Py_GetBuildInfo, Py_GetCompiler, Py_GetCopyright, and
Py_GetPlatform. Keep the public extern "C" function names unchanged and apply
the same pattern consistently if Py_GetVersion or similar getters in
pylifecycle.rs use the same structure.
Source: Coding guidelines
44e1a4c to
c567388
Compare
| const API_VERSION: u32 = 0x0; // what C api? | ||
| #[pyattr(name = "copyright")] | ||
| const COPYRIGHT: &str = "Copyright (c) 2019 RustPython Team"; | ||
| pub const COPYRIGHT: &str = "Copyright (c) 2019 RustPython Team"; |
There was a problem hiding this comment.
hmm, another suggestion.
let's define this as
| pub const COPYRIGHT: &str = "Copyright (c) 2019 RustPython Team"; | |
| pub const COPYRIGHT: &CStr = c"Copyright (c) 2019 RustPython Team"; |
Then add a IntoPyObject conversion from &CStr to String to ignore the tail null char.
This will not hurt any RustPython use case and also will make C api happier without LazyLock
Summary by CodeRabbit
sysconstants, including build info and compiler, alongside updated visibility for existing version-related constants.