Mark unrunnable test_threading. Fix SSL args names. Add args to sysconfigdata #6470
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis PR refactors certificate-related method signatures across SSL modules to use a shared Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
448281c to
a84452a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/stdlib/src/openssl.rs (1)
2523-2528: LGTM: Consistent use of GetCertArgs struct.The method correctly adopts the new
GetCertArgsstruct. The extraction logic is sound and maintains the same default behavior.Optional: Consider naming consistency
For consistency with
get_ca_certs(line 1549), you could rename the local variable frombinarytobinary_form:fn getpeercert( &self, args: GetCertArgs, vm: &VirtualMachine, ) -> PyResult<Option<PyObjectRef>> { - let binary = args.binary_form.unwrap_or(false); + let binary_form = args.binary_form.unwrap_or(false);Then update the usage on line 2541 accordingly. This is a minor stylistic improvement and not required.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_sysconfig.pyis excluded by!Lib/**Lib/test/test_threading.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/stdlib/src/openssl.rscrates/stdlib/src/ssl.rscrates/vm/src/stdlib/sysconfigdata.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/stdlib/sysconfigdata.rscrates/stdlib/src/ssl.rscrates/stdlib/src/openssl.rs
🧠 Learnings (1)
📚 Learning: 2025-06-28T16:31:03.991Z
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
crates/vm/src/stdlib/sysconfigdata.rs
🧬 Code graph analysis (1)
crates/stdlib/src/ssl.rs (1)
crates/stdlib/src/openssl.rs (1)
get_ca_certs(1544-1568)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
🔇 Additional comments (6)
crates/stdlib/src/openssl.rs (3)
1-1: LGTM: Spell-checker directive updated appropriately.The spell-checker directive correctly includes the new compiler/linker variable names that are referenced in the codebase.
2262-2266: LGTM: Well-structured argument encapsulation.The
GetCertArgsstruct properly uses theFromArgsderive macro and correctly declares thebinary_formfield as an optional argument. This standardizes argument handling across certificate methods.
1544-1549: LGTM: Clean refactoring to use shared argument struct.The method correctly uses the new
GetCertArgsstruct and extracts thebinary_formvalue with an appropriate default offalse.crates/stdlib/src/ssl.rs (3)
844-848: LGTM! Clean refactoring to standardize certificate argument handling.The
GetCertArgsstruct follows RustPython conventions and mirrors the implementation pattern inopenssl.rs. This refactoring improves code consistency and makes the API more maintainable.
1697-1698: Signature refactoring maintains backward compatibility.The updated signature using
GetCertArgspreserves the same API surface and default behavior. The argument handling is consistent with the openssl module implementation.
3447-3452: LGTM! Consistent with the certificate argument refactoring.The
getpeercertmethod correctly adopts theGetCertArgspattern, maintaining backward compatibility and default behavior. This completes the standardization across bothget_ca_certsandgetpeercertmethods.
| // Compiler configuration for native extension builds | ||
| "CC" => "cc", | ||
| "CXX" => "c++", | ||
| "CFLAGS" => "", | ||
| "CPPFLAGS" => "", | ||
| "LDFLAGS" => "", | ||
| "LDSHARED" => "cc -shared", | ||
| "CCSHARED" => "", | ||
| "SHLIB_SUFFIX" => ".so", | ||
| "SO" => ".so", | ||
| "AR" => "ar", | ||
| "ARFLAGS" => "rcs", | ||
| "OPT" => "", | ||
| "BASECFLAGS" => "", | ||
| "BLDSHARED" => "cc -shared", |
There was a problem hiding this comment.
Platform-specific hardcoded values will be incorrect on Windows.
The hardcoded values for SHLIB_SUFFIX, SO, LDSHARED, and BLDSHARED assume a Unix/Linux environment. On Windows, shared libraries use .dll instead of .so, and the linker commands differ significantly.
🔎 Consider using conditional compilation for platform-specific values
// Compiler configuration for native extension builds
"CC" => "cc",
"CXX" => "c++",
"CFLAGS" => "",
"CPPFLAGS" => "",
"LDFLAGS" => "",
- "LDSHARED" => "cc -shared",
+ "LDSHARED" => if cfg!(windows) { "link.exe" } else { "cc -shared" },
"CCSHARED" => "",
- "SHLIB_SUFFIX" => ".so",
- "SO" => ".so",
+ "SHLIB_SUFFIX" => if cfg!(windows) { ".dll" } else if cfg!(target_os = "macos") { ".dylib" } else { ".so" },
+ "SO" => if cfg!(windows) { ".pyd" } else { ".so" },
"AR" => "ar",
"ARFLAGS" => "rcs",
"OPT" => "",
"BASECFLAGS" => "",
- "BLDSHARED" => "cc -shared",
+ "BLDSHARED" => if cfg!(windows) { "link.exe" } else { "cc -shared" },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Compiler configuration for native extension builds | |
| "CC" => "cc", | |
| "CXX" => "c++", | |
| "CFLAGS" => "", | |
| "CPPFLAGS" => "", | |
| "LDFLAGS" => "", | |
| "LDSHARED" => "cc -shared", | |
| "CCSHARED" => "", | |
| "SHLIB_SUFFIX" => ".so", | |
| "SO" => ".so", | |
| "AR" => "ar", | |
| "ARFLAGS" => "rcs", | |
| "OPT" => "", | |
| "BASECFLAGS" => "", | |
| "BLDSHARED" => "cc -shared", | |
| // Compiler configuration for native extension builds | |
| "CC" => "cc", | |
| "CXX" => "c++", | |
| "CFLAGS" => "", | |
| "CPPFLAGS" => "", | |
| "LDFLAGS" => "", | |
| "LDSHARED" => if cfg!(windows) { "link.exe" } else { "cc -shared" }, | |
| "CCSHARED" => "", | |
| "SHLIB_SUFFIX" => if cfg!(windows) { ".dll" } else if cfg!(target_os = "macos") { ".dylib" } else { ".so" }, | |
| "SO" => if cfg!(windows) { ".pyd" } else { ".so" }, | |
| "AR" => "ar", | |
| "ARFLAGS" => "rcs", | |
| "OPT" => "", | |
| "BASECFLAGS" => "", | |
| "BLDSHARED" => if cfg!(windows) { "link.exe" } else { "cc -shared" }, |
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.