Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(exa PR) 1190: Fix device major minor numbers #13

Closed
wants to merge 7 commits into from
Closed

Conversation

cafkafk
Copy link
Member

@cafkafk cafkafk commented Jul 29, 2023

SteveLauC and others added 3 commits May 2, 2023 22:08
This is meant as the final step in fixing ogham/exa#1126.

The different build targets of Rust libc's `major()` and `minor()`
functions have conflicting return types like `c_uint`, `c_int` and
`i32`, so an `i64` should be able to represent all of them.
@cafkafk cafkafk changed the title (exa PR) 1190 (exa PR) 1190: Fix device major minor numbers Jul 29, 2023
@cafkafk cafkafk self-assigned this Jul 29, 2023
src/fs/file.rs Show resolved Hide resolved
Comment on lines +197 to +198
pub major: i64,
pub minor: i64,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering libc defines pub type c_uint = u32;, I don't see the reason for using i64, would this ever be negative? https://docs.rs/libc/latest/libc/type.c_uint.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be to make space for this difference:

# Linux
pub type dev_t = u64;
pub fn major(dev: ::dev_t) -> ::c_uint;
pub fn minor(dev: ::dev_t) -> ::c_uint;

# macOS
pub type dev_t = i32;
pub fn major(dev: dev_t) -> i32;
pub fn minor(dev: dev_t) -> i32;

ogham/exa#1126 (comment)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this ever be negative?

For glibc, I guess it won't be.

Ref: how to interpret the dev_t type

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this ever be negative?

For glibc, I guess it won't be.

Ref: how to interpret the dev_t type

I guess the original intent might have been to have space enough for both pub fn major(dev: dev_t) -> i32; and pub fn major(dev: ::dev_t) -> ::c_uint; (u32)?

@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

This was super helpful to understand what's going on for future reference: ogham/exa#1126 (comment)

@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

In order to progress on this we'd need feedback from someone with an apple box

@cafkafk cafkafk marked this pull request as draft July 30, 2023 04:50
@SteveLauC
Copy link

In order to progress on this we'd need feedback from someone with an apple box

For macOS, I am not sure if the major can be negative, but the minor won't be, see:

https://github.com/rust-lang/libc/blob/28ab9b9e7bd04a5c5aca3f4d78583214f63d4002/src/unix/bsd/apple/mod.rs#L5186-L5196

@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

For potential testers:

cargo run --release -- -l /dev/ > changes.txt; exa -l /dev/ > old.txt; diff changes.txt old.txt;

@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

I found an apple laptop to test this on:

error[E0308]: mismatched types
--> src/fs/file.rs:366:39
|
366 | major: unsafe { major(device_ids) as i64 },
| ----- ^^^^^^^^^^ expected i32, found u64
| |
| arguments to this function are incorrect
|
note: function defined here
--> /Users/ces/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libc-0.2.147/src/unix/bsd/apple/mod.rs:5186:12
|
5186 | pub fn major(dev: dev_t) -> i32 {
| ^^^^^
help: you can convert a u64 to an i32 and panic if the converted value doesn't fit
|
366 | major: unsafe { major(device_ids.try_into().unwrap()) as i64 },
| ++++++++++++++++++++

error[E0308]: mismatched types
--> src/fs/file.rs:367:39
|
367 | minor: unsafe { minor(device_ids) as i64 },
| ----- ^^^^^^^^^^ expected i32, found u64
| |
| arguments to this function are incorrect
|
note: function defined here
--> /Users/ces/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libc-0.2.147/src/unix/bsd/apple/mod.rs:5190:12
|
5190 | pub fn minor(dev: dev_t) -> i32 {
| ^^^^^
help: you can convert a u64 to an i32 and panic if the converted value doesn't fit
|
367 | minor: unsafe { minor(device_ids.try_into().unwrap()) as i64 },
| ++++++++++++++++++++

For more information about this error, try rustc --explain E0308.
error: could not compile eza (bin "eza") due to 2 previous errors

@cafkafk cafkafk added not ready for PRs that aren't finished os › macos labels Jul 30, 2023
@sbatial sbatial mentioned this pull request Jul 30, 2023
63 tasks
@cafkafk cafkafk added this to the exa pulls done milestone Jul 31, 2023
@cafkafk
Copy link
Member Author

cafkafk commented Aug 4, 2023

This needs an apple user to work on these changes.

@cafkafk cafkafk removed not ready for PRs that aren't finished looking for testers needs feedback labels Aug 4, 2023
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

This pull request is stale because it has been open for 30 days with no activity.

@github-actions
Copy link

This pull request was closed because it has been inactive for 14 days since being marked as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants