GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — host_env: Simplify stat, fstatat with Rustix by joshuamegnauth54 · Pull Request #8455 · RustPython/RustPython 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 30728473-f83e-44b2-88e7-d046b6050213
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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
as always, great finding!
ty<3
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5da13ae1-5f7a-4177-8be0-3eaf2ee864fc
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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Cargo shear failed randomly. 🤔 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I wish we didn't lose CPython compatible behavior by this
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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
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
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.