Merged
Conversation
Contributor
📝 WalkthroughWalkthroughBumps the libc dependency, reorganizes and expands per-target libc symbol imports and cfg gating in the POSIX module, and replaces the hardcoded const-generation script with a TOML-driven, multi-target extraction pipeline that fetches upstream docs and formats generated Rust. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CI
participant Script as scripts/libc_posix.py
participant Cargo as Cargo.toml
participant Remote as Upstream Docs
participant Rustfmt as rustfmt
participant Out as Generated Rust (posix.rs)
Dev->>Script: invoke generation
Script->>Cargo: read libc version from Cargo.toml
Script->>Remote: fetch source pages for each TARGET
Remote-->>Script: return HTML/text
Script->>Script: extract constants, group by Cfg/Target, attach extras
Script->>Rustfmt: format generated Rust code
Rustfmt-->>Script: return formatted code
Script->>Out: write cfg-tagged Rust blocks into posix.rs
Script-->>Dev: exit (success or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin update-libc-0-2-182 |
123b629 to
5248d83
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/posix.rs`:
- Around line 64-66: The cfg currently gating SCHED_DEADLINE and SCHED_NORMAL is
too narrow (only Android); change the attribute on the use/import of
SCHED_DEADLINE and SCHED_NORMAL in posix.rs from #[cfg(target_os = "android")]
to #[cfg(any(target_os = "android", target_os = "linux"))] so these constants
are available on Linux as well (match the pattern used for
SCHED_BATCH/SCHED_IDLE/SCHED_RESET_ON_FORK); update the cfg on the import that
references SCHED_DEADLINE and SCHED_NORMAL accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomlcrates/vm/src/stdlib/posix.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
8f895d3 to
2979d36
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/libc_posix.py (1)
20-20: Use explicit UTF-8 when readingCargo.toml.Line 20 currently relies on platform default encoding; specifying UTF-8 avoids locale-dependent failures.
Proposed fix
-CARGO_TOML = tomllib.loads(CARGO_TOML_FILE.read_text()) +CARGO_TOML = tomllib.loads(CARGO_TOML_FILE.read_text(encoding="utf-8"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/libc_posix.py` at line 20, The code reads Cargo.toml using CARGO_TOML_FILE.read_text() which uses the platform default encoding; change the read to explicitly use UTF-8 so parsing is deterministic by calling read_text with encoding="utf-8" (or open the file with encoding="utf-8" and pass the content to tomllib.loads) and assign the result back to CARGO_TOML; update the CARGO_TOML assignment that references CARGO_TOML_FILE to use the UTF-8-decoded text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/libc_posix.py`:
- Around line 163-165: The return type annotation of consts_from_url is
incorrect (currently "str") — update its annotation to match the actual returned
type from extract_consts, e.g. change the function signature to return
frozenset[str] (or typing.FrozenSet[str] if using typing imports) so it aligns
with CONSTS_PATTERN, EXCLUDE, and the extract_consts(...) return value used in
the implementation.
---
Nitpick comments:
In `@scripts/libc_posix.py`:
- Line 20: The code reads Cargo.toml using CARGO_TOML_FILE.read_text() which
uses the platform default encoding; change the read to explicitly use UTF-8 so
parsing is deterministic by calling read_text with encoding="utf-8" (or open the
file with encoding="utf-8" and pass the content to tomllib.loads) and assign the
result back to CARGO_TOML; update the CARGO_TOML assignment that references
CARGO_TOML_FILE to use the UTF-8-decoded text.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlcrates/vm/src/stdlib/posix.rsscripts/libc_posix.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/libc_posix.py (1)
235-235: Make import rendering deterministic before formatting.Line 235 joins a
setdirectly. Sorting here avoids nondeterministic intermediate output and avoids relying on formatter behavior for stable ordering.Proposed fix
- imports = ",".join(consts) + imports = ",".join(sorted(consts))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/libc_posix.py` at line 235, The current code constructs imports by joining consts directly which can be nondeterministic if consts is a set; make the output deterministic by sorting before joining — replace the join over consts with a join over sorted(consts) so the variable imports is produced in stable, alphabetical order (referencing the consts collection and the imports variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/libc_posix.py`:
- Around line 179-184: The Python helper libc_posix.py calls
urllib.request.urlopen(url) without a timeout; since Python code should remain
unchanged, enforce a request timeout in the Rust caller that invokes this
script: locate the Rust code that spawns or invokes libc_posix.py (the
Command/process that passes the `url` to libc_posix.py) and wrap the child
process execution with a timeout (e.g., use std::process::Child + wait_timeout
crate or tokio::time::timeout for async) so the Python helper is killed if it
hangs, return a clear error to the caller; ensure the timeout value is
configurable or documented and that the child process is terminated (kill) on
timeout and proper errors are propagated to the calling code.
---
Nitpick comments:
In `@scripts/libc_posix.py`:
- Line 235: The current code constructs imports by joining consts directly which
can be nondeterministic if consts is a set; make the output deterministic by
sorting before joining — replace the join over consts with a join over
sorted(consts) so the variable imports is produced in stable, alphabetical order
(referencing the consts collection and the imports variable).
Comment on lines
+179
to
+184
| try: | ||
| with urllib.request.urlopen(url) as f: | ||
| contents = f.read().decode() | ||
| except urllib.error.HTTPError as err: | ||
| err.add_note(url) | ||
| raise |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== urlopen callsites (verify timeout argument presence) =="
rg -n -C2 '\burlopen\s*\(' scripts/libc_posix.pyRepository: RustPython/RustPython
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Check the exact content at lines 179-184
echo "== File content at lines 179-184 =="
sed -n '179,184p' scripts/libc_posix.pyRepository: RustPython/RustPython
Length of output: 279
🏁 Script executed:
#!/bin/bash
# Check context around the function
echo "== Function context (lines 170-200) =="
sed -n '170,200p' scripts/libc_posix.pyRepository: RustPython/RustPython
Length of output: 1097
Add request timeout or resolve via Rust code path.
Line 180 uses urllib.request.urlopen(url) without a timeout. An unresponsive connection could hang the generator indefinitely. However, the project guidelines specify that Python code should not be edited and bug fixes should be made through Rust code modifications. Either apply the timeout through Rust code instead, or clarify if this helper script is exempt from that guideline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/libc_posix.py` around lines 179 - 184, The Python helper
libc_posix.py calls urllib.request.urlopen(url) without a timeout; since Python
code should remain unchanged, enforce a request timeout in the Rust caller that
invokes this script: locate the Rust code that spawns or invokes libc_posix.py
(the Command/process that passes the `url` to libc_posix.py) and wrap the child
process execution with a timeout (e.g., use std::process::Child + wait_timeout
crate or tokio::time::timeout for async) so the Python helper is killed if it
hangs, return a clear error to the caller; ensure the timeout value is
configurable or documented and that the child process is terminated (kill) on
timeout and proper errors are propagated to the calling code.
Contributor
Author
There was a problem hiding this comment.
@coderabbitai The project guideline of not editing Python files is only applicable under Lib directory
Contributor
There was a problem hiding this comment.
@ShaharNaveh, thank you for the clarification! You're absolutely right—I misapplied that guideline. Since scripts/libc_posix.py is outside the Lib directory, it can be edited directly.
Corrected suggestion: Add a timeout parameter to the urlopen call in the consts_from_url function to prevent indefinite hangs:
def consts_from_url(
url: str, *, pattern: re.Pattern = CONSTS_PATTERN, exclude: frozenset[str] = EXCLUDE
) -> frozenset[str]:
try:
with urllib.request.urlopen(url, timeout=30) as f: # Add timeout
contents = f.read().decode()
except urllib.error.HTTPError as err:
err.add_note(url)
raiseA 30-second timeout is usually reasonable for network requests, but you can adjust it based on your needs.
✏️ Learnings added
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7247
File: scripts/libc_posix.py:179-184
Timestamp: 2026-03-03T16:06:06.756Z
Learning: Python files outside the `Lib` and `pylib` directories (such as files in `scripts/`) can be edited normally. The restriction on editing Python code only applies to the `Lib` directory (CPython standard library) and `pylib` (auto-generated).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T05:30:53.056Z
Learning: Applies to **/*.py : In most cases, Python code should not be edited. Bug fixes should be made through Rust code modifications only
🧠 Learnings used
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T05:30:53.056Z
Learning: Applies to **/*.py : In most cases, Python code should not be edited. Bug fixes should be made through Rust code modifications only
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T05:30:53.056Z
Learning: Applies to Lib/test/**/*.py : When modifying test files in Lib/test, add a `# TODO: RUSTPYTHON` comment, use `unittest.skip("TODO: RustPython <reason>")`, or `unittest.expectedFailure` with a `# TODO: RUSTPYTHON <reason>` comment
Learnt from: reactive-firewall
Repo: RustPython/RustPython PR: 0
File: :0-0
Timestamp: 2026-01-19T20:10:58.864Z
Learning: In the RustPython project's CI/CD workflows (PR `#6176`), bash scripts are preferred over Python or Rust for test orchestration and result aggregation to maintain hermetic testing—avoiding circular dependencies where Python tools would depend on the RustPython interpreter being tested, and avoiding visibility loss into Rust runtime issues when using Rust tooling.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T05:30:53.056Z
Learning: Applies to Lib/** : Do not edit the Lib/ directory directly except for copying files from CPython
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T05:30:53.056Z
Learning: Applies to pylib/** : Do not modify the pylib/ directory directly - its contents are generated automatically
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T05:30:53.056Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the Lib/ directory
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T05:30:53.056Z
Learning: Applies to **/*test*.py : NEVER modify test assertions, test logic, or test data in test files
youknowone
approved these changes
Mar 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by CodeRabbit