posix descriptor and dir_fd forms, and the clamped ceiling #1071 left behind - #1078
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (23)
WalkthroughThe change expands POSIX and Windows ChangesPOSIX compatibility
Optimizer and API contracts
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant ParityTests
participant interp_posix_rs
participant HostFilesystem
ParityTests->>interp_posix_rs: call descriptor or dir_fd operation
interp_posix_rs->>interp_posix_rs: validate platform capability and modifiers
interp_posix_rs->>HostFilesystem: execute filesystem operation
HostFilesystem-->>interp_posix_rs: return result or error
interp_posix_rs-->>ParityTests: return value or exception
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit ea93329). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/548796e4de403ff25e98508988c90b1612a951c2/pyre-interpreter/src/module/posix/interp_posix.rs#L4680
When the caller supplies a closed descriptor or an invalid value other than -1 through dir_fd, unwrap_fd accepts it and this unsafe BorrowedFd::borrow_raw asserts that it denotes a live resource for the borrow's lifetime. Safe Python input can therefore violate Rust's unsafe contract instead of reaching fchownat and deterministically raising EBADF; preserve the upstream raw-fd syscall shape rather than constructing a BorrowedFd from unvalidated input. The new descriptor branches for chown and chmod repeat the same issue.
AGENTS.md reference: AGENTS.md:L231-L233
https://github.com/youknowone/pyre/blob/548796e4de403ff25e98508988c90b1612a951c2/pyre-interpreter/src/module/posix/interp_posix.rs#L1910
For the newly accepted descriptor form, valid pre-epoch inputs such as os.utime(fd, ns=(-1, -1)) never reach futimens: the shared converters above represent timestamps as unsigned Duration values and reject every negative value. Both CPython and upstream's signed seconds/nanoseconds representation accept these timestamps, so the advertised supports_fd implementation remains incomplete; retain signed timespec components when dispatching to futimens and utimensat.
AGENTS.md reference: AGENTS.md:L231-L233
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/check.py`:
- Line 1982: Update the parameter list of the enclosing function around
wasm_float_tol to insert a keyword-only separator before wasm_float_tol, keeping
the preceding threshold arguments positional and requiring keyword arguments for
wasm_float_tol and min_pypy_ratio. Check all call sites to ensure neither
optional performance argument is passed positionally.
In `@pyre/extra_tests/parity_tests/os_supports_dir_fd.py`:
- Around line 102-105: Update the composed os.utime test around the call using
dir_fd and follow_symlinks=False to write and assert a distinct whole-second
link timestamp instead of LINK_MTIME + 1. Preserve the target timestamp
assertion and ensure the new value differs from the preceding link timestamp
setup.
- Around line 46-51: Register temporary-directory cleanup immediately after each
mkdtemp call using atexit.register(shutil.rmtree, d, ignore_errors=True). In
pyre/extra_tests/parity_tests/os_supports_dir_fd.py (lines 46-51), clean up d
and its contents after the directory is created; in
pyre/extra_tests/parity_tests/os_supports_fd.py (lines 19-22), apply the same
registration so both normal completion, assertion failures, and the Windows
early exit clean up the directory.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 2676-2685: Update HAVE_FUTIMENS and HAVE_UTIMENSAT to require the
host_env feature, matching the host_env-dependent capability checks used by the
related fd-relative calls. Keep the existing unix and sandbox conditions, and
ensure these constants do not advertise functionality when rustpython-host_env
is unavailable.
- Around line 4165-4177: Update truncate_length_w to convert the parsed length
with libc::off_t::try_from instead of an unchecked cast, returning an
OverflowError when the value cannot fit off_t. Preserve the helper’s existing
exact “Python int too large to convert to C long” error message and retain
successful conversions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 845d08a1-ed7e-4eeb-b01b-06662260494f
📒 Files selected for processing (16)
pyre/bench/synth/attr_instance_shadows_class.pypyre/bench/synth/getframe_force_cancel_journal.pypyre/bench/synth/getframe_while_subwalk_decline_shapes.pypyre/bench/synth/pickle_ctor_args.pypyre/bench/synth/type_call_inline_init_branch_deopt.pypyre/bench/synth/type_dict_surrogate.pypyre/bench/synth/type_immutable_reject.cranelift.jitstatspyre/bench/synth/type_immutable_reject.dynasm.jitstatspyre/bench/synth/type_immutable_reject.pypyre/bench/synth/type_immutable_reject.wasm.jitstatspyre/bench/synth/type_metatype_data_descr.pypyre/bench/synth/unary_positive_resume.pypyre/check.pypyre/extra_tests/parity_tests/os_supports_dir_fd.pypyre/extra_tests/parity_tests/os_supports_fd.pypyre/pyre-interpreter/src/module/posix/interp_posix.rs
| /// `utime` reaches `futimens` and `utimensat` through `libc` rather than | ||
| /// `host_env`, so it needs one less condition than the rest. | ||
| const HAVE_FUTIMENS: bool = cfg!(all(unix, not(feature = "sandbox"))); | ||
| /// The name form of `utime` is one `utimensat`, so the same bit carries | ||
| /// both of its modifiers: `dir_fd` is the descriptor the name resolves | ||
| /// against and `follow_symlinks=False` is `AT_SYMLINK_NOFOLLOW`. | ||
| const HAVE_UTIMENSAT: bool = cfg!(all(unix, not(feature = "sandbox"))); | ||
| /// `rposix.HAVE_FSTATAT` — what `DirFD` is parameterised on | ||
| /// (`interp_posix.py:612,660`), and what `_have_functions` advertises. | ||
| /// (`interp_posix.py:612,660`). `stat_at` calls `fstatat` through `libc`. | ||
| const HAVE_FSTATAT: bool = cfg!(all(unix, not(feature = "sandbox"))); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether rustpython-host_env and its `posix` module are gated on the `host_env` feature.
set -euo pipefail
# Locate the interpreter manifest and inspect the dependency + feature table.
fd -t f 'Cargo.toml' pyre | while IFS= read -r manifest; do
if rg -q 'rustpython-host_env|host_env' "$manifest"; then
echo "=== $manifest ==="
rg -n -C 4 'rustpython-host_env|^\[features\]|^host_env' "$manifest"
fi
done
# Check whether the `posix` module itself is feature-gated inside the host_env crate.
fd -t f 'lib.rs' --exec rg -n -C 3 'pub mod posix|cfg\(feature\s*=\s*"host_env"\)' {} \;
# Confirm every call site of set_file_times_at and its surrounding cfg.
rg -nP -C 6 'set_file_times_at\s*\(' --type=rustRepository: youknowone/pyre
Length of output: 1940
🏁 Script executed:
#!/bin/bash
# Inspect how set_file_times_at is called and whether HAVE_FUTIMENS/HAVE_UTIMENSAT affect fd/dir_fd paths.
set -euo pipefail
echo "=== Candidate files ==="
fd -t f 'interp_posix.rs|Cargo.toml' .
echo "=== set_file_times_at usages with context ==="
rg -n -C 8 'set_file_times_at|utime_fd|utime' --type=rust | sed -n '1,220p'
echo "=== HAVE_FUTIMENS/HAVE_UTIMENSAT usages ==="
rg -n -C 6 'HAVE_FUTIMENS|HAVE_UTIMENSAT' --type=r
echo "=== Relevant Cargo dependency section ==="
sed -n '1,120p' pyre/pyre-interpreter/Cargo.toml || trueRepository: youknowone/pyre
Length of output: 1546
🏁 Script executed:
#!/bin/bash
# Inspect utime implementation and HAVE_UTIMENSAT advertising.
set -euo pipefail
echo "=== interp_posix.rs outline area containing utime/constants ==="
wc -l pyre/pyre-interpreter/src/module/posix/interp_posix.rs
rg -n -C 12 'set_file_times_at|utime_fd|utime|HAVE_FUTIMENS|HAVE_UTIMENSAT' pyre/pyre-interpreter/src/module/posix/interp_posix.rs
echo "=== Feature gating of rustpython-host_env ==="
sed -n '1,120p' pyre/pyre-interpreter/Cargo.tomlRepository: youknowone/pyre
Length of output: 18782
🏁 Script executed:
#!/bin/bash
# Check whether `utime_impl` path has cfg attributes on the `rustpython_host_env` call itself.
python3 - <<'PY'
from pathlib import Path
p = Path("pyre/pyre-interpreter/src/module/posix/interp_posix.rs")
s = p.read_text()
start = s.index(" fn utime_impl(args: &[PyObjectRef])")
end = s.index(" crate::module_ns_store(", start)
func = s[start:end]
for i, line in enumerate(func.splitlines(), 1):
if "rustpython_host_env::posix::set_file_times_at" in line:
prev = func.splitlines()[i-2] if i > 1 else ""
print(f"line {i} from utime_impl: {line.strip()}")
print(f"previous from utime_impl: {prev.strip()}")
print("contains cfg before actual call:", "cfg(" in prev)
PYRepository: youknowone/pyre
Length of output: 362
Keep H*UTIMENS conditional on host_env.
HOST_POSIX includes host_env, and the Rust dependencies make rustpython-host_env optional behind the host_env feature. Since utime calls rustpython_host_env::posix::set_file_times_at when dir_fd/follow_symlinks=False is used, advertising HAVE_FUTIMENS and HAVE_UTIMENSAT without host_env can expose capabilities that are not usable. Use the same host_env-dependent condition here as for the related fd-relative calls, or guard the rustpython_host_env path with its feature.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 2676 -
2685, Update HAVE_FUTIMENS and HAVE_UTIMENSAT to require the host_env feature,
matching the host_env-dependent capability checks used by the related
fd-relative calls. Keep the existing unix and sandbox conditions, and ensure
these constants do not advertise functionality when rustpython-host_env is
unavailable.
| /// `space.int_w` over the `r_longlong` half of `interp_posix.py:404`. | ||
| #[cfg(all(unix, not(feature = "sandbox")))] | ||
| fn truncate_length_w(obj: PyObjectRef) -> Result<libc::off_t, crate::PyError> { | ||
| let w_length = crate::baseobjspace::space_index(obj)?; | ||
| let length = crate::baseobjspace::int_w(w_length).map_err(|err| { | ||
| if err.kind == crate::PyErrorKind::OverflowError { | ||
| crate::PyError::overflow_error("Python int too large to convert to C long") | ||
| } else { | ||
| err | ||
| } | ||
| })?; | ||
| Ok(length as libc::off_t) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine the declared target set and whether any 32-bit target is built.
set -euo pipefail
# Declared targets in CI and build config.
fd -t f -e yml -e yaml . .github --exec rg -n -C 2 'target|--target' {} \; 2>/dev/null || true
fd -t f 'config.toml' .cargo --exec cat {} \; 2>/dev/null || true
# Other off_t conversions already in the tree, to see the established convention.
rg -nP -C 3 'as\s+libc::off_t|off_t::try_from' --type=rustRepository: youknowone/pyre
Length of output: 10998
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant Rust helper and nearby call path without running repository code.
sed -n '4130,4225p' pyre/pyre-interpreter/src/module/posix/interp_posix.rs
# Inspect the RPython/PyPy source referenced by the parity check, if present.
sed -n '380,425p' pyre/pyre-interpreter/src/module/posix/interp_posix.py 2>/dev/null || true
# Find integer-size helpers/usages to understand whether usize/off_t conversions are checked.
rg -n -C 2 '#\[cfg\(.*32.*\)|usize::try_from|libc::off_t::try_from|off_t::try_from|as\s+libc::off_t|libc::ssize_t::try_from|Py_ssize_t|c::Py_ssize_t' --type=rust pyre pyre-interpreter 2>/dev/null || true
# Check crate metadata and declared targets without installing/building.
for f in Cargo.toml Cargo.lock pyre-interpreter/Cargo.toml .cargo/config.toml .github/workflows/*.yml; do
test -f "$f" || continue
echo "===== $f ====="
rg -n -C 2 'targets\s*=|target\s*=|--target|target-arch|i686|i386|386|x86_64|wasm32|offset|off_t|os\.truncate|ftruncate' "$f" || true
doneRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the RPython Python source for the length validation semantics if included.
printf '===== interp_posix.py around truncate =====\n'
sed -n '380,425p' pyre/pyre-interpreter/src/module/posix/interp_posix.py
# Check whether os.truncate/ftruncate tests exist and what lengths they use.
printf '\n===== truncate/ftruncate tests =====\n'
rg -n -C 3 'truncate|ftruncate|2\*\*\{40\}|2\*\*40|big int|2\*\*\(' tests pyre/pyre-interpreter pyre/test 2>/dev/null || true
# Find all Rust ftruncate truncation helpers and whether any use try_from/off_t checks.
printf '\n===== precise ftruncate helpers =====\n'
rg -n -C 2 'ftruncate.*as\s+libc::off_t|libc::off_t::try_from|try_from\(.*ftruncate|size as libc::off_t|length as libc::off_t' --type=rustRepository: youknowone/pyre
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '===== locate interp_posix.py =====\n'
fd -t f 'interp_posix\.py$' . || true
python3 - <<'PY'
import pathlib, re
for p in pathlib.Path('.').rglob('interp_posix.py'):
print(p)
PY
printf '\n===== inspect truncat helpers in posix modules =====\n'
for f in $(fd -t f 'interp_posix\.py$' .); do
echo "--- $f ---"
rg -n -C 4 'truncate|ftruncate|O_CLOEXEC|OFF_T|_longlong|r_longlong|OFF64_L' "$f" || true
done
printf '\n===== precise ftruncate helpers in Rust =====\n'
rg -n -C 2 'ftruncate.*as\s+libc::off_t|libc::off_t::try_from|try_from\(.*ftruncate|size as libc::off_t|length as libc::off_t' --type=rustRepository: youknowone/pyre
Length of output: 5122
🌐 Web query:
Python os.truncate integer length too large for platform off_t raises OverflowError documentation
💡 Result:
The OverflowError raised by Python's os.truncate (and os.ftruncate) when an integer length argument is too large occurs because the length must be converted to the C-level off_t type, which is platform-dependent and has a fixed size (often 32-bit on older or specific 32-bit systems) [1][2][3]. When the Python integer passed as the length exceeds the maximum value representable by the platform's C off_t type, Python raises an OverflowError [3][4]. This behavior is a byproduct of the underlying operating system's system call implementation—specifically, that the C function (e.g., truncate or ftruncate) expects an off_t argument [3]. Key points regarding this behavior: 1. Platform Dependency: The constraint is determined by the C compiler and the platform's architecture. On systems where off_t is 32-bit, you cannot truncate a file to a size larger than approximately 2 GiB [5][6]. On systems with 64-bit off_t (standard on modern 64-bit systems), this limit is significantly higher [1][2]. 2. Historical Context: While Python handles arbitrarily large integers, functions that interface directly with C system calls must often perform a narrow conversion to C-compatible types [5][7]. OverflowError is sometimes used in these instances when the value exceeds what the C-level type can accommodate [4][7]. 3. Current Status: There have been discussions within the Python development community about mitigating these OverflowErrors to provide more consistent behavior (e.g., potentially raising an alternative error or handling values differently), but because the underlying C system call is restricted by the platform, raising an error remains the standard response when the OS cannot support the requested operation [3][4]. If you encounter this error, it typically indicates that you are working on a platform or file system configuration that lacks "large file support" (LFS) or is running on a 32-bit architecture where the OS itself cannot handle files of the requested size [2]. Ensure that your environment is configured for large file support, which usually involves ensuring that _FILE_OFFSET_BITS=64 is set during the compilation of the Python interpreter on affected platforms [2].
Citations:
- 1: https://bugs.python.org/issue400510
- 2: https://bugs.python.org/issue26926
- 3: Avoid raising OverflowError in truncate() if possible python/cpython#74005
- 4: https://bugs.python.org/issue29819
- 5: https://bugs.python.org/issue1215928
- 6: Large tarfiles cause overflow python/cpython#42059
- 7: https://docs.python.org/3/library/exceptions.html
Reject lengths that do not fit off_t before passing them to ftruncate.
len >= 2**40 passes int_w, but on 32-bit off_t builds length as libc::off_t wraps and ftruncate truncates to the wrong size. os.truncate/os.ftruncate should raise OverflowError for values that do not fit the C argument type; use libc::off_t::try_from(length) and keep this helper’s exact error message for parity.
🐛 Proposed fix
})?;
- Ok(length as libc::off_t)
+ libc::off_t::try_from(length).map_err(|_| {
+ crate::PyError::overflow_error("Python int too large to convert to C long")
+ })
}📝 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.
| /// `space.int_w` over the `r_longlong` half of `interp_posix.py:404`. | |
| #[cfg(all(unix, not(feature = "sandbox")))] | |
| fn truncate_length_w(obj: PyObjectRef) -> Result<libc::off_t, crate::PyError> { | |
| let w_length = crate::baseobjspace::space_index(obj)?; | |
| let length = crate::baseobjspace::int_w(w_length).map_err(|err| { | |
| if err.kind == crate::PyErrorKind::OverflowError { | |
| crate::PyError::overflow_error("Python int too large to convert to C long") | |
| } else { | |
| err | |
| } | |
| })?; | |
| Ok(length as libc::off_t) | |
| } | |
| /// `space.int_w` over the `r_longlong` half of `interp_posix.py:404`. | |
| #[cfg(all(unix, not(feature = "sandbox")))] | |
| fn truncate_length_w(obj: PyObjectRef) -> Result<libc::off_t, crate::PyError> { | |
| let w_length = crate::baseobjspace::space_index(obj)?; | |
| let length = crate::baseobjspace::int_w(w_length).map_err(|err| { | |
| if err.kind == crate::PyErrorKind::OverflowError { | |
| crate::PyError::overflow_error("Python int too large to convert to C long") | |
| } else { | |
| err | |
| } | |
| })?; | |
| libc::off_t::try_from(length).map_err(|_| { | |
| crate::PyError::overflow_error("Python int too large to convert to C long") | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 4165 -
4177, Update truncate_length_w to convert the parsed length with
libc::off_t::try_from instead of an unchecked cast, returning an OverflowError
when the value cannot fit off_t. Preserve the helper’s existing exact “Python
int too large to convert to C long” error message and retain successful
conversions unchanged.
548796e to
d3e70b7
Compare
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3e70b74bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `inheritable=False`; nothing here forks between the open | ||
| // and the close, but the descriptor is still not the | ||
| // caller's to inherit. | ||
| let fd = unsafe { libc::open(c_path.as_ptr(), libc::O_WRONLY | libc::O_CLOEXEC) }; |
There was a problem hiding this comment.
When path names a FIFO with no reader, this O_WRONLY open blocks while retaining the interpreter lock, so another Python thread cannot run to open the FIFO's read end and the process deadlocks. The existing os.open implementation explicitly executes this syscall through call_external_function because FIFO opens may wait; route this through that implementation, as upstream truncate does, or use the same blocking guard and EINTR handling here.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
d3e70b7 to
f1e960b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 4208-4224: Update the truncate path around the direct libc::open
call to invoke open through crate::module::thread::call_external_function,
matching the existing posix.open implementation, so the interpreter is released
during blocking I/O. Add EINTR retry handling for the open operation before
returning an error, while preserving O_WRONLY|O_CLOEXEC and the existing close
and ftruncate_retry flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e97eb36-5a90-4306-a0b0-f17be1a0eb10
📒 Files selected for processing (4)
majit/majit-metainterp/src/optimizeopt/virtualize.rspyre/extra_tests/parity_tests/os_supports_dir_fd.pypyre/extra_tests/parity_tests/os_supports_fd.pypyre/pyre-interpreter/src/module/posix/interp_posix.rs
| let c_path = std::ffi::CString::new(path.as_bytes.as_slice()) | ||
| .map_err(|_| crate::PyError::value_error("embedded null in path"))?; | ||
| // `open(space, w_path, os.O_WRONLY)` is `rposix.open` with | ||
| // `inheritable=False`; nothing here forks between the open | ||
| // and the close, but the descriptor is still not the | ||
| // caller's to inherit. | ||
| let fd = unsafe { libc::open(c_path.as_ptr(), libc::O_WRONLY | libc::O_CLOEXEC) }; | ||
| if fd < 0 { | ||
| return Err(io_err_with_filename( | ||
| std::io::Error::last_os_error(), | ||
| path.w_path(), | ||
| )); | ||
| } | ||
| let truncated = ftruncate_retry(fd, length); | ||
| unsafe { libc::close(fd) }; | ||
| truncated.map_err(|e| io_err_with_filename(e, path.w_path()))?; | ||
| Ok(pyre_object::w_none()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Route the open in truncate through call_external_function and retry EINTR.
Line 4214 calls libc::open directly. posix.open (Line 1346) wraps the same syscall in crate::module::thread::call_external_function, which releases the interpreter around a call that can block on a FIFO or a slow device. This site keeps the interpreter blocked for the whole open. The call also has no EINTR handling, while the following ftruncate_retry does, so a signal during the open surfaces as InterruptedError.
♻️ Proposed change
- let fd = unsafe { libc::open(c_path.as_ptr(), libc::O_WRONLY | libc::O_CLOEXEC) };
- if fd < 0 {
- return Err(io_err_with_filename(
- std::io::Error::last_os_error(),
- path.w_path(),
- ));
- }
+ let fd = loop {
+ let (fd, errno) = crate::module::thread::call_external_function(|| unsafe {
+ libc::open(c_path.as_ptr(), libc::O_WRONLY | libc::O_CLOEXEC)
+ });
+ if fd >= 0 {
+ break fd;
+ }
+ crate::builtins::eintr_retry_with(
+ std::io::Error::from_raw_os_error(errno),
+ |e| io_err_with_filename(e, path.w_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.
| let c_path = std::ffi::CString::new(path.as_bytes.as_slice()) | |
| .map_err(|_| crate::PyError::value_error("embedded null in path"))?; | |
| // `open(space, w_path, os.O_WRONLY)` is `rposix.open` with | |
| // `inheritable=False`; nothing here forks between the open | |
| // and the close, but the descriptor is still not the | |
| // caller's to inherit. | |
| let fd = unsafe { libc::open(c_path.as_ptr(), libc::O_WRONLY | libc::O_CLOEXEC) }; | |
| if fd < 0 { | |
| return Err(io_err_with_filename( | |
| std::io::Error::last_os_error(), | |
| path.w_path(), | |
| )); | |
| } | |
| let truncated = ftruncate_retry(fd, length); | |
| unsafe { libc::close(fd) }; | |
| truncated.map_err(|e| io_err_with_filename(e, path.w_path()))?; | |
| Ok(pyre_object::w_none()) | |
| let c_path = std::ffi::CString::new(path.as_bytes.as_slice()) | |
| .map_err(|_| crate::PyError::value_error("embedded null in path"))?; | |
| // `open(space, w_path, os.O_WRONLY)` is `rposix.open` with | |
| // `inheritable=False`; nothing here forks between the open | |
| // and the close, but the descriptor is still not the | |
| // caller's to inherit. | |
| let fd = loop { | |
| let (fd, errno) = crate::module::thread::call_external_function(|| unsafe { | |
| libc::open(c_path.as_ptr(), libc::O_WRONLY | libc::O_CLOEXEC) | |
| }); | |
| if fd >= 0 { | |
| break fd; | |
| } | |
| crate::builtins::eintr_retry_with( | |
| std::io::Error::from_raw_os_error(errno), | |
| |e| io_err_with_filename(e, path.w_path()), | |
| )?; | |
| }; | |
| let truncated = ftruncate_retry(fd, length); | |
| unsafe { libc::close(fd) }; | |
| truncated.map_err(|e| io_err_with_filename(e, path.w_path()))?; | |
| Ok(pyre_object::w_none()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 4208 -
4224, Update the truncate path around the direct libc::open call to invoke open
through crate::module::thread::call_external_function, matching the existing
posix.open implementation, so the interpreter is released during blocking I/O.
Add EINTR retry handling for the open operation before returning an error, while
preserving O_WRONLY|O_CLOEXEC and the existing close and ftruncate_retry flow.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1e960b8a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| access: std::time::Duration, | ||
| modified: std::time::Duration, |
There was a problem hiding this comment.
For the newly enabled descriptor form, representing both timestamps as std::time::Duration makes every pre-epoch value unrepresentable: os.utime(fd, (-1, -2)) and os.utime(fd, ns=(-1, -2)) are valid on POSIX, but the shared conversion rejects them with ValueError before this helper reaches futimens. Upstream preserves signed seconds/nanoseconds through parse_utime_args and writes them directly to timespec; use an equivalent signed representation rather than routing the new fd path through Duration.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
f1e960b to
bae92a6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bae92a610d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| )); | ||
| } | ||
| let truncated = ftruncate_retry(fd, length); | ||
| unsafe { libc::close(fd) }; |
There was a problem hiding this comment.
When the descriptor opened for a path reports a delayed writeback error such as EIO or ENOSPC from close, this unconditional discard makes os.truncate(path, length) report success even though the filesystem reported failure. PyPy's interp_posix.py:429-431 calls close() in the finally block and therefore propagates that error; preserve the close result here rather than ignoring it.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| .map_err(|e| io_err_with_filename(e, path.w_path()))? | ||
| { | ||
| let limit = if path.as_fd != -1 { | ||
| host_posix::fpathconf(path.as_fd, name).map_err(|e| io_err(e, ""))? |
There was a problem hiding this comment.
When the newly accepted descriptor is queried for an indeterminate limit (for example PC_ASYNC_IO or PC_SYMLINK_MAX on Linux), host_posix::fpathconf represents the successful -1/unchanged-errno result as None, and the shared branch below converts that to Python None. os.pathconf(fd, name) must return the integer -1, as PyPy's descriptor branch passes the result directly to space.newint; otherwise callers of this newly advertised fd form receive the wrong type and value.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
…rectory cleanup `run_bench`'s `wasm_float_tol` and `min_pypy_ratio` are now keyword-only. Ruff reports FBT002 for the first; no call site passed either positionally. Ten parity scripts called `tempfile.mkdtemp` and never removed the result, so every run of the suite left a directory behind — three runners over 196 scripts. `atexit.register(shutil.rmtree, ...)` covers the early `raise SystemExit` on Windows and every assertion failure without wrapping each script in a `try`/`finally`. The #1078 review named the two in that diff; the other eight are the same line. os_supports_dir_fd.py's composed dir_fd + follow_symlinks call wrote `LINK_MTIME + 1`, one nanosecond past a whole second, against a file that states two lines above that every timestamp is a whole second so a coarse filesystem still reads back what was written. It also could not tell "the call did nothing" from "the value rounded down", because the step before it had already written `LINK_MTIME`. It writes a second whole-second pair instead. Assisted-by: Claude
Four answers from the #1078 review that were the wrong value rather than the wrong call. `pathconf`'s fifth rides with the conf* tables, which it shares a helper with. `truncate` opened the name it was given with a bare `libc::open`. The name can be a FIFO with no reader, and the call waited there holding the interpreter, so no other thread could reach the other end; it also reported an interrupted open as `InterruptedError` where the `ftruncate` beside it retried. It now goes through `call_external_function` under the same retry loop, which is what `interp_posix.py:418` reaches by opening through the module's own `open`. The close is no longer discarded: `interp_posix.py:429-431` closes in a `finally`, through a `close` that raises, so a writeback error the close is first to see is the caller's. The truncation's own failure still wins when both fail. `truncate_length_w` narrowed to `off_t` with an `as` cast. A length wider than `off_t` became a different length rather than an error, and the file was truncated to that; `off_t::try_from` reports it with the message the helper already had for a too-wide value. `utime` carried both timestamps as `std::time::Duration`, which has no second below the epoch, so every pre-epoch time was refused with "timestamp out of range". `rposix.futimens` and `rposix.utimensat` (`rposix.py:2634-2671`) keep the seconds and the nanoseconds apart and signed; this does the same, with the floor-division `_PyTime_ObjectToTimespec` applies, so `ns=(-1, -1)` is `(-1, 999999999)` and reads back as `-1`. The name form now calls `utimensat` directly rather than `rustpython_host_env::posix::set_file_times_at`, whose signature cannot carry a negative second — which is also what `interp_posix.rs:3136` already said it did. The Windows host call still counts upwards from the epoch, and turns a pre-epoch time away rather than writing a different one. `times` is accepted as a keyword. `interp_posix.py:1862` puts `__kwonly__` after `w_times`, so it is the one argument here a caller may spell either way, and it was positional-only. extra_tests/parity_tests/os_utime_pathconf_truncate.py pins these against CPython 3.14, together with pathconf's answer. Assisted-by: Claude
`BorrowedFd::borrow_raw` documents one value it may not be given: `-1`, which the standard library reserves as the niche that makes `Option<BorrowedFd>` cost nothing. Ten call sites built one straight out of an `fd` argument the caller supplied, and `os.fchmod(-1, 0o644)`, `os.chown(p, -1, -1, dir_fd=-1)` and `os.sendfile(-1, ...)` are all reachable from Python. `fd_borrow` answers those with the `EBADF` the syscall would have answered with, so the observable behaviour is unchanged and the one integer that may not become a handle no longer does. The sites reading a descriptor the module itself just produced — `dup2`'s result, and `path.as_fd` past its own `!= -1` guard — are left alone; those are not caller values. Reported in the #1078 review. Assisted-by: Claude
Both were noop stubs: `confstr_names` answered `None` rather than a dict, and `confstr` answered `None` whatever it was asked. `posixmodule.c posix_constants_confstr` and `rposix.py:2248-2300` name the same candidate set, every entry `#ifdef`-guarded, so a host publishes exactly the names its own `<unistd.h>` defines. `libc` carries `_CS_PATH` and nothing else, and the two numberings disagree from that first entry on — 1 on the Apple targets, 0 in glibc's `bits/confname.h`, whose enum also restarts twice, at 1000 and at 1100. Both tables are written out for that reason: 17 names on Darwin, 27 on glibc. The ten the candidate set carries for the System V hosts are defined by neither header, so neither table has them. The tables were derived by crossing `posix_constants_confstr` with each host's header rather than written from memory. The same derivation over the macOS SDK reproduces CPython 3.14's own `os.confstr_names` on this host entry for entry, which is what says the method is right. The glibc half comes from that project's `bits/confname.h`, and every value in it is confirmed a second time by `libc`: it carries `_CS_PATH` 0, `_CS_GNU_LIBC_VERSION` 2 and `_CS_GNU_LIBPTHREAD_VERSION` 3 outright, and it puts `_CS_POSIX_V6_ILP32_OFF32_CFLAGS` at 1116 — one past the end of the XBS5 run, which is what fixes that run at 1100..1115. No Linux compile was reached; this branch has none available. The call is `rposix.confstr` (`rposix.py:2129-2143`): ask for the length, fill a buffer of exactly that size. A zero length is either a name the host has no string for, which is `None`, or one it does not know, which is the errno it set — so errno is cleared before the question is put. The length counts the terminator and the string does not, so `len - 1` bytes are decoded, the way `os_confstr_impl` does; `rffi.charp2strn(buf, n)` keeps it. The value can be a search path, so it is decoded through the filesystem handler. `confname_arg` now takes the table to resolve against, and the dict-building it shares with `pathconf_names` is one function. `pathconf` and `fpathconf` come with it, because the fix lands in the lines that refactor touched. Both answered `None` where the host has no determinate limit; `interp_posix.py:2433` hands whatever `pathconf` returned to `space.newint`, so the answer is the number `-1`. `PC_ASYNC_IO` and `PC_SYMLINK_MAX` are the names that reach it on hosts that do not implement them, and `None` is neither that value nor a type a caller can compare against a limit. Reported in the #1078 review; pinned by extra_tests/parity_tests/os_utime_pathconf_truncate.py. The sandbox build refuses `confstr`: the answer is the host's own search path among other strings, and `pathconf`, `fpathconf` and `sysconf` are refused there beside it. extra_tests/parity_tests/os_confstr.py checks what holds on any host that has the call — the names resolve, the values are distinct, CS_PATH is a real search path, and an unknown name is refused. Assisted-by: Claude
bae92a6 to
460580b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 460580b343
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let name = unsafe { std::ffi::CStr::from_ptr((*entry).d_name.as_ptr()) }; | ||
| let name = name.to_bytes(); | ||
| if name != b"." && name != b".." { | ||
| names.push(name.to_vec()); |
There was a problem hiding this comment.
For the newly supported os.scandir(fd) form, retaining only each entry's name discards the d_type and inode that readdir already returned. Consequently, if an entry is removed or replaced after the scan, DirEntry.is_file()/is_dir() and inode() perform a fresh fstatat and return false, raise, or describe the replacement, whereas PyPy's W_DirEntry stores known_type and inode from the directory entry. Carry those fields into the created DirEntry objects instead of reducing entries to names.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
| crate::PyError::overflow_error( | ||
| "Python int too large to convert to C long", | ||
| ) | ||
| })? as libc::id_t; |
There was a problem hiding this comment.
On platforms such as 64-bit Linux where id_t is 32-bit, this cast silently truncates any Python integer that fits i64 but not id_t. For example, os.waitid(os.P_PID, 2**32 + child_pid, os.WEXITED) can operate on and reap child_pid, while CPython rejects the argument with OverflowError; use a checked conversion before issuing the syscall.
Useful? React with 👍 / 👎.
| /// modifier. `os.py:118` reads it as `chmod` honouring `dir_fd`; `os.py:179` | ||
| /// deliberately does *not* read it for `follow_symlinks`, because a host can | ||
| /// have `fchmodat` and still not honour `AT_SYMLINK_NOFOLLOW`. | ||
| const HAVE_FCHMODAT: bool = cfg!(all(unix, not(feature = "sandbox"))); |
There was a problem hiding this comment.
In a Unix --no-default-features build, this evaluates to true even though the real chmod override is compiled only inside the #[cfg(all(unix, feature = "host_env"))] block; the initial no-op chmod placeholder therefore remains installed. os.py will put chmod in supports_dir_fd, but os.chmod(path, mode, dir_fd=fd) then silently returns without changing the file. Either compile the libc implementation outside host_env or include host_env in this capability condition.
Useful? React with 👍 / 👎.
| /// apart. `os.py:189` reads it a second time as `stat` honouring | ||
| /// `follow_symlinks`; where this bit is false, `MS_WINDOWS` carries that | ||
| /// second claim instead (`os.py:192`). | ||
| const HAVE_LSTAT: bool = HAVE_FSTATAT; |
There was a problem hiding this comment.
In the sandbox configuration, HAVE_FSTATAT is false, but the sandbox branch of stat_path still implements follow_symlinks=False through host_seam::ops::lstat, and stat is not overwritten by the sandbox stubs. Tying HAVE_LSTAT to HAVE_FSTATAT therefore removes os.stat from supports_follow_symlinks despite the modifier working, causing callers to choose unnecessary or incorrect fallbacks; preserve the independently supported HAVE_LSTAT capability.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
…hat rejected one
os.chdir, os.chmod, os.chown, os.pathconf, os.statvfs and os.utime each
appeared in os.supports_fd and answered a descriptor with
TypeError: expected str, bytes or os.PathLike object, not int
They now unwrap through `gateway::fsencode_path_or_fd_w` and branch on
`Path.as_fd` where `interp_posix.py` does: chdir to fchdir (:910-918),
chmod to fchmod (:1228-1243), chown to fchown (:2475-2500), pathconf to
fpathconf (:2420-2433), statvfs to fstatvfs (:704-719) and utime to
futimens (:1860-1904). lchown keeps `allow_fd=0` and so keeps the
narrower allowed-type list in its own type error.
chown and utime reject follow_symlinks=False beside a descriptor, and
utime rejects dir_fd, with the ValueErrors 3.14 words; `interp_posix.py`
spells the first of them "cannnot" and words utime's dir_fd conflict
"can't specify both dir_fd and fd", where 3.14 says "can't specify dir_fd
without matching path". The parity suite reads CPython as the oracle.
_have_functions is now built from the same constants the entry points
branch on, so the advertisement cannot drift from the behaviour: the
family drops out under sandbox and on the hosts carrying no
host_env::posix, rather than each being spelled twice. HAVE_FUTIMES
leaves the list because nothing here calls futimes, and os.py:150-151
reads either bit as the same utime capability.
Assisted-by: Claude
os.truncate was one of the noop placeholders registered so os.py's
_exists probes find a name, so it returned None and left the file
untouched — on a path and on a descriptor alike — while HAVE_FTRUNCATE
advertised it in os.supports_fd. os.ftruncate did the work all along:
with open(p, "wb") as f: f.write(b"0123456789")
os.truncate(p, 4); os.stat(p).st_size # 10
os.ftruncate(fd, 1); os.stat(fd).st_size # 1
interp_posix.py:414-431 takes a descriptor as it stands and opens a name
write-only, truncates whichever it ended up with, and closes only the one
it opened. The EINTR retry loop and the length conversion move out of the
os.ftruncate closure so both callers share them; the path form reports
the name it opened, which the descriptor form has none of.
Assisted-by: Claude
`os.getcwd()` built its str with `to_string_lossy`, so a directory whose name carries a byte with no UTF-8 reading came back with U+FFFD in place of it and no longer named the directory it came from. interp_posix.py:906 is `space.fsdecode(getcwdb(space))`, whose surrogateescape round-trips. `gateway::fsdecode_os_str` and `fsdecode_filename_bytes` are the same decoders sys.argv goes through, so the two boundaries agree. Not exercised end to end here: APFS refuses to create a directory whose name is not valid UTF-8 (OSError errno 92), so the case needs a Linux host. Reported by the Codex parity review on #1066 (section 3). Assisted-by: Claude
The set is what callers read to choose an fd-relative implementation over a path-based one, so a member that rejects an integer — or accepts one and does nothing — sends the caller down a route that cannot work. The script asserts the eight-name floor before the per-name blocks, so a capability that is dropped rather than fixed fails here instead of quietly shrinking the coverage of the guarded blocks below it. Each name is then called with a descriptor and the result observed rather than the return value trusted: chdir moves the process, chmod and utime are read back through stat, truncate shrinks the file. It also pins the two type errors path_or_fd emits — the caller-named list widens with the descriptor form, so lchown answers "string, bytes or os.PathLike" where chown answers "string, bytes, os.PathLike or integer" — the two ValueErrors a descriptor beside dir_fd or follow_symlinks raises, and that utime's times/ns conflict outranks both. listdir, scandir and execve are skipped: the first two take a nullable path and word their type error differently, and execve takes an argv. Assisted-by: Claude
…MENSAT
chown already reached `fchownat`, but with `AT_FDCWD` hard-coded and the
dir_fd keyword rejected with NotImplementedError one step earlier; the
descriptor the caller names is now what the name resolves against.
`_DirFD_Unavailable.unwrap` (interp_posix.py:285-292) converts before it
reports the platform, so the value is unwrapped first and the availability
bit consulted second. A descriptor path plus dir_fd is the ValueError
interp_posix.py:2481-2483 raises, checked ahead of the follow_symlinks one
it already had.
Neither macro was in `_have_functions`, so os.py put chown and utime in
neither supports_dir_fd (os.py:119,133) nor supports_follow_symlinks
(os.py:180,191) while both entry points implemented the modifiers —
utime through one `utimensat`, chown through one `fchownat`. HAVE_LCHOWN
and HAVE_LUTIMES stay unlisted for the reason HAVE_FUTIMES already carried:
os.py reads them as the same capability and neither `lchown` nor `lutimes`
is called here.
Two lib-python tests move from failing to passing against the same corpus
otherwise unchanged (dynasm, 599 tests across the two modules, no other
row moves):
test_os.UtimeTests.test_utime_invalid_arguments — asserted
NotImplementedError for `follow_symlinks=False` because utime was
absent from supports_follow_symlinks, and pyre did not raise
test_shutil.TestMisc.test_chown — shutil.chown(dir_fd=...) hit the
NotImplementedError
Assisted-by: Claude
…laims them The companion to os_supports_fd.py for the other two capability sets. It asserts the chown/stat/utime floor in both supports_dir_fd and supports_follow_symlinks, then calls each with the modifier and reads the result back rather than trusting the return value. Every dir_fd call is repeated against a name that does not exist under the descriptor, which is what separates "resolved the name against dir_fd" from "ignored it and reached the same file through the cwd" — a fixture whose relative name also resolves from the process cwd cannot tell the two apart. follow_symlinks=False is read back through lstat with the target asserted unmoved, the two modifiers are then used together, and the three ValueErrors a descriptor path produces are pinned by message. The last block is what the second set buys a caller: shutil.copystat with follow_symlinks=False substitutes `_nop` (shutil.py:435-439) for any name the set does not carry, so before this the call silently copied nothing. Assisted-by: Claude
optimize_getfield_gc answered GETFIELD_GC on a virtual only when the trace
had already stored that field; an unset field fell through to PassOn and the
load was emitted. virtualize.py:184-193 substitutes
optimizer.new_const(fielddescr) when opinfo.getfield returns None.
Port that fallback. typeptr, w_class and the GETFIELD_RAW_* opcodes stay out
of it: the first two are header fields the same function already resolves
from class identity, and upstream defines this handler for
GETFIELD_GC_{I,R,F} only.
pytraceback.rs:462 reads an exception's traceback slot before writing it, so
every raise emitted that load and the arg-forcing pass materialized the
exception behind it. type_immutable_reject's compiled loop body goes from 62
ops with 8 allocations to 19 ops with none.
wasm's exception_value_op_caught baseline drops to guard_failures=1, the
value dynasm and cranelift already record.
Assisted-by: Claude
… MS_WINDOWS makes
`("HAVE_LSTAT", true)` was the one hardcoded entry in a table whose every
other bit is the condition its entry point branches on. os.py reads that
bit twice: :118 `_add("HAVE_LSTAT", "lstat")` builds supports_dir_fd, and
:189 `_add("HAVE_LSTAT", "stat")` builds supports_follow_symlinks. On
Windows HAVE_FSTATAT is false, so `os.lstat(name, dir_fd=fd)` raises while
the set still listed it:
os.supports_dir_fd # {<built-in function lstat>} on windows
HAVE_LSTAT now follows HAVE_FSTATAT, which is the call both spellings
resolve a dir_fd with. That drops os.stat from supports_follow_symlinks
wherever the bit goes false, and os.py offers MS_WINDOWS (:192) as the
other vehicle for that claim — which also claims chmod takes a descriptor
(:143) and honours follow_symlinks (:184).
So os.chmod on Windows grows the two forms the bit advertises. The
descriptor form is `host_nt::fchmod` on the handle the CRT descriptor
wraps, and dispatches with no follow_symlinks test, as
interp_posix.py:1233-1241 does; follow_symlinks=False is
`host_nt::win32_lchmod`, the name's own attributes rather than the file
the link resolves to. dir_fd stays refused, and refuses the way
`_DirFD_Unavailable.unwrap` (interp_posix.py:285-292) does — converting
the value first, so a wrongly typed one is a TypeError.
MS_WINDOWS is appended after the HAVE_* rows, the position
interp_posix.py:2854-2855 gives it.
os_supports_fd.py's win32 branch was asserting os.stat alone and exiting;
it now exercises every name the platform advertises — the round-trip
through a descriptor for chmod and truncate, follow_symlinks read back
through the attribute bit, the dir_fd rejection, and the widened
allowed-type message — and fails on any advertised name it has no probe
for.
Assisted-by: Claude
fdlistdir reads the names a descriptor holds: fdopendir on an F_DUPFD_CLOEXEC duplicate, a readdir loop that clears errno per call, then rewinddir before closedir so the caller's descriptor keeps its offset. listdir returns those names as str. scandir builds its entries from the same call when it is given a descriptor. Each entry's path is the bare name and carries the descriptor, and stat, inode, is_dir, is_file and is_symlink resolve that name with fstatat against it; a failed stat names the entry. The three is_* predicates now share one dir_entry_kind that reads S_IFMT. gateway gains fsencode_path_or_fd_nullable_w, the nullable half of _unwrap_path: None resolves to "." in the unwrapper rather than at each boundary, and the allowed-type list becomes the four-way matrix, which the DeprecationWarning now words the same way as the TypeError. _have_functions lists HAVE_FDOPENDIR, in the position the upstream name list gives it. The constant is HOST_POSIX, the condition fdlistdir compiles under. os_supports_fd.py covers both descriptor forms, the None forms, and the nullable allowed-type message. Assisted-by: Claude
path_or_fd_w takes bytes and no longer any readable buffer. _unwrap_path's buffer arm (interp_posix.py:188-198) accepts one with a DeprecationWarning; 3.14 completed that deprecation, so a bytearray now gets the same TypeError every other rejected type gets. posix.fspath carried a second copy of that arm, which is what let os.fsencode and os.fsdecode hand a bytearray back unconverted. It also never checked what __fspath__ answered with; both are now the gateway's rules. The type in these messages is named by _PyType_Name — its own name without the module that qualifies it elsewhere, so array.array is reported as array. os_path_argument_types.py covers the rejection at every path boundary, the two shapes of the message, and that bytes, a bytes subclass and os.PathLike still work. Assisted-by: Claude
The unix chmod was registered with a fixed arity of 2, so it took no keyword at all — spelling either modifier's own default was a TypeError. It now binds its arguments the way chown_entry does and dispatches like _chmod_path (interp_posix.py:1254-1258): fchmodat where a name has to be resolved against a directory descriptor or the final symlink must not be followed, plain chmod otherwise. A descriptor still answers before either modifier is consulted (interp_posix.py:1233-1242). chown turns both away in that case and chmod does not, so the two entry points differ here on purpose. os.lchmod stops being one of the no-op stubs at the top of the module and becomes chmod's follow_symlinks=False arm, registered only on the hosts that carry a working lchmod. _have_functions gains HAVE_FCHMODAT, which os.py:118 reads as chmod honouring dir_fd, and HAVE_LCHMOD, which os.py:183 reads as chmod honouring follow_symlinks. The second is the narrower bit for the reason os.py:159-177 gives: fchmodat can be present and still not honour AT_SYMLINK_NOFOLLOW. os_chmod_modifiers.py exercises each claim rather than asserting it — the dir_fd arm resolves a name that does not exist in the working directory, and the follow_symlinks arm reads the link's mode and the target's separately. Assisted-by: Claude
…'s to see DirEntry's is_dir/is_file/is_symlink answered False for every stat failure. check_mode (interp_scandir.py:319-330) answers "not this type" for ENOENT alone — a vanished entry is better reported as not being of the asked-for kind than as an error — and propagates the rest, named by the entry. Both arms of dir_entry_kind now do that. chmod reads ENOTSUP and EOPNOTSUPP on the follow_symlinks=False path as the modifier being unavailable rather than as an OS error (interp_posix.py:1247-1251): a host can accept AT_SYMLINK_NOFOLLOW and not implement it, which is the same fact that makes HAVE_LCHMOD a narrower bit than HAVE_FCHMODAT. listdir and scandir given a descriptor report it as the failure's filename, since it is what named the directory. Found by the Codex parity review of this branch. Assisted-by: Claude
None of the five resolved a name against a directory descriptor, and none accepted the keyword at all: `open`, `mkdir` and `mkfifo` read their arguments positionally, so a call carrying any keyword reached them as a trailing dict and failed converting it to an integer; `rmdir`, `unlink` and `remove` were registered with a declared arity, which turns every keyword away. Each now binds its positional-or-keyword prefix through `bind_path_args` and reads `*, dir_fd=None` through `dir_fd_kwarg`, which spells `DirFD(available)` (`interp_posix.py:274-292`): `None` and an absent argument are the same default, and the value is converted before the platform is reported. The name form dispatches to `openat`, `mkdirat`, `mkfifoat` and `unlinkat` — the last with `AT_REMOVEDIR` for `rmdir` (`rposix.py:2717-2720`). `_have_functions` gains HAVE_MKDIRAT, HAVE_MKFIFOAT, HAVE_OPENAT and HAVE_UNLINKAT, which `os.py:124-132` reads into `supports_dir_fd`. HAVE_MKNODAT is not among them: `mknod` is still a placeholder that creates nothing. `chmod` and `chown` carried the same `dir_fd` block written out twice; both now call `dir_fd_kwarg`. The five path boundaries also name themselves when the argument is not a path — `unlink: path should be string, bytes or os.PathLike, not int`. extra_tests/parity_tests/os_dir_fd_modifiers.py exercises the modifier resolving against a descriptor rather than the working directory, the advertisement matching the behaviour, and the argument-list messages. Assisted-by: Claude
The three sat in the block of noop stubs that expects a real implementation
further down and had none, so each took any argument, made no syscall and
reported success. Their callers probe for presence and believe the answer:
`shutil.copystat` (`shutil.py:467`) reaches chflags through
`lookup("chflags")`, `tempfile._resetperms` through a `try: _os.chflags`, and
`tarfile.makedev` through `hasattr(os, "mknod")` — so the flags were never
copied and no fallback ran. The stub list is also not `#[cfg]`-gated, so
`hasattr(os, 'chflags')` was true on Linux, where the interface does not exist.
`mknod(path, mode=0o600, device=0, *, dir_fd=None)` is now `mknod`/`mknodat`,
registered beside `mkfifo` in the POSIX `host_env` block.
`chflags(path, flags, follow_symlinks=True)` and `lchflags(path, flags)` are
one call whose `follow_symlinks=False` arm is the second name, registered only
on the BSD-flavoured hosts that carry the pair. `<sys/stat.h>` declares
`lchflags` on the Apple targets, where `libc` carries only `chflags` and
`fchflags`, so it is named in an `extern` block.
The three names are dropped from the noop list, so a host without the call no
longer answers `hasattr`. `os.py:112-114` guards `_add` with `fn in _globals`,
and all three consumers above probe before calling.
`_have_functions` gains HAVE_LCHFLAGS (`os.py:182` → chflags in
supports_follow_symlinks) and HAVE_MKNODAT (`os.py:126` → mknod in
supports_dir_fd).
`bind_path_args` takes the keyword-only names rather than assuming `dir_fd`,
because a signature without a keyword-only tail counts a surplus argument
differently: every argument counts against the one limit and it is always
"at most", which is why `os.lchflags(p, 0, follow_symlinks=False)` is a count
error and not an unknown keyword.
HAVE_LCHMOD was spelled without the `host_env` half of the condition its
registration carries. Both it and the new HAVE_LCHFLAGS now read
`HOST_POSIX && BSD_FLAVOURED`, which is the invariant the table's own comment
states: each bit is the condition the entry point itself is compiled under.
`mknod` reports the failing name in `filename`, following
`interp_posix.py:1360-1372` `wrap_oserror2(space, e, w_path)`; CPython's
`os_mknod_impl` uses the pathless `posix_error()` there, as it does for
`mkfifo`, which pyre already named.
Assisted-by: Claude
The three sat in the noop-stub block and answered `None`. `tarfile` reads a node's pair out of `st_rdev` to write a header (`tarfile.py:2275-2276`) and puts one back together to recreate the node (`:2735`), so the header field it wrote was not a number. They are `libc::major`/`minor`/`makedev` (`interp_posix.py:2551-2563`), which is the host's own encoding rather than arithmetic that can be spelled portably — macOS splits a `dev_t` 8/24 and Linux 12/20 with the minor bits in two pieces. The device argument is narrowed to `dev_t` rather than to a C int, because that type is wider than an int where the pair is, and a value that does not fit reports the overflow instead of wrapping. `libc` is a shim under `feature = "sandbox"` and carries no `dev_t`, so the names are absent there rather than answering with another host's arithmetic. The remaining 40 lying names in that stub list are measured and filed separately. Assisted-by: Claude
`os.py:881` writes the spawn family in Python over fork+exec+waitpid, guarded
by `if _exists("fork") and not _exists("spawnv") and _exists("execv")`. A
`spawnv` bound here is therefore not a placeholder waiting to be overwritten
further down — it is what stops that definition from ever running, and the
noop stub won. `fork`, `execv`, `_exit`, `waitpid`, `WIFSTOPPED` and
`waitstatus_to_exitcode` are all real, so dropping the four names is the whole
implementation.
The same block defines P_WAIT and P_NOWAIT, and the constant list above bound
both to 0 — so the two modes were equal and `os.spawnv(os.P_NOWAIT, …)` waited
instead of returning a pid. POSIX has no spawn call and no such constants;
`<process.h>` does, so they are now bound on Windows only, with the values
`_spawnv` reads.
The sandbox build drops the four names too: the spawn family is app-level code
in os.py rather than an external, and binding a name there would take P_WAIT
and P_NOWAIT with it. `fork` is a raising stub in that build, so the definition
os.py provides refuses at the fork.
extra_tests/parity_tests/os_spawn_family.py spawns a child both ways and checks
that P_WAIT hands back the exit code where P_NOWAIT hands back a pid — which is
the assertion the two modes being equal fails.
Assisted-by: Claude
The four families sat in a list that bound every name to 0, under a comment saying zero stubs were fine for os.py init. os.py names none of them, so the zero was serving nothing; the readers are the callers. `os._exit` takes an EX_* straight to the exit status, `statvfs(...).f_flag` is masked with the ST_* bits, and the RTLD_* set is handed back to `dlopen`, where a zero asks for `RTLD_LOCAL | RTLD_LAZY` whatever the caller named. Within each family every member also compared equal to every other. EX_* are `<sysexits.h>` literals: `libc` binds none of them, and the header is a verbatim descendant of the 4.3BSD one wherever it is carried, so the values are the same on every host that has it. The rest come from `libc`, except the Apple scheduling policies — `<pthread/pthread_impl.h>` declares those and the crate does not mirror them. They are bound on the POSIX builds, which is where the headers are; SCHED_BATCH and SCHED_IDLE narrow to Linux and RTLD_DEEPBIND to glibc, as `rposix.py:296-300` and `rdynload.py:50-82` read them. Before this they were bound unconditionally, so the Windows and wasm builds carried names whose header their host does not have. `host_seam::sys` names each one, because the sandbox build reaches `libc` through that facade rather than directly. WNOHANG, WCONTINUED, WUNTRACED and the PRIO_* trio stay in the zero list: the POSIX blocks further down overwrite those with the real values, and the comment above them now says so. extra_tests/parity_tests/os_constants.py checks each family for the shape it has to have — the members distinct, the flag members single bits — which is what binding a whole family to one value destroys. Assisted-by: Claude
Seventeen names in the noop-stub list answered `None` and were never given a body. They fall into four kinds, and none of the four is a name the module should carry. `fstatat`, `faccessat`, `futimens`, `futimes` and `fdopendir` are the C entry points the calls above them are served with — `openat` and its family are how `dir_fd` and a descriptor path are honoured, not calls of their own — and `setenv` is the C spelling of `putenv`. `moduledef.py` publishes none of them. A name bound for one is a capability a caller probes for and believes. `pipe2`, `dup3` and the four scheduling-policy calls are Linux's own additions, and were bound on every host. Nothing serves them on any host here, so they are dropped rather than kept as a stub that reports success; the Linux bodies — `sched_getparam` and `sched_setparam` need a `sched_param` type that does not exist here yet — are filed as their own task. `WEXITED`, `WNOWAIT` and `WSTOPPED` are `waitid`'s option flags, which are numbers rather than calls. They are bound with the other wait options, from libc. The fourth kind is the one `spawnv` was: names os.py writes in Python and lists in its own `__all__`. `popen` (os.py:1020-1067) and `get_exec_path` (os.py:649) were both in the stub list; `getenv` (os.py:818-825) had a real body here, and the SEEK_SET/SEEK_CUR/SEEK_END trio came off the constants table that os.py fixes at 0/1/2 itself (os.py:203-206). os.py's definitions win — unlike the spawn family, none of these four is guarded on the name being free — so what the bindings changed was `os.__all__`, where each arrived twice: once through `_get_exports_list` and once through os.py's own list. `os.__all__` held six duplicates against CPython's none; it now holds none. `popen` leaves the sandbox build's raising-stub list for the same reason the spawn family did: os.py builds it over `subprocess`, whose fork the stubs beside it already refuse. extra_tests/parity_tests/os_module_surface.py checks `os.__all__` for a name listed twice, which is what catches this whole class at once, and checks that each C entry point is absent and each option flag is a number. Assisted-by: Claude
The three answered `None` from the stub list, which is a number a caller cannot tell from a group id and a name it cannot tell from a terminal. `getpgrp` (`interp_posix.py:2167-2172`) cannot fail and so is not checked; `getpgid` (`:2201-2210`) can be asked about a process that is not there and reports it. `ctermid` (`:2603-2608`) is read the way `rposix.py:1724-1728` reads it — the call is handed a null pointer and answers the static buffer it keeps — and the result is a filename, so it is decoded through `fsdecode_filename_bytes` rather than assumed to be text. `<stdio.h>` declares `ctermid` on every POSIX host, and the `libc` crate carries it for a handful, so the declaration is spelled out where the crate has none — the same shape `lchflags` already uses here. The sandbox build refuses all three instead: a process group and the controlling terminal's name are host facts, and the neighbouring reads (`getpid`, `getppid`, `ttyname`, `tcgetpgrp`) are refused there for the same reason. `confstr` and `confstr_names` stay stubs. They need the `_CS_*` table, which the `libc` crate carries one entry of per host while the module publishes 17 on Darwin and about fifty on glibc; spelling the rest out is what `pathconf_names` already does here, and it is filed as its own task rather than written from values this host cannot check. extra_tests/parity_tests/os_process_group.py checks the two group calls against each other and the terminal name for being a path, which is what a `None` fails. Assisted-by: Claude
…rectory cleanup `run_bench`'s `wasm_float_tol` and `min_pypy_ratio` are now keyword-only. Ruff reports FBT002 for the first; no call site passed either positionally. Ten parity scripts called `tempfile.mkdtemp` and never removed the result, so every run of the suite left a directory behind — three runners over 196 scripts. `atexit.register(shutil.rmtree, ...)` covers the early `raise SystemExit` on Windows and every assertion failure without wrapping each script in a `try`/`finally`. The #1078 review named the two in that diff; the other eight are the same line. os_supports_dir_fd.py's composed dir_fd + follow_symlinks call wrote `LINK_MTIME + 1`, one nanosecond past a whole second, against a file that states two lines above that every timestamp is a whole second so a coarse filesystem still reads back what was written. It also could not tell "the call did nothing" from "the value rounded down", because the step before it had already written `LINK_MTIME`. It writes a second whole-second pair instead. Assisted-by: Claude
Four answers from the #1078 review that were the wrong value rather than the wrong call. `pathconf`'s fifth rides with the conf* tables, which it shares a helper with. `truncate` opened the name it was given with a bare `libc::open`. The name can be a FIFO with no reader, and the call waited there holding the interpreter, so no other thread could reach the other end; it also reported an interrupted open as `InterruptedError` where the `ftruncate` beside it retried. It now goes through `call_external_function` under the same retry loop, which is what `interp_posix.py:418` reaches by opening through the module's own `open`. The close is no longer discarded: `interp_posix.py:429-431` closes in a `finally`, through a `close` that raises, so a writeback error the close is first to see is the caller's. The truncation's own failure still wins when both fail. `truncate_length_w` narrowed to `off_t` with an `as` cast. A length wider than `off_t` became a different length rather than an error, and the file was truncated to that; `off_t::try_from` reports it with the message the helper already had for a too-wide value. `utime` carried both timestamps as `std::time::Duration`, which has no second below the epoch, so every pre-epoch time was refused with "timestamp out of range". `rposix.futimens` and `rposix.utimensat` (`rposix.py:2634-2671`) keep the seconds and the nanoseconds apart and signed; this does the same, with the floor-division `_PyTime_ObjectToTimespec` applies, so `ns=(-1, -1)` is `(-1, 999999999)` and reads back as `-1`. The name form now calls `utimensat` directly rather than `rustpython_host_env::posix::set_file_times_at`, whose signature cannot carry a negative second — which is also what `interp_posix.rs:3136` already said it did. The Windows host call still counts upwards from the epoch, and turns a pre-epoch time away rather than writing a different one. `times` is accepted as a keyword. `interp_posix.py:1862` puts `__kwonly__` after `w_times`, so it is the one argument here a caller may spell either way, and it was positional-only. extra_tests/parity_tests/os_utime_pathconf_truncate.py pins these against CPython 3.14, together with pathconf's answer. Assisted-by: Claude
`BorrowedFd::borrow_raw` documents one value it may not be given: `-1`, which the standard library reserves as the niche that makes `Option<BorrowedFd>` cost nothing. Ten call sites built one straight out of an `fd` argument the caller supplied, and `os.fchmod(-1, 0o644)`, `os.chown(p, -1, -1, dir_fd=-1)` and `os.sendfile(-1, ...)` are all reachable from Python. `fd_borrow` answers those with the `EBADF` the syscall would have answered with, so the observable behaviour is unchanged and the one integer that may not become a handle no longer does. The sites reading a descriptor the module itself just produced — `dup2`'s result, and `path.as_fd` past its own `!= -1` guard — are left alone; those are not caller values. Reported in the #1078 review. Assisted-by: Claude
Both were noop stubs: `confstr_names` answered `None` rather than a dict, and `confstr` answered `None` whatever it was asked. `posixmodule.c posix_constants_confstr` and `rposix.py:2248-2300` name the same candidate set, every entry `#ifdef`-guarded, so a host publishes exactly the names its own `<unistd.h>` defines. `libc` carries `_CS_PATH` and nothing else, and the two numberings disagree from that first entry on — 1 on the Apple targets, 0 in glibc's `bits/confname.h`, whose enum also restarts twice, at 1000 and at 1100. Both tables are written out for that reason: 17 names on Darwin, 27 on glibc. The ten the candidate set carries for the System V hosts are defined by neither header, so neither table has them. The tables were derived by crossing `posix_constants_confstr` with each host's header rather than written from memory. The same derivation over the macOS SDK reproduces CPython 3.14's own `os.confstr_names` on this host entry for entry, which is what says the method is right. The glibc half comes from that project's `bits/confname.h`, and every value in it is confirmed a second time by `libc`: it carries `_CS_PATH` 0, `_CS_GNU_LIBC_VERSION` 2 and `_CS_GNU_LIBPTHREAD_VERSION` 3 outright, and it puts `_CS_POSIX_V6_ILP32_OFF32_CFLAGS` at 1116 — one past the end of the XBS5 run, which is what fixes that run at 1100..1115. No Linux compile was reached; this branch has none available. The call is `rposix.confstr` (`rposix.py:2129-2143`): ask for the length, fill a buffer of exactly that size. A zero length is either a name the host has no string for, which is `None`, or one it does not know, which is the errno it set — so errno is cleared before the question is put. The length counts the terminator and the string does not, so `len - 1` bytes are decoded, the way `os_confstr_impl` does; `rffi.charp2strn(buf, n)` keeps it. The value can be a search path, so it is decoded through the filesystem handler. `confname_arg` now takes the table to resolve against, and the dict-building it shares with `pathconf_names` is one function. `pathconf` and `fpathconf` come with it, because the fix lands in the lines that refactor touched. Both answered `None` where the host has no determinate limit; `interp_posix.py:2433` hands whatever `pathconf` returned to `space.newint`, so the answer is the number `-1`. `PC_ASYNC_IO` and `PC_SYMLINK_MAX` are the names that reach it on hosts that do not implement them, and `None` is neither that value nor a type a caller can compare against a limit. Reported in the #1078 review; pinned by extra_tests/parity_tests/os_utime_pathconf_truncate.py. The sandbox build refuses `confstr`: the answer is the host's own search path among other strings, and `pathconf`, `fpathconf` and `sysconf` are refused there beside it. extra_tests/parity_tests/os_confstr.py checks what holds on any host that has the call — the names resolve, the values are distinct, CS_PATH is a real search path, and an unknown name is refused. Assisted-by: Claude
None of the three existed. All are POSIX rather than Linux-only — the Apple targets carry every one of them — so they were absent on a host that has them. `lockf` is `interp_posix.py:3006-3012`: one call under the `eintr_retry` loop, put through the call gate because `F_LOCK` waits. Its four commands are published beside it. It answers `None`, which is what `os_lockf_impl` does; `interp_posix.py:3012` answers the `0` the call returns on success, and 3.14 — the oracle the parity suite reads — does not carry it. `waitid` is one `interp_posix.py:1722` names and does not have, so the shape is CPython 3.14's: a five-field `waitid_result` structseq, the three `P_*` id types and the six `CLD_*` codes. A zero `si_pid` is the "nothing to report" answer and is `None` rather than a result of zeroes. `WEXITED`, `WSTOPPED` and `WNOWAIT` were already published. `SEEK_HOLE` and `SEEK_DATA` are the two `whence` values beyond the three os.py fixes itself. The hosts whose headers define them are named rather than excluded, so a host left out is one short of a name rather than one carrying a wrong value. The sandbox build refuses `lockf` and `waitid`; their constants are numbers and stay. extra_tests/parity_tests/os_lockf_waitid_seek.py locks a region, walks a file with both new whence values, and reports a child with WNOWAIT before reaping it — which is the difference between waitid and waitpid. Assisted-by: Claude
…table
The constant block that binds EX_*, ST_*, RTLD_* and SCHED_* was registered
twice: once before the `nt` constants and once after, the second copy carrying
the F_* and SEEK_HOLE/SEEK_DATA rows the first did not. Both bound the same
values, so nothing observed the duplication. The first copy is removed.
os.EX_OK is not one of the names Windows has not got — CPython answers it there
with the same 0 — so it is bound with the `nt` constants as well. Only that one
member of the family; the rest of `<sysexits.h>` stays where the header is.
os.spawnv and os.spawnve go back to being registered on Windows. The reading
that unbinds them is the POSIX one: os.py:881 writes the spawn family over
fork+exec+waitpid and does so only `if not _exists("spawnv")`, so a binding
there prevents the definition. That block is behind `_exists("fork")`, which
Windows does not have, and `nt` carries `_spawnv` itself — so on Windows the
name is the module's or it is nowhere.
host_seam::sys orders the two scheduling `pub use` lines as rustfmt does.
Assisted-by: Claude
Each of these asserted on Windows something only a POSIX host answers, and the Windows runners are what reported it. - os_constants.py expected the whole `<sysexits.h>` family to be absent there. EX_OK is not: CPython binds it on Windows too, with the same 0. - os_spawn_family.py checked `P_NOWAIT == P_NOWAITO` before the Windows exit. That equality is os.py's, written in the branch that has fork; the C runtime numbers P_NOWAITO 3. - os_path_argument_types.py pinned listdir's TypeError with `integer` in the list of types it takes. The word is there for the descriptor form, which the Windows build has no fdopendir to serve. - os_utime_pathconf_truncate.py wrote a timestamp before the epoch. A FILETIME counts 100ns ticks, so CPython reads -1ns back as -100, and this build's Windows path carries the timestamp as an unsigned duration and refuses the range outright. winreg_key_lifecycle.py removed the saved hive by name and then rmdir'd its directory. The registry writes its transaction log beside the hive under names of its own, so the directory was not empty; it is emptied instead. Assisted-by: Claude
460580b to
ea93329
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/ea933298adeb7f6a1758188d44a3d68c32b1ba2d/pyre-interpreter/src/module/posix/interp_posix.rs#L2512
When the newly supported descriptor form is called as os.utime(fd) without times or ns, this passes a userspace snapshot of the current time to futimens instead of preserving the “now” state as UTIME_NOW. On POSIX, setting both timestamps to now is permitted when the caller has write access, while supplying explicit timestamps generally requires ownership, so a writable descriptor for a file owned by another user can now fail with EPERM; carry the omitted-time state through to utime_fd and emit UTIME_NOW, as upstream do_utimens does.
AGENTS.md reference: AGENTS.md:L231-L233
https://github.com/youknowone/pyre/blob/ea933298adeb7f6a1758188d44a3d68c32b1ba2d/pyre-interpreter/src/module/posix/interp_posix.rs#L5282-L5285
When ftruncate is interrupted by a signal, this loop retries immediately without invoking checksignals_now; crt_call! is only a raw libc-call wrapper and does not provide the call gate claimed above. Consequently, a periodic signal whose Python handler raises or disables the timer can repeatedly interrupt the newly implemented os.truncate(path, ...) while its handler never gets a turn, delaying the exception or livelocking the call. Use the same eintr_retry_with handling used by open below before retrying.
AGENTS.md reference: AGENTS.md:L231-L233
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
The descriptor and
dir_fdforms of the posix entry points that advertisethem, plus the one ratio ceiling #1071 did not reach.
Rebased onto
origin/mainafter #1071 landed. Four commits from the earlierrevision are gone because #1071 supersedes them:
spectral_norm's statedfloor (main states it at 0.25 from the runners' 0.5x–4.3x span, against my 0.4
from a partial reading), seven of the eight ceilings this branch had raised,
and a
type_immutable_rejectN-sizing that contradicts the shape main landed.posix — the capability sets and what stands behind them
os.supports_fd/supports_dir_fd/supports_follow_symlinksare whatcallers like
shutil.copystatandshutil.rmtreeread to choose anfd-relative implementation over a path-based one. A bit set where the entry
point rejects the argument sends the caller down a route that cannot work; a
bit clear where the entry point implements it costs the caller a fallback it
did not need.
chownalready reachedfchownat, but withAT_FDCWDhard-coded anddir_fdrejected withNotImplementedErrorone step earlier. The descriptorthe caller names is now what the name resolves against.
HAVE_FCHOWNATandHAVE_UTIMENSATjoin_have_functions. Neither waslisted, so os.py put
chownandutimein neithersupports_dir_fdnorsupports_follow_symlinkswhile both entry points implemented the modifiers.HAVE_LCHOWN/HAVE_LUTIMESstay unlisted for the reasonHAVE_FUTIMESalready carried: os.py reads them as the same capability their
*atsiblingprovides, and neither
lchownnorlutimesis called here.supports_fdnames that rejected an integer,os.truncate(advertised and doing nothing),fstaton a Windows descriptor,and
getcwddecoded with the filesystem handler.This is not only a missed fallback. Two lib-python tests move from failing to
passing, with no other row moving across 599 tests in the two modules:
test_os.UtimeTests.test_utime_invalid_argumentsassertedNotImplementedErrorforfollow_symlinks=Falsebecauseutimewas absentfrom
supports_follow_symlinks— and pyre did not raise.test_shutil.TestMisc.test_chownhit theNotImplementedErrorfromshutil.chown(dir_fd=...).Two parity scripts pin the surface against CPython 3.14.
os_supports_fd.pycalls every member of that set with a descriptor and observes the result rather
than trusting the return value;
os_supports_dir_fd.pydoes the same for theother two sets, with every
dir_fdcall repeated against a name that does notexist under the descriptor — which is what separates "resolved against dir_fd"
from "ignored it and reached the same file through the cwd".
bench — the ceiling #1071 left behind
getframe_force_cancel_journalcrossed its ceiling of 29 on the ubunturunner's cranelift arm in both main CI runs on record (31.5x and 32.1x in the
median-3 fail detail, 30.8x/32.6x in the tables), and #1071 widened the rest of
the clamped family without touching it. It is held to #1071's own rule: ceiling
twice the slowest ratio a runner reported (32.6x), floor half the fastest
(8.0x), both stated because the derived floor would sit above the measurement.
One observation for a future adjustment:
pickle_ctor_args's new header statesa span of 18.6x–70.3x, but the windows runner read 101.5x (table 103.0x) at
fcf997da0e0. 145 still clears it, so nothing fails — but that reading is thewindows regression tracked separately, and widening the ceiling on top of a
regression is not done here.
Verification
pyre/extra_tests/parity_tests/run.py— 193 scripts, cpython 3.14 / dynasm /cranelift all OK, runner exit code 0.
python3 pyre/check.py— zero ratio or perf gate failures.branch: a control binary built from origin/main's posix source reproduces six
representative deviations with identical counts (
bridges_compiled 18 -> 8; guard_failures 3610 -> 1605etc.), so they are this host's known deviationfamily. Nothing was re-recorded — improvements and
bridges_compiledregressions are mixed, and the cause has to be settled before a baseline
moves.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
confstr,waitid,lockf, device-number helpers, sparse-file seeking, and additional platform constants.Bug Fixes
Tests