GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Mark unrunnable test_threading. Fix SSL args names. Add args to sysconfigdata by youknowone · Pull Request #6470 · RustPython/RustPython

Conversation

youknowone commented Dec 23, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Summary by CodeRabbit

Refactor
- Streamlined SSL/TLS certificate handling method signatures for improved code organization.
Chores
- Enhanced system configuration data with compiler and native extension settings.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone added 2 commits

December 23, 2025 11:55


          mark test_threading

d919fe5


          fix ssl

73e1c38

coderabbitai Bot commented Dec 23, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)

Lib/test/test_os.py is excluded by !Lib/**

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This PR refactors certificate-related method signatures across SSL modules to use a shared GetCertArgs struct instead of individual optional parameters, standardizing argument handling. Additionally, compiler and linker environment variables are added to the sysconfigdata module's configuration dictionary.

Changes

Cohort / File(s)	Summary
SSL Certificate Arguments Refactoring `crates/stdlib/src/openssl.rs`, `crates/stdlib/src/ssl.rs`	Introduces `GetCertArgs` struct with `binary_form: OptionalArg<bool>` field. Updates `get_ca_certs()` and `getpeercert()` method signatures to accept `args: GetCertArgs` instead of individual `binary_form`/`binary` parameters. Internal logic adjusted to derive the boolean value from `args.binary_form`.
Build Configuration Variables `crates/vm/src/stdlib/sysconfigdata.rs`	Adds compiler and linker environment variables (CC, CXX, CFLAGS, CPPFLAGS, LDFLAGS, LDSHARED, CCSHARED, SHLIB_SUFFIX, SO, AR, ARFLAGS, OPT, BASECFLAGS, BLDSHARED) to the `build_time_vars` dictionary for native extension configuration.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

RustPython#6219: Modifies the same SSL certificate-related APIs in stdlib/src/ssl.rs (getpeercert/get_ca_certs signatures and certificate handling).

Suggested reviewers

coolreader18
ShaharNaveh
arihant2math

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the three main changes in the pull request: marking a test, fixing SSL argument names by refactoring to use GetCertArgs, and adding compiler configuration variables to sysconfigdata.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone changed the title ~~common fix for pip~~ common fix for ctypespip

Dec 23, 2025

youknowone changed the title ~~common fix for ctypespip~~ Mark unrunnable test_threading. Fix SSL args names. Add args to sysconfigdata

Dec 23, 2025


          fix sysconfigdata

a84452a

youknowone force-pushed the common-fix branch from 448281c to a84452a Compare

December 23, 2025 04:13

youknowone marked this pull request as ready for review

December 23, 2025 04:22

coderabbitai Bot reviewed

Dec 23, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 GetCertArgs struct. 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 from binary to binary_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

📥 Commits

Reviewing files that changed from the base of the PR and between 2342006 and a84452a.

⛔ Files ignored due to path filters (2)

Lib/test/test_sysconfig.py is excluded by !Lib/**
Lib/test/test_threading.py is excluded by !Lib/**

📒 Files selected for processing (3)

crates/stdlib/src/openssl.rs
crates/stdlib/src/ssl.rs
crates/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 running cargo fmt to 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.rs
crates/stdlib/src/ssl.rs
crates/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 GetCertArgs struct properly uses the FromArgs derive macro and correctly declares the binary_form field 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 GetCertArgs struct and extracts the binary_form value with an appropriate default of false.

crates/stdlib/src/ssl.rs (3)

844-848: LGTM! Clean refactoring to standardize certificate argument handling.

The GetCertArgs struct follows RustPython conventions and mirrors the implementation pattern in openssl.rs. This refactoring improves code consistency and makes the API more maintainable.

1697-1698: Signature refactoring maintains backward compatibility.

The updated signature using GetCertArgs preserves 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 getpeercert method correctly adopts the GetCertArgs pattern, maintaining backward compatibility and default behavior. This completes the standardization across both get_ca_certs and getpeercert methods.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/vm/src/stdlib/sysconfigdata.rs

Comment on lines +23 to +37

		// 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",

coderabbitai Bot Dec 23, 2025

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change

	// 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" },

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          skip spawnve on windows

df523cb

youknowone merged commit ec75888 into RustPython:main

Dec 23, 2025

13 checks passed

Uh oh!

There was an error while loading. Please reload this page.

youknowone deleted the common-fix branch

December 23, 2025 07:09

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

1 participant

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

youknowone commented Dec 23, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 23, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Dec 23, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.