Attempt to automate posix consts #6117
Conversation
WalkthroughAdds a new codegen script ( 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
🧪 Generate unit tests
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. 🪧 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 Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_shortis an alias (i16); it has noMAX. Usei16::MAXand 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 pathsIf any
add*call fails, we return without callingposix_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 pathsBoth
posix_spawnattr_initandposix_spawn_file_actions_initrequire correspondingdestroycalls. Add them for all paths afterattrpis 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 returningEnsure 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)) + }
🧹 Nitpick comments (5)
scripts/libc_posix.py (4)
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
urlopenwithout 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 parameterizePinning
LIBC_VERSIONrisks 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-versiontoo.
94-97: Target CPython source rather than rendered docs for stabilityScraping 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?
vm/src/stdlib/posix.rs (1)
1605-1616: Avoid hard-coding POSIX_SPAWN_SETSID magic numberIf available, prefer
libc::POSIX_SPAWN_SETSIDto0x0080. 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_SETSIDexists on your minimum supported libc, you can simplify to that directly.
📜 Review details
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.
📒 Files selected for processing (2)
scripts/libc_posix.py(1 hunks)vm/src/stdlib/posix.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 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
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto 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
⏰ 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). (11)
- 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)
🔇 Additional comments (2)
vm/src/stdlib/posix.rs (2)
50-56: Nice: cleanly grouped android-only base constantsThe 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_DATAandSEEK_HOLEaren’t guaranteed in Android or DragonFly’s libc—run per-targetcargo check(e.g.aarch64-linux-android,x86_64-unknown-dragonfly) to confirm these imports build, and add#[cfg]guards if any fail.
| 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.
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)🤖 Prompt for AI Agents
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.
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 OSErrorsAsserts 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 pathsposix_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 setWithout 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;
🧹 Nitpick comments (2)
vm/src/stdlib/posix.rs (2)
97-100: Verify NetBSD XATTR_*; likely Linux/macOS onlyNetBSD 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 riskySEEK_{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.
📜 Review details
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.
📒 Files selected for processing (2)
scripts/libc_posix.py(1 hunks)vm/src/stdlib/posix.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/libc_posix.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto 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
⏰ 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). (11)
- 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)
🔇 Additional comments (2)
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 yourlibccrate version definesPOSIX_SPAWN_SETSID(requires glibc ≥ 2.26 with_GNU_SOURCE); if it’s missing, bumplibcor gate out/implement a fallback. (spinics.net)
| #[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.
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};📝 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.
| #[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}; |
| #[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.
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,
+};🤖 Prompt for AI Agents
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.
| #[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.
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};📝 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.
| #[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}; |
🤖 Prompt for AI Agents
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.
| #[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.
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;📝 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.
| #[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; |
🤖 Prompt for AI Agents
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.
| #[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 inconclusive
Tighten 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;📝 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.
| #[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; |
🤖 Prompt for AI Agents
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
vm/src/stdlib/posix.rs (5)
50-53: Fix FreeBSD import: drop Linux-only MFD_HUGE_MASKMFD_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 breaksMFD_* 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/RedoxThis 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 LinuxO_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;
🧹 Nitpick comments (3)
vm/src/stdlib/posix.rs (3)
75-82: Nit: simplify cfg to unixany(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 RedoxThese 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" ))]
📜 Review details
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.
📒 Files selected for processing (2)
scripts/libc_posix.py(1 hunks)vm/src/stdlib/posix.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/libc_posix.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto 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
🧠 Learnings (1)
📚 Learning: 2025-06-27T14:47:28.810Z
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
⏰ 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). (11)
- 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)
🔇 Additional comments (11)
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-compatiblesys/xattr.hdefinesXATTR_CREATEandXATTR_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 inbits/seek_constants.hfor 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.
| #[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 chain
Split 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 Agents
In 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.
I can say it is arbitrary based on history of the module. Currently |
ShaharNaveh
changed the title
2 participants
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
osdirectly and what intoposix, if someone can share some knowledge it would be very appreciated:)Summary by CodeRabbit