host_env: Simplify stat, fstatat with Rustix - #8455
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change replaces non-Windows libc stat operations with Rustix, moves ChangesFilesystem stat migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 `@crates/host_env/src/fileutils.rs`:
- Line 5: Update the NetBSD branch in the OS stdlib implementation to access
StatStruct fields st_atime_nsec, st_mtime_nsec, and st_ctime_nsec, replacing the
legacy st_atimensec, st_mtimensec, and st_ctimensec names while preserving the
existing timestamp behavior.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 30728473-f83e-44b2-88e7-d046b6050213
📒 Files selected for processing (6)
crates/host_env/src/fileutils.rscrates/host_env/src/io.rscrates/host_env/src/posix.rscrates/host_env/src/posix_unix_like.rscrates/host_env/src/posix_wasi.rscrates/vm/src/stdlib/os.rs
💤 Files with no reviewable changes (2)
- crates/host_env/src/posix_wasi.rs
- crates/host_env/src/posix.rs
ShaharNaveh
left a comment
There was a problem hiding this comment.
as always, great finding!
ty<3
b08a96b to
1c5a481
Compare
1c5a481 to
71d5994
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 `@crates/vm/src/stdlib/os.rs`:
- Around line 1237-1243: Fix the conditional compilation around the timestamp
assignments in the enclosing function by making the NetBSD condition consistent
across both adjacent cfg attributes. Remove NetBSD from the exclusion in the
second cfg so atime, mtime, and ctime are assigned for NetBSD while retaining
the existing WASI exclusion.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5da13ae1-5f7a-4177-8be0-3eaf2ee864fc
📒 Files selected for processing (6)
crates/host_env/src/fileutils.rscrates/host_env/src/io.rscrates/host_env/src/posix.rscrates/host_env/src/posix_unix_like.rscrates/host_env/src/posix_wasi.rscrates/vm/src/stdlib/os.rs
💤 Files with no reviewable changes (2)
- crates/host_env/src/posix.rs
- crates/host_env/src/posix_wasi.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/host_env/src/posix_unix_like.rs
- crates/host_env/src/fileutils.rs
- crates/host_env/src/io.rs
71d5994 to
62419ec
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. |
According to POSIX, `time_t` should be 64 bits. `musl` changed its `time_t` to an `i64` over five years ago. `glibc` provides compatibility features that declare `time_t` as either `i32` or `i64`. Rustix uses the raw Linux syscall for stat which returns an `i64`. For our purposes, an `i64` makes sense because it's modern and avoids the year 2038 problem. It also reduces our dependency on what `libc` defines. Sources: * https://www.man7.org/linux/man-pages/man3/time_t.3type.html * https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/sys_types.h.html
62419ec to
29ee814
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. |
|
Cargo shear failed randomly. 🤔 |
|
According to POSIX,
time_tshould be 64 bits.muslchanged itstime_tto ani64over five years ago.glibcprovides compatibility features that declaretime_tas eitheri32ori64. Rustix uses the raw Linux syscall for stat which returns ani64. For our purposes, ani64makes sense because it's modern and avoids the year 2038 problem. It also reduces our dependency on whatlibcdefines.Sources:
Summary
Summary by CodeRabbit
Bug Fixes
Refactor