Attempt to automate posix consts - #6117
Conversation
Sorry, something went wrong.
WalkthroughAdds a new codegen script (scripts/libc_posix.py) that fetches and groups libc constants per OS and emits cfg-gated import blocks. Reworks vm/src/stdlib/posix.rs to reorganize OS-gated libc imports, exposing many new constants and removing several scheduling-related ones. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Script as scripts/libc_posix.py
participant Remote as libc-test (github.com/rust-lang/libc-test)
participant Formatter as format_groups
participant Output as Stdout
Dev->>Script: run script (main)
Script->>Remote: fetch wanted consts (OS docs) using OS_CONSTS_PAT
Script->>Remote: fetch per-OS constant lists (LIBC_VERSION, TARGET_OS)
Script->>Script: parse constants via CONSTS_PAT, apply EXCLUDE/RENAMES/EXTRAS
Script->>Script: build const -> OS-set map
Script->>Formatter: group by OS-set and format cfg-gated blocks
Formatter-->>Output: emit `#[cfg(...)]` + `use libc::{...}` + `#[pyattr]`
Output-->>Dev: printed code blocks for posix.rs consumption
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem✨ Finishing Touches
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 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type @coderabbitai help to get the list of available commands. Other keywords and placeholders
Status, Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)vm/src/stdlib/posix.rs (4)1646-1653: Won’t compile: libc::c_short::MAX does not exist; fix overflow check and cast
libc::c_short is an alias (i16); it has no MAX. Use i16::MAX and cast safely.
Apply this diff:
- if flags > libc::c_short::MAX as i32 { + if flags > i16::MAX as i32 || flags < i16::MIN as i32 { return Err(vm.new_value_error("Too many flags set for posix_spawn")); } - let ret = - unsafe { libc::posix_spawnattr_setflags(&mut attrp, flags as libc::c_short) }; + let ret = unsafe { + libc::posix_spawnattr_setflags(&mut attrp, flags as libc::c_short) + };
1517-1567: Leak: posix_spawn_file_actions_init resources are never destroyed on error paths
If any add* call fails, we return without calling posix_spawn_file_actions_destroy, leaking resources.
Apply this diff inside the error branch:
- if ret != 0 { - let err = std::io::Error::from_raw_os_error(ret); - return Err(IOErrorBuilder::with_filename(&err, self.path, vm)); - } + if ret != 0 { + let err = std::io::Error::from_raw_os_error(ret); + unsafe { libc::posix_spawn_file_actions_destroy(&mut file_actions) }; + return Err(IOErrorBuilder::with_filename(&err, self.path, vm)); + }
1570-1656: Leak: posix_spawnattr_init/file_actions not destroyed on all exit paths
Both posix_spawnattr_init and posix_spawn_file_actions_init require corresponding destroy calls. Add them for all paths after attrp is initialized, including intermediate failures.
Apply these diffs for the two intermediate error cases:
- if ret != 0 { - return Err(vm.new_os_error(format!("posix_spawnattr_setpgroup failed: {ret}"))); - } + if ret != 0 { + unsafe { + libc::posix_spawn_file_actions_destroy(&mut file_actions); + libc::posix_spawnattr_destroy(&mut attrp); + } + return Err(vm.new_os_error(format!("posix_spawnattr_setpgroup failed: {ret}"))); + }- if ret != 0 { - return Err( - vm.new_os_error(format!("posix_spawnattr_setsigmask failed: {ret}")) - ); - } + if ret != 0 { + unsafe { + libc::posix_spawn_file_actions_destroy(&mut file_actions); + libc::posix_spawnattr_destroy(&mut attrp); + } + return Err( + vm.new_os_error(format!("posix_spawnattr_setsigmask failed: {ret}")) + ); + }And ensure both are destroyed on the final return (success or error), see next comment.
1701-1706: Always destroy posix_spawn resources before returning
Ensure no leaks on success and error returns.
Apply this diff:
- if ret == 0 { - Ok(pid) - } else { - let err = std::io::Error::from_raw_os_error(ret); - Err(IOErrorBuilder::with_filename(&err, self.path, vm)) - } + if ret == 0 { + unsafe { + libc::posix_spawn_file_actions_destroy(&mut file_actions); + libc::posix_spawnattr_destroy(&mut attrp); + } + Ok(pid) + } else { + let err = std::io::Error::from_raw_os_error(ret); + unsafe { + libc::posix_spawn_file_actions_destroy(&mut file_actions); + libc::posix_spawnattr_destroy(&mut attrp); + } + Err(IOErrorBuilder::with_filename(&err, self.path, vm)) + }
scripts/libc_posix.py (4)📜 Review detailsvm/src/stdlib/posix.rs (1)11-13: Tighten regex to avoid capturing functions like os.WIFEXITED(...)
Use a negative lookahead to skip tokens immediately followed by '(' to reduce reliance on EXCLUDE for function-like names.
Apply this diff:
-OS_CONSTS_PAT = re.compile( - r"\bos\.(_*[A-Z]+(?:_+[A-Z]+)*_*)" -) # TODO: Exclude matches if they have `(` after (those are functions) +OS_CONSTS_PAT = re.compile( + r"\bos\.(_*[A-Z]+(?:_+[A-Z]+)*_*)(?!\s*\()" +) # Avoid function-like names: os.FOO(...)
68-73: Add timeouts and error handling to network fetches
urlopen without a timeout can hang CI; errors aren’t handled. Add a small timeout and surface helpful messages.
Apply this diff and the import below:
-def get_consts(url: str, pattern: re.Pattern = CONSTS_PAT) -> frozenset[str]: - with urllib.request.urlopen(url) as f: - resp = f.read().decode() +def get_consts(url: str, pattern: re.Pattern = CONSTS_PAT) -> frozenset[str]: + try: + with urllib.request.urlopen(url, timeout=15) as f: + resp = f.read().decode() + except Exception as e: + raise RuntimeError(f"failed to fetch {url}: {e}") from eAlso add the import (either is fine; generic Exception above keeps it simple):
-import urllib.request +import urllib.request +import urllib.error
16-16: Avoid hard-coding libc version; derive or parameterize
Pinning LIBC_VERSION risks drift vs. Cargo.lock. Read it from env/CLI, falling back to the constant.
Apply this diff:
-LIBC_VERSION = "0.2.175" +import os +LIBC_VERSION = os.environ.get("LIBC_VERSION", "0.2.175")If helpful, I can wire a tiny argparse to allow --libc-version too.
94-97: Target CPython source rather than rendered docs for stability
Scraping docs is brittle across versions. Prefer parsing CPython’s posixmodule.c (or a pinned commit of os.rst) so the generator aligns with the project’s targeted CPython version.
Do you want me to adjust the script to accept a CPython ref (tag/commit) and pull constants from posixmodule.c?
1605-1616: Avoid hard-coding POSIX_SPAWN_SETSID magic number
If available, prefer libc::POSIX_SPAWN_SETSID to 0x0080. If not defined, gate a local const behind #[cfg(target_os = "linux")].
Apply this diff:
- #[cfg(target_os = "linux")] - { - flags |= 0x0080; // POSIX_SPAWN_SETSID value on Linux - } + #[cfg(target_os = "linux")] + { + #[allow(non_upper_case_globals)] + const POSIX_SPAWN_SETSID_LINUX: i32 = { + #[cfg(any())] { libc::POSIX_SPAWN_SETSID } // force compile-time fallback + #[cfg(not(any()))] { 0x0080 } + }; + flags |= POSIX_SPAWN_SETSID_LINUX; + }If libc::POSIX_SPAWN_SETSID exists on your minimum supported libc, you can simplify to that directly.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 CommitsReviewing files that changed from the base of the PR and between 7509387 and f66367e.
📒 Files selected for processing (2)- scripts/libc_posix.py (1 hunks)
- vm/src/stdlib/posix.rs (1 hunks)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Follow PEP 8 style for custom Python code
Use ruff for linting Python code
Files:
- scripts/libc_posix.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmt to format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
- vm/src/stdlib/posix.rs
- 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 on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
vm/src/stdlib/posix.rs (2)50-56: Nice: cleanly grouped android-only base constants
The android-gated set looks consistent and mirrors CPython exposure.
136-138: Verify SEEK_DATA/SEEK_HOLE availability on all targets
vm/src/stdlib/posix.rs:136–138
SEEK_DATA and SEEK_HOLE aren’t guaranteed in Android or DragonFly’s libc—run per-target cargo check (e.g. aarch64-linux-android, x86_64-unknown-dragonfly) to confirm these imports build, and add #[cfg] guards if any fail.
Sorry, something went wrong.
| continue | ||
|
|
||
| group_consts[target_oses].add(const) | ||
| group_consts = {grp: v | EXTRAS.get(grp, set()) for grp, v in group_consts.items()} |
There was a problem hiding this comment.
⚠️ Potential issue
EXTRAS may be silently dropped when their group doesn’t already exist
EXTRAS are only merged into already-existing groups. If a group like {"macos"} has no other members, its extras (e.g., COPYFILE_DATA) won’t be emitted. Ensure extras create their group.
Apply this diff:
- group_consts = {grp: v | EXTRAS.get(grp, set()) for grp, v in group_consts.items()}
+ # Merge extras and ensure their groups exist even if empty
+ for grp, extras in EXTRAS.items():
+ group_consts.setdefault(grp, set()).update(extras)In scripts/libc_posix.py around line 112, EXTRAS are only merged into groups that already exist so groups present only in EXTRAS will be dropped; modify the merge to ensure every key in EXTRAS is present by taking the union of group_consts.keys() and EXTRAS.keys() and then for each group produce the union of existing values and EXTRAS[group] (or use group_consts.get(group, set()) | EXTRAS.get(group, set())), so extras create their group instead of being silently ignored.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)vm/src/stdlib/posix.rs (3)1519-1523: Don’t use asserts for posix_spawn initializers; return proper OSErrors
Asserts will abort instead of surfacing OSError to Python. Handle init failures explicitly.
Apply:
let mut file_actions = unsafe { let mut fa = std::mem::MaybeUninit::uninit(); - assert!(libc::posix_spawn_file_actions_init(fa.as_mut_ptr()) == 0); + let r = libc::posix_spawn_file_actions_init(fa.as_mut_ptr()); + if r != 0 { + return Err(vm.new_os_error(format!("posix_spawn_file_actions_init failed: {r}"))); + } fa.assume_init() };let mut attrp = unsafe { let mut sa = std::mem::MaybeUninit::uninit(); - assert!(libc::posix_spawnattr_init(sa.as_mut_ptr()) == 0); + let r = libc::posix_spawnattr_init(sa.as_mut_ptr()); + if r != 0 { + // cleanup file_actions before returning + libc::posix_spawn_file_actions_destroy(&mut file_actions); + return Err(vm.new_os_error(format!("posix_spawnattr_init failed: {r}"))); + } sa.assume_init() };Also applies to: 1572-1576
1565-1569: Leak fix: always destroy posix_spawn file_actions/attr on all exit paths
posix_spawn_file_actions_init/posix_spawnattr_init allocate resources that must be destroyed. Several early returns skip destruction.
Apply:
- if ret != 0 { - let err = std::io::Error::from_raw_os_error(ret); - return Err(IOErrorBuilder::with_filename(&err, self.path, vm)); - } + if ret != 0 { + let err = std::io::Error::from_raw_os_error(ret); + unsafe { libc::posix_spawn_file_actions_destroy(&mut file_actions); } + return Err(IOErrorBuilder::with_filename(&err, self.path, vm)); + }- if ret != 0 { - return Err(vm.new_os_error(format!("posix_spawnattr_setflags failed: {ret}"))); - } + if ret != 0 { + unsafe { + libc::posix_spawn_file_actions_destroy(&mut file_actions); + libc::posix_spawnattr_destroy(&mut attrp); + } + return Err(vm.new_os_error(format!("posix_spawnattr_setflags failed: {ret}"))); + }- if ret == 0 { - Ok(pid) - } else { - let err = std::io::Error::from_raw_os_error(ret); - Err(IOErrorBuilder::with_filename(&err, self.path, vm)) - } + unsafe { + libc::posix_spawn_file_actions_destroy(&mut file_actions); + libc::posix_spawnattr_destroy(&mut attrp); + } + if ret == 0 { + Ok(pid) + } else { + let err = std::io::Error::from_raw_os_error(ret); + Err(IOErrorBuilder::with_filename(&err, self.path, vm)) + }Optional: wrap these in small RAII types with Drop to make this bulletproof and simplify control flow. I can draft that if you want.
Also applies to: 1653-1657, 1703-1709
1577-1591: Set POSIX_SPAWN_SETSIGDEF when setting the default signal set
Without the flag, the defaulted signal set may be ignored.
Apply:
assert!( unsafe { libc::posix_spawnattr_setsigdefault(&mut attrp, set.as_ref()) } == 0 ); + flags |= libc::POSIX_SPAWN_SETSIGDEF;
vm/src/stdlib/posix.rs (2)📜 Review details97-100: Verify NetBSD XATTR_*; likely Linux/macOS only
NetBSD generally uses extattr(2) namespaces rather than XATTR_* macros. This import may not exist and could break builds.
Proposed tightening (if confirmed):
-#[cfg(any(target_os = "linux", target_os = "macos", target_os = "netbsd"))] +#[cfg(any(target_os = "linux", target_os = "macos"))] #[pyattr] use libc::{XATTR_CREATE, XATTR_REPLACE};
128-137: Android gating for SEEK_DATA/SEEK_HOLE may be risky
SEEK_{DATA,HOLE} are not consistently available on Android bionic headers. Consider dropping Android from this block unless you’ve verified libc exposes them on your min API level.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 CommitsReviewing files that changed from the base of the PR and between f66367e and e9594c3.
📒 Files selected for processing (2)- scripts/libc_posix.py (1 hunks)
- vm/src/stdlib/posix.rs (1 hunks)
- scripts/libc_posix.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmt to format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
- vm/src/stdlib/posix.rs
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
vm/src/stdlib/posix.rs (2)101-109: Double-check GRND_ gating*
GRND_RANDOM/NONBLOCK are certainly on Linux/Android; availability on DragonFly/NetBSD is not universal and may not be exposed by libc.
Would you like me to check libc’s per-target bindings and BSD manpages for these flags and propose exact cfgs?
1607-1619: Use libc::POSIX_SPAWN_SETSID instead of magic number
Confirm that your libc crate version defines POSIX_SPAWN_SETSID (requires glibc ≥ 2.26 with _GNU_SOURCE); if it’s missing, bump libc or gate out/implement a fallback. (spinics.net)
Sorry, something went wrong.
| #[cfg(target_os = "freebsd")] | ||
| #[pyattr] | ||
| use libc::{MFD_HUGE_MASK, SF_MNOWAIT, SF_NOCACHE, SF_NODISKIO, SF_SYNC}; | ||
|
|
There was a problem hiding this comment.
⚠️ Potential issue
Fix FreeBSD import: MFD_HUGE_MASK is Linux-only
MFD_HUGE_MASK isn’t available on FreeBSD (it’s a Linux memfd hugepage flag) and will fail to compile under that target.
Apply:
-#[cfg(target_os = "freebsd")]
-#[pyattr]
-use libc::{MFD_HUGE_MASK, SF_MNOWAIT, SF_NOCACHE, SF_NODISKIO, SF_SYNC};
+#[cfg(target_os = "freebsd")]
+#[pyattr]
+use libc::{SF_MNOWAIT, SF_NOCACHE, SF_NODISKIO, SF_SYNC};‼️ 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.
| #[cfg(target_os = "freebsd")] | |
| #[pyattr] | |
| use libc::{MFD_HUGE_MASK, SF_MNOWAIT, SF_NOCACHE, SF_NODISKIO, SF_SYNC}; | |
| #[cfg(target_os = "freebsd")] | |
| #[pyattr] | |
| use libc::{SF_MNOWAIT, SF_NOCACHE, SF_NODISKIO, SF_SYNC}; |
Sorry, something went wrong.
| #[cfg(any(target_os = "android", target_os = "freebsd", target_os = "linux"))] | ||
| #[pyattr] | ||
| use libc::{ | ||
| MFD_ALLOW_SEALING, MFD_CLOEXEC, MFD_HUGETLB, POSIX_FADV_DONTNEED, POSIX_FADV_NOREUSE, | ||
| POSIX_FADV_NORMAL, POSIX_FADV_RANDOM, POSIX_FADV_SEQUENTIAL, POSIX_FADV_WILLNEED, | ||
| }; | ||
|
|
There was a problem hiding this comment.
⚠️ Potential issue
Split memfd flags from POSIX_FADV_*; avoid FreeBSD/Android build breaks
- memfd flags (MFD_*) are Linux/Android-only; FreeBSD doesn’t define MFD_HUGETLB/ALLOW_SEALING.
- The POSIX_FADV_* advice values are fine on these Unixes.
Apply:
-#[cfg(any(target_os = "android", target_os = "freebsd", target_os = "linux"))]
-#[pyattr]
-use libc::{
- MFD_ALLOW_SEALING, MFD_CLOEXEC, MFD_HUGETLB, POSIX_FADV_DONTNEED, POSIX_FADV_NOREUSE,
- POSIX_FADV_NORMAL, POSIX_FADV_RANDOM, POSIX_FADV_SEQUENTIAL, POSIX_FADV_WILLNEED,
-};
+// memfd_* flags: Linux/Android only
+#[cfg(any(target_os = "android", target_os = "linux"))]
+#[pyattr]
+use libc::{MFD_ALLOW_SEALING, MFD_CLOEXEC, MFD_HUGETLB};
+
+// posix_fadvise advice values
+#[cfg(any(target_os = "android", target_os = "freebsd", target_os = "linux"))]
+#[pyattr]
+use libc::{
+ POSIX_FADV_DONTNEED, POSIX_FADV_NOREUSE, POSIX_FADV_NORMAL, POSIX_FADV_RANDOM,
+ POSIX_FADV_SEQUENTIAL, POSIX_FADV_WILLNEED,
+};In vm/src/stdlib/posix.rs around lines 86 to 92, the import groups mix
Linux/Android-only MFD_* constants with POSIX_FADV_* constants which exist on
FreeBSD too; split the imports so MFD_ALLOW_SEALING, MFD_CLOEXEC and MFD_HUGETLB
are imported only under #[cfg(any(target_os = "android", target_os = "linux"))]
(or target_os = "linux" if Android handled separately), while
POSIX_FADV_DONTNEED, POSIX_FADV_NOREUSE, POSIX_FADV_NORMAL, POSIX_FADV_RANDOM,
POSIX_FADV_SEQUENTIAL and POSIX_FADV_WILLNEED remain under #[cfg(any(target_os =
"android", target_os = "freebsd", target_os = "linux"))]; update the use libc::{
... } lines accordingly so FreeBSD builds do not reference undefined MFD_*
symbols.
Sorry, something went wrong.
| #[cfg(any(target_os = "android", target_os = "linux", target_os = "netbsd"))] | ||
| #[pyattr] | ||
| use libc::{TFD_CLOEXEC, TFD_NONBLOCK, TFD_TIMER_ABSTIME, TFD_TIMER_CANCEL_ON_SET}; | ||
|
|
There was a problem hiding this comment.
⚠️ Potential issue
Restrict timerfd constants to Linux/Android
TFD_* are Linux-only (and present on Android); NetBSD doesn’t provide timerfd.
Apply:
-#[cfg(any(target_os = "android", target_os = "linux", target_os = "netbsd"))]
+#[cfg(any(target_os = "android", target_os = "linux"))]
#[pyattr]
use libc::{TFD_CLOEXEC, TFD_NONBLOCK, TFD_TIMER_ABSTIME, TFD_TIMER_CANCEL_ON_SET};‼️ 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.
| #[cfg(any(target_os = "android", target_os = "linux", target_os = "netbsd"))] | |
| #[pyattr] | |
| use libc::{TFD_CLOEXEC, TFD_NONBLOCK, TFD_TIMER_ABSTIME, TFD_TIMER_CANCEL_ON_SET}; | |
| #[cfg(any(target_os = "android", target_os = "linux"))] | |
| #[pyattr] | |
| use libc::{TFD_CLOEXEC, TFD_NONBLOCK, TFD_TIMER_ABSTIME, TFD_TIMER_CANCEL_ON_SET}; |
In vm/src/stdlib/posix.rs around lines 93 to 96, the cfg currently includes
netbsd but the TFD_* timerfd constants are only available on Linux and Android;
remove target_os = "netbsd" from the cfg so it reads cfg(any(target_os =
"android", target_os = "linux")) and keep the #[pyattr] use libc::{TFD_CLOEXEC,
TFD_NONBLOCK, TFD_TIMER_ABSTIME, TFD_TIMER_CANCEL_ON_SET}; unchanged.
Sorry, something went wrong.
| #[cfg(any( | ||
| target_os = "android", | ||
| target_os = "freebsd", | ||
| target_os = "linux", | ||
| target_os = "redox" | ||
| ))] | ||
| #[pyattr] | ||
| use libc::O_PATH; | ||
|
|
There was a problem hiding this comment.
⚠️ Potential issue
O_PATH is Linux-specific
FreeBSD/Redox don’t expose O_PATH like Linux; this will likely fail to compile on those targets.
Apply:
-#[cfg(any(
- target_os = "android",
- target_os = "freebsd",
- target_os = "linux",
- target_os = "redox"
-))]
+#[cfg(any(target_os = "android", target_os = "linux"))]
#[pyattr]
use libc::O_PATH;‼️ 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.
| #[cfg(any( | |
| target_os = "android", | |
| target_os = "freebsd", | |
| target_os = "linux", | |
| target_os = "redox" | |
| ))] | |
| #[pyattr] | |
| use libc::O_PATH; | |
| #[cfg(any(target_os = "android", target_os = "linux"))] | |
| #[pyattr] | |
| use libc::O_PATH; |
In vm/src/stdlib/posix.rs around lines 110 to 118, the current cfg block exposes libc::O_PATH for multiple OSes although O_PATH is Linux-specific; restrict the import to Linux only by changing the cfg to target_os = "linux" (or add a cfg-if that imports O_PATH only when target_os == "linux") and remove it from freebsd/redox/android targets so the code compiles on those platforms without referencing a non-existent symbol.
Sorry, something went wrong.
| #[cfg(any( | ||
| target_os = "android", | ||
| target_os = "linux", | ||
| target_os = "netbsd", | ||
| target_os = "openbsd" | ||
| ))] | ||
| #[pyattr] | ||
| use libc::O_RSYNC; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusiveTighten O_RSYNC gating
O_RSYNC is a Linux-ism (alias of O_SYNC). It’s not generally available on *BSDs and may not be exposed by libc there.
Tentative change:
-#[cfg(any(
- target_os = "android",
- target_os = "linux",
- target_os = "netbsd",
- target_os = "openbsd"
-))]
-#[pyattr]
-use libc::O_RSYNC;
+#[cfg(target_os = "linux")]
+#[pyattr]
+use libc::O_RSYNC;If Android’s bionic exposes O_RSYNC in your minimum API level, we can include it after verification.
Restrict O_RSYNC import to Linux
O_RSYNC is only defined in libc for Linux (added under “Solarish” in libc 0.2.160) and isn’t exposed on NetBSD or OpenBSD (github.com). Change the cfg to target Linux only; if Android’s bionic proves to define O_RSYNC on your minimum API level, you can re-add it then.
-#[cfg(any(
- target_os = "android",
- target_os = "linux",
- target_os = "netbsd",
- target_os = "openbsd"
-))]
-#[pyattr]
-use libc::O_RSYNC;
+#[cfg(target_os = "linux")]
+#[pyattr]
+use libc::O_RSYNC;‼️ 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.
| #[cfg(any( | |
| target_os = "android", | |
| target_os = "linux", | |
| target_os = "netbsd", | |
| target_os = "openbsd" | |
| ))] | |
| #[pyattr] | |
| use libc::O_RSYNC; | |
| #[cfg(target_os = "linux")] | |
| #[pyattr] | |
| use libc::O_RSYNC; |
In vm/src/stdlib/posix.rs around lines 119 to 127, the cfg currently enables importing libc::O_RSYNC for Android, Linux, NetBSD, and OpenBSD although O_RSYNC is defined only on Linux; update the cfg to target_os = "linux" only (remove Android, NetBSD and OpenBSD) so the use libc::O_RSYNC is compiled only on Linux, and add a comment noting you can re-enable Android if/when bionic on your minimum API level exposes O_RSYNC.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)vm/src/stdlib/posix.rs (5)🧹 Nitpick comments (3)50-53: Fix FreeBSD import: drop Linux-only MFD_HUGE_MASK
MFD_HUGE_MASK isn’t defined on FreeBSD and will break that build. Keep only the SF_* flags here.
-#[cfg(target_os = "freebsd")] -#[pyattr] -use libc::{MFD_HUGE_MASK, SF_MNOWAIT, SF_NOCACHE, SF_NODISKIO, SF_SYNC}; +#[cfg(target_os = "freebsd")] +#[pyattr] +use libc::{SF_MNOWAIT, SF_NOCACHE, SF_NODISKIO, SF_SYNC};
87-93: Split memfd flags from POSIX_FADV_*; avoid FreeBSD/Android build breaks
MFD_* are Linux/Android-only. POSIX_FADV_* are also available on FreeBSD. Split the groups.
-#[cfg(any(target_os = "android", target_os = "freebsd", target_os = "linux"))] -#[pyattr] -use libc::{ - MFD_ALLOW_SEALING, MFD_CLOEXEC, MFD_HUGETLB, POSIX_FADV_DONTNEED, POSIX_FADV_NOREUSE, - POSIX_FADV_NORMAL, POSIX_FADV_RANDOM, POSIX_FADV_SEQUENTIAL, POSIX_FADV_WILLNEED, -}; +// memfd_* flags: Linux/Android only +#[cfg(any(target_os = "android", target_os = "linux"))] +#[pyattr] +use libc::{MFD_ALLOW_SEALING, MFD_CLOEXEC, MFD_HUGETLB}; + +// posix_fadvise advice values (Android/FreeBSD/Linux) +#[cfg(any(target_os = "android", target_os = "freebsd", target_os = "linux"))] +#[pyattr] +use libc::{ + POSIX_FADV_DONTNEED, POSIX_FADV_NOREUSE, POSIX_FADV_NORMAL, POSIX_FADV_RANDOM, + POSIX_FADV_SEQUENTIAL, POSIX_FADV_WILLNEED, +};
94-97: Restrict TFD_ (timerfd) to Linux/Android*
NetBSD doesn’t provide timerfd.
-#[cfg(any(target_os = "android", target_os = "linux", target_os = "netbsd"))] +#[cfg(any(target_os = "android", target_os = "linux"))] #[pyattr] use libc::{TFD_CLOEXEC, TFD_NONBLOCK, TFD_TIMER_ABSTIME, TFD_TIMER_CANCEL_ON_SET};
111-118: O_PATH is Linux-specific (Android may also expose); remove FreeBSD/Redox
This will fail on FreeBSD/Redox.
-#[cfg(any( - target_os = "android", - target_os = "freebsd", - target_os = "linux", - target_os = "redox" -))] +#[cfg(any(target_os = "android", target_os = "linux"))] #[pyattr] use libc::O_PATH;
120-127: Restrict O_RSYNC to Linux
O_RSYNC isn’t generally exposed on BSDs; Android’s support is unclear.
-#[cfg(any( - target_os = "android", - target_os = "linux", - target_os = "netbsd", - target_os = "openbsd" -))] -#[pyattr] -use libc::O_RSYNC; +#[cfg(target_os = "linux")] +#[pyattr] +use libc::O_RSYNC;
vm/src/stdlib/posix.rs (3)📜 Review details75-82: Nit: simplify cfg to unix
any(target_os = "android", unix) is redundant; Android is already unix.
-#[cfg(any(target_os = "android", unix))] +#[cfg(unix)] #[pyattr] use libc::{ F_OK, O_CLOEXEC, O_DIRECTORY, O_NOFOLLOW, O_NONBLOCK, PRIO_PGRP, PRIO_PROCESS, PRIO_USER, R_OK, RTLD_GLOBAL, RTLD_LAZY, RTLD_LOCAL, RTLD_NOW, W_OK, WCONTINUED, WNOHANG, WUNTRACED, X_OK, };
193-208: Verify CLD_ and F_LOCK family coverage; consider dropping redox*
Some CLD_* and F_LOCK/TLOCK/ULOCK may not exist on Redox; broad gating risks build breaks.
-#[cfg(any( +#[cfg(any( target_os = "android", target_os = "dragonfly", target_os = "freebsd", target_os = "linux", target_os = "macos", target_os = "netbsd", - target_os = "openbsd", - target_os = "redox" + target_os = "openbsd" ))]
209-221: Verify O_ASYNC/WEXITED/WNOWAIT/WSTOPPED on Redox
These waitid/async constants may be missing on Redox; suggest excluding if CI fails.
-#[cfg(any( +#[cfg(any( target_os = "android", target_os = "dragonfly", target_os = "freebsd", target_os = "linux", target_os = "macos", target_os = "netbsd", - target_os = "openbsd", - target_os = "redox" + target_os = "openbsd" ))]
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 CommitsReviewing files that changed from the base of the PR and between e9594c3 and 29c40ae.
📒 Files selected for processing (2)- scripts/libc_posix.py (1 hunks)
- vm/src/stdlib/posix.rs (1 hunks)
- scripts/libc_posix.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmt to format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
- vm/src/stdlib/posix.rs
Learnt from: moreal PR: RustPython/RustPython#5847 File: vm/src/stdlib/stat.rs:547-567 Timestamp: 2025-06-27T14:47:28.810Z Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
- vm/src/stdlib/posix.rs
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
vm/src/stdlib/posix.rs (11)54-57: LGTM: PIDFD_NONBLOCK gating is correct (Linux-only).
58-64: LGTM: Darwin-only constants and aliasing to _COPYFILE_DATA look good.
83-86: LGTM: O_SYMLINK correctly gated to macOS/redox.
102-110: LGTM: GRND_ coverage looks reasonable across the listed Unixes.*
149-159: LGTM: RTLD_NOLOAD gating looks fine.
160-169: LGTM: O_DSYNC across listed Unixes is reasonable.
171-181: LGTM: SCHED_OTHER exposure across these targets is fine.
139-147: Remove macOS from O_DIRECT (not a Darwin open(2) flag)
macOS uses F_NOCACHE via fcntl; O_DIRECT isn’t available there.
-#[cfg(any( - target_os = "android", - target_os = "dragonfly", - target_os = "freebsd", - target_os = "linux", - target_os = "netbsd" -))] +#[cfg(any( + target_os = "android", + target_os = "dragonfly", + target_os = "freebsd", + target_os = "linux", + target_os = "netbsd" +))] #[pyattr] use libc::O_DIRECT;Likely an incorrect or invalid review comment.
182-192: Keep Redox in the cfg since O_EXLOCK and O_SHLOCK are defined in Redox’s libc.
Likely an incorrect or invalid review comment.
98-101: Retain NetBSD in cfg NetBSD’s Linux-compatible sys/xattr.h defines XATTR_CREATE and XATTR_REPLACE, so the original #[cfg(any(target_os = "linux", target_os = "macos", target_os = "netbsd"))] is correct.
Likely an incorrect or invalid review comment.
129-137: Keep the single import; all three flags are supported on macOS and Android
macOS’s <dlfcn.h> defines RTLD_NODELETE and its headers (and Python itself) expose SEEK_DATA/SEEK_HOLE (opensource.apple.com, docs.python.org). Android’s Bionic <dlfcn.h> defines RTLD_NODELETE and its <unistd.h> pulls in bits/seek_constants.h for SEEK_DATA/SEEK_HOLE (android.googlesource.com). The existing #[cfg(any(target_os = "android", "dragonfly", "freebsd", "linux", "macos"))] use libc::{RTLD_NODELETE, SEEK_DATA, SEEK_HOLE}; is correct.Likely an incorrect or invalid review comment.
Sorry, something went wrong.
| #[cfg(any(target_os = "android", target_os = "linux"))] | ||
| #[pyattr] | ||
| use libc::{ | ||
| CLONE_FILES, CLONE_FS, CLONE_NEWCGROUP, CLONE_NEWIPC, CLONE_NEWNET, CLONE_NEWNS, | ||
| CLONE_NEWPID, CLONE_NEWUSER, CLONE_NEWUTS, CLONE_SIGHAND, CLONE_SYSVSEM, CLONE_THREAD, | ||
| CLONE_VM, EFD_CLOEXEC, EFD_NONBLOCK, EFD_SEMAPHORE, O_NOATIME, O_TMPFILE, P_PIDFD, | ||
| SCHED_BATCH, SCHED_IDLE, SCHED_RESET_ON_FORK, SPLICE_F_MORE, SPLICE_F_MOVE, | ||
| SPLICE_F_NONBLOCK, | ||
| }; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chainSplit Linux-only flags from Android-safe ones; likely Android build break
Several items here are Linux-only or not guaranteed in bionic (e.g., P_PIDFD, O_TMPFILE, O_NOATIME, CLONE_NEW*). Keep eventfd on Android; move the rest to Linux-only.
-#[cfg(any(target_os = "android", target_os = "linux"))]
-#[pyattr]
-use libc::{
- CLONE_FILES, CLONE_FS, CLONE_NEWCGROUP, CLONE_NEWIPC, CLONE_NEWNET, CLONE_NEWNS,
- CLONE_NEWPID, CLONE_NEWUSER, CLONE_NEWUTS, CLONE_SIGHAND, CLONE_SYSVSEM, CLONE_THREAD,
- CLONE_VM, EFD_CLOEXEC, EFD_NONBLOCK, EFD_SEMAPHORE, O_NOATIME, O_TMPFILE, P_PIDFD,
- SCHED_BATCH, SCHED_IDLE, SCHED_RESET_ON_FORK, SPLICE_F_MORE, SPLICE_F_MOVE,
- SPLICE_F_NONBLOCK,
-};
+# // Android + Linux: eventfd
+#[cfg(any(target_os = "android", target_os = "linux"))]
+#[pyattr]
+use libc::{EFD_CLOEXEC, EFD_NONBLOCK, EFD_SEMAPHORE};
+
+# // Linux-only: clone namespaces, pidfd, tmpfile, noatime, splice, sched_*
+#[cfg(target_os = "linux")]
+#[pyattr]
+use libc::{
+ CLONE_FILES, CLONE_FS, CLONE_NEWCGROUP, CLONE_NEWIPC, CLONE_NEWNET, CLONE_NEWNS, CLONE_NEWPID,
+ CLONE_NEWUSER, CLONE_NEWUTS, CLONE_SIGHAND, CLONE_SYSVSEM, CLONE_THREAD, CLONE_VM, O_NOATIME,
+ O_TMPFILE, P_PIDFD, SCHED_BATCH, SCHED_IDLE, SCHED_RESET_ON_FORK, SPLICE_F_MORE, SPLICE_F_MOVE,
+ SPLICE_F_NONBLOCK,
+};To confirm bionic coverage, please verify:
🌐 Web query:
Which of these constants are exposed by Android bionic's headers: P_PIDFD, O_TMPFILE, O_NOATIME, CLONE_NEWCGROUP?
💡 Result:
O_TMPFILE — yes. [1]
O_NOATIME — yes. [1]
CLONE_NEWCGROUP — yes. [2]
P_PIDFD — not exposed in bionic's public headers (bionic exposes pidfd APIs in <sys/pidfd.h>, but P_PIDFD itself is not defined there). [3][4]
Sources:
- asm-generic fcntl.h (O_TMPFILE, O_NOATIME). [1]
- linux sched.h (CLONE_NEWCGROUP). [2]
- bionic <sys/pidfd.h> (pidfd APIs). [3]
- linux/uapi pidfd header / userland references (shows pidfd flags but P_PIDFD is not in bionic headers). [4]
Only exclude P_PIDFD on Android; keep O_TMPFILE, O_NOATIME, CLONE_NEWCGROUP under Android+Linux
O_TMPFILE, O_NOATIME and CLONE_NEWCGROUP are defined in bionic’s asm-generic fcntl.h and linux/sched.h and safe on Android (android.googlesource.com). P_PIDFD is not defined in bionic’s public headers (sys/pidfd.h provides APIs but no P_PIDFD) (android.googlesource.com); move only P_PIDFD into the #[cfg(target_os = "linux")] block.
🤖 Prompt for AI AgentsIn vm/src/stdlib/posix.rs around lines 65 to 74, the import currently excludes the entire block on non-linux targets but the review notes P_PIDFD is the only symbol missing on Android; update the cfg so O_TMPFILE, O_NOATIME and CLONE_NEWCGROUP (and the other listed constants) remain under #[cfg(any(target_os = "android", target_os = "linux"))] while moving P_PIDFD into a separate #[cfg(target_os = "linux")] import line; adjust the use/libc import(s) accordingly so all safe symbols are available on Android but P_PIDFD is only imported for Linux.
Sorry, something went wrong.
I can say it is arbitrary based on history of the module. Currently os.rs tends to have shared definitions between nt, posix, and posix_compat, but not strictly. You can safely relocate everything if it is better. |
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
👍
Sorry, something went wrong.
Attempt to automate the definition of posix consts. I'm sure that this will require the script to be more adjustable but IMO this is a good first step in automating something that we do manually. for example: #5795
I ain't so sure how we decide what goes into os directly and what into posix, if someone can share some knowledge it would be very appreciated:)
Summary by CodeRabbit