Update nix to 0.30 #6120
Update nix to 0.30 #6120
Conversation
WalkthroughBumps nix to 0.30, adds Owned::leak to crt_fd, and migrates many raw i32/RawFd usages to typed BorrowedFd/OwnedFd/MaybeFd across mmap, posix subprocess, posix, signal, and other unix call sites; posix_spawn is reimplemented using nix::spawn and several public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python caller
participant M as mmap::py_new
participant CRT as crt_fd (Borrowed/Owned)
participant OS as OS (fstat/mmap)
Py->>M: py_new(MmapNewArgs{fileno: i32, ...})
alt fileno >= 0
M->>CRT: Borrowed::try_borrow_raw(fileno) -> Borrowed
CRT->>CRT: duplicate -> Owned (new_fd)
M->>OS: fstat(Borrowed)
M->>OS: mmap/map_* using &new_fd
M-->>Py: PyMmap{ fd: AtomicCell(new_fd_raw) }
else anonymous
M->>OS: map_anon(...)
M-->>Py: PyMmap{ fd: AtomicCell(-1) }
end
sequenceDiagram
autonumber
participant Py as Python caller
participant F as fork_exec(ForkExecArgs<'fd'>)
participant K as KeepFds
participant N as dup helpers / nix::spawn
participant EP as errpipe (Fd)
Py->>F: fork_exec(ForkExecArgs<'fd'>)
F->>K: build keep list (&[BorrowedFd])
F->>N: set_inheritable / dup stdio via BorrowedFd/MaybeFd
F->>EP: prepare errpipe (Fd)
alt child path
F->>N: exec(&ForkExecArgs<'_'>, ProcArgs)
N->>N: perform dup/close using BorrowedFd/OwnedFd
N->>N: call nix::spawn with file actions/attrs
else parent path
F-->>Py: pid or error from EP
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧬 Code graph analysis (2)stdlib/src/mmap.rs (3)
stdlib/src/posixsubprocess.rs (1)
🔇 Additional comments (15)
✨ 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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
common/src/crt_fd.rs (1)
193-195: Clarify/strengthen semantics of Owned::leak to avoid misuseThis safely returns a Borrowed with an arbitrary lifetime while intentionally “leaking” the fd. Consider making the lifetime explicit and signaling the leak via 'static, plus marking it must_use, to reduce accidental misuse.
Suggested change:
- pub fn leak<'fd>(self) -> Borrowed<'fd> { - unsafe { Borrowed::borrow_raw(self.into_raw()) } - } + /// Intentionally leaks the underlying file descriptor and returns a borrowed + /// view of it. The fd will not be closed automatically; the caller is + /// responsible for closing it later (e.g. via crt_fd::close on a reconstructed Owned). + #[must_use] + pub fn leak(self) -> Borrowed<'static> { + unsafe { Borrowed::borrow_raw(self.into_raw()) } + }Alternatively, keep the generic lifetime but document the safety expectations prominently.
stdlib/src/mmap.rs (1)
180-180: Consider adding documentation for the fd field change.The change from
RawFdtoAtomicCell<i32>is part of the migration to typed FD wrappers. The field now stores-1for anonymous mappings and the raw FD value otherwise.Consider adding a doc comment to clarify the sentinel value:
+ /// The file descriptor for this mmap. -1 for anonymous mappings. fd: AtomicCell<i32>,
📜 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 ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(1 hunks)common/src/crt_fd.rs(1 hunks)stdlib/src/mmap.rs(6 hunks)stdlib/src/posixsubprocess.rs(8 hunks)vm/src/stdlib/io.rs(1 hunks)vm/src/stdlib/os.rs(1 hunks)vm/src/stdlib/posix.rs(17 hunks)vm/src/stdlib/signal.rs(1 hunks)
🧰 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/os.rscommon/src/crt_fd.rsvm/src/stdlib/io.rsvm/src/stdlib/signal.rsstdlib/src/mmap.rsstdlib/src/posixsubprocess.rsvm/src/stdlib/posix.rs
🧬 Code graph analysis (6)
vm/src/stdlib/os.rs (1)
vm/src/stdlib/posix.rs (1)
dup(1679-1684)
vm/src/stdlib/io.rs (1)
stdlib/src/fcntl.rs (1)
fcntl(59-90)
vm/src/stdlib/signal.rs (1)
common/src/crt_fd.rs (3)
try_borrow_raw(260-266)as_raw(184-186)as_raw(269-271)
stdlib/src/mmap.rs (3)
common/src/crt_fd.rs (1)
try_borrow_raw(260-266)vm/src/stdlib/posix.rs (1)
dup(1679-1684)common/src/fileutils.rs (2)
fstat(11-21)fstat(98-162)
stdlib/src/posixsubprocess.rs (2)
vm/src/stdlib/posix.rs (14)
try_from_object(192-200)try_from_object(204-212)try_from_object(693-697)try_from_object(1178-1180)try_from_object(1184-1186)try_from_object(1822-1834)try_from_object(2216-2231)obj(1153-1161)obj(1823-1823)obj(2217-2217)pipe(896-902)set_inheritable(6-15)set_inheritable(866-868)dup(1679-1684)common/src/crt_fd.rs (14)
as_raw_fd(74-76)as_raw_fd(107-109)as_raw_fd(221-223)as_raw_fd(297-299)into_raw_fd(78-81)into_raw_fd(235-237)as_fd(214-216)as_fd(290-292)from(200-202)from(207-209)from(276-278)from(283-285)borrow_raw(100-105)borrow_raw(247-250)
vm/src/stdlib/posix.rs (1)
vm/src/stdlib/nt.rs (1)
set_inheritable(93-100)
⏰ 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). (10)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
🔇 Additional comments (41)
Cargo.toml (1)
187-187: nix 0.30 bump looks good; feature set still validThe enabled features (fs, user, process, term, time, signal, ioctl, socket, sched, zerocopy, dir, hostname, net, poll) match nix’s current feature list. The API change toward I/O-safe FDs (AsFd/BorrowedFd) also aligns with 0.30.
Refs: nix 0.30 features and modules; fcntl now generic over AsFd. (docs.rs, github.com)
vm/src/stdlib/io.rs (1)
3975-3976: Pass AsFd to nix::fcntl::fcntl — correct for 0.30’s I/O-safe APIUsing Fildes (implements AsFd) directly matches nix 0.30’s signature for fcntl.
Refs: fcntl now generic over AsFd. (github.com)
vm/src/stdlib/signal.rs (1)
273-282: Typed-FD nonblocking check is correct and clearerBorrowing via crt_fd::Borrowed::try_borrow_raw and calling nix::fcntl with the typed fd matches nix 0.30’s API; the error message now prints the raw fd explicitly, which is helpful.
Refs: nix 0.30 fcntl AsFd; features include signal/fcntl. (github.com, docs.rs)
vm/src/stdlib/os.rs (1)
381-381: LGTM!The removal of
fno.as_raw()when callingnix::unistd::dupis correct. According to the nix 0.30 changelog and the broader migration to IO-safe types,dupnow acceptsBorrowedFddirectly instead of raw file descriptors.stdlib/src/mmap.rs (6)
30-30: LGTM!The addition of
rustpython_common::crt_fdimport is appropriate for the migration from raw file descriptors to typed FD wrappers.
341-342: LGTM!The conversion to typed FD handling using
crt_fd::Borrowed::try_borrow_rawis correct. This properly converts the raw FD to a borrowed FD wrapper, returningNonefor the sentinel value-1(anonymous mappings).
368-381: LGTM!The refactored FD handling correctly:
- Duplicates the FD using
unistd::dupto get anOwnedFd- Maps the memory using the new FD via typed references
- Returns the owned FD for lifecycle management
- Falls back to anonymous mapping when no FD is provided
388-388: LGTM!The FD storage correctly uses
map_or(-1, |fd| fd.into_raw())to store the sentinel value-1for anonymous mappings or the raw FD value otherwise.
843-844: LGTM!The size method correctly retrieves the FD using
try_borrow_rawand usesfstatto get the file size, consistent with the new typed FD approach.
343-344: fstat import verified
Theuse nix::sys::stat::fstat;statement exists on line 27, sofstat(fd)is properly imported and available.stdlib/src/posixsubprocess.rs (11)
21-21: LGTM!The import of FD-related types from
std::os::fdis appropriate for the migration to typed file descriptors.
35-35: LGTM!The function signature correctly uses the lifetime parameter
'fdfor theForkExecArgsto ensure proper FD borrowing.
61-61: LGTM!The struct correctly includes a lifetime parameter
'fdto track the lifetime of borrowed file descriptors.
123-167: LGTM!The new
FdandMaybeFdtypes provide proper abstraction for FD handling:
Fdwraps aBorrowedFd<'static>for valid FDsMaybeFdhandles the sentinel value-1case- Proper trait implementations for
AsRawFd,IntoRawFd,AsFd, andWrite
201-211: LGTM!The field types have been correctly updated to use the new FD wrappers:
fds_to_keepnow usesBorrowedFd<'fd>for proper lifetime tracking- All pipe FDs use
MaybeFdto handle the-1sentinel case- Error pipe FDs use
Fdas they must be valid
230-238: LGTM!The exec function correctly:
- Takes a reference to
ForkExecArgsto avoid moving- Uses the error pipe FD directly without creating a
File- Properly writes error information to the pipe
264-266: LGTM!The code correctly uses
posix::set_inheritablewith the borrowed FD and compares raw FD values for the error pipe check.
297-311: LGTM!The
dup_into_stdiohelper function is well-designed:
- Properly handles the
MaybeFdenum- Sets inheritance for FDs that are already at the target
- Uses the provided closure for duplication when needed
377-387: LGTM!The
KeepFdsstruct andshould_keepmethod correctly:
- Use
BorrowedFdwith proper lifetime tracking- Convert to raw FD only for comparison in binary search
- Properly check if an FD should be kept based on the criteria
483-490: LGTM!The brute force FD closing correctly:
- Chains the FDs to keep with proper raw FD extraction
- Uses
tuple_windowsto generate ranges between kept FDs- Safely closes FDs using
libc::close
276-283: Potential issue with FD duplication in stdin case.When
c2pwriteis FD 0 (stdin), the code duplicates it and sets it as inheritable. However, there's a potential issue: ifdupfails, the function returns early without proper cleanup.Consider handling the error case more gracefully:
let c2pwrite = match args.c2pwrite { MaybeFd::Some(c2pwrite) if c2pwrite.as_raw_fd() == 0 => { - let fd = unistd::dup(c2pwrite)?; - posix::set_inheritable(fd.as_fd(), true)?; - MaybeFd::Some(fd.into()) + match unistd::dup(c2pwrite) { + Ok(fd) => { + posix::set_inheritable(fd.as_fd(), true)?; + MaybeFd::Some(fd.into()) + } + Err(e) => return Err(e), + } } fd => fd, };Likely an incorrect or invalid review comment.
vm/src/stdlib/posix.rs (20)
4-4: Adopt io-safe FDs in imports — LGTM.
6-15: Helper for inheritable flag is correct and side-effect free.
Uses F_GETFD/F_SETFD with FD_CLOEXEC as expected; no redundant syscalls when flags unchanged.
46-47: Importing OwnedFd/BorrowedFd/AsFd aligns the module with nix 0.30 IO-safety.
386-388: fchdir now accepts BorrowedFd — good parity with nix’s signature.
437-440: fchown[at]/fchown conversions are correct with DirFd::get() and BorrowedFd.
857-863: get_inheritable logic is correct.
Checks FD_CLOEXEC bit directly; result maps to Python bool properly.
866-868: Wrapper delegates to the shared helper — nice reuse.
871-877: get_blocking correctly inspects O_NONBLOCK.
880-893: set_blocking toggles O_NONBLOCK correctly and avoids redundant F_SETFL.
896-902: pipe sets non-inheritable on both ends — matches PEP 446 expectations.
942-946: _fchmod takes BorrowedFd and uses nix::sys::stat::fchmod — good migration.
983-985: fchmod wrapper is correct passthrough to _fchmod.
1223-1228: openpty makes both ends non-inheritable — good catch.
1424-1461: posix_spawn: file_actions mapping looks right.
- Open: uses OFlag::from_bits_retain and Mode::from_bits_retain (acceptable given platform variance).
- Close/Dup2: straightforward mapping to nix’s file actions.
Error mapping to IOErrorBuilder with filename context is a nice touch.
1464-1467: posix_spawn: attributes and flags wiring is sound.
- setsigdef/setsigmask: builds SigSet then sets flags.
- setpgroup/resetids: correct flags.
- setsid: good cfg-gated support with a clear NotImplemented on unsupported targets.
Also applies to: 1478-1482, 1485-1489, 1492-1493, 1496-1511, 1524-1528, 1538-1540
1552-1559: posix_spawn/p: result conversion and error mapping — LGTM.
1679-1684: dup returns OwnedFd and enforces non-inheritability — correct per PEP 446.
1687-1694: Dup2Args ownership model is sensible for nix 0.30’s dup2 API.
BorrowedFd for source and OwnedFd for target enables safe in-place replacement.
1697-1705: Align dup2 return value with CPython or document deviation. CPython’s os.dup2 returns None; here you return the new file descriptor. Either change this to return None to match CPython or explicitly note the intentional change in your docs/CHANGELOG.
373-375: Confirm thatDirFd::get()yields a raw file descriptor (RawFd) as required bynix::unistd::symlinkat, not a wrapper type.
| } | ||
|
|
||
| #[derive(Copy, Clone)] | ||
| enum MaybeFd { |
There was a problem hiding this comment.
Wouldn't it be more correct to have:
type MaybeFd = Option<(FD type)>or maybe even a newtype:
struct MaybeFd<T>(Option<T>)?
There was a problem hiding this comment.
In this case, None corresponds to -1, not PyNone
There was a problem hiding this comment.
ah, I see. Then this is probably the best I can think about atm
There was a problem hiding this comment.
Is Some/None naming the best? It looks a bit confusing.
If it needs option-like operations, #[derive(result_like::OptionLike)] will be helpful.
Then this code also work
if let Some(value) = maybe_fd.into_option() {
...
}
There was a problem hiding this comment.
Good point. I've renamed the variants, which I think makes it more clear.
youknowone
left a comment
There was a problem hiding this comment.
looks good. please check the comment before merge
1b25724 to
3b60133
Compare
Now much more of nix uses IO-safe types!
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores