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

harmonize dirent->d_ino according to posix #647

Closed
wants to merge 1 commit into from

Conversation

Mic92
Copy link
Contributor

@Mic92 Mic92 commented Jul 7, 2017

POSIX 2004 states that dirent shall include the field d_ino of type ino_t:
http://pubs.opengroup.org/onlinepubs/009695399/basedefs/dirent.h.html

Updated darwin/dragonfly/freebsd and netbsd definitions to met POSIX and make
portable use easier.

POSIX 2004 states that dirent shall include the field `d_ino` of type `ino_t`:
http://pubs.opengroup.org/onlinepubs/009695399/basedefs/dirent.h.html

Updated darwin/dragonfly/freebsd and netbsd definitions to met POSIX and make
portable use easier.
@Mic92
Copy link
Contributor Author

Mic92 commented Jul 7, 2017

motivation see nix-rust.

@semarie
Copy link
Contributor

semarie commented Jul 7, 2017

changing the name of dirent struct's member looks like a breaking change in libc. any Rust program relying on old name will not compile any more.

I am also a bit surprised that travis doesn't complain: a quick look at source code of BSDs show no d_ino field in dirent:

Maybe there is a #define somewhere ?

POSIX is only a specification, not all OSs follow it strictly.

@Mic92
Copy link
Contributor Author

Mic92 commented Jul 7, 2017

I would have made a define, however we have not equivalent in Rust. I suppose travis does not complain because binary compatibility is preserved. In the current form the definition it is unnecessary inconsistent also operating systems follow POSIX here from the semantics (Somebody just blindly copied the definition from the man pages). I can also work around this in nix in instead.

@Mic92
Copy link
Contributor Author

Mic92 commented Jul 7, 2017

It should be at least ino_t on all platforms.

@Mic92
Copy link
Contributor Author

Mic92 commented Jul 7, 2017

I fixed this in nix now. Once the pull request there is merged, I will also close this pull request:
nix-rust/nix@6ed79b0#diff-03012aeef26d4c7454d6dc08d0e31312R57

@alexcrichton
Copy link
Member

Thanks for the PR! As this is a breaking change we can't merge this at this time, but I'll tag it as such.

@bors
Copy link
Contributor

bors commented Feb 5, 2019

☔ The latest upstream changes (presumably #1217) made this pull request unmergeable. Please resolve the merge conflicts.

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 20, 2019

@Mic92 could you rebase this ?

Also, do you have a link to the headers of each of the OSes this PR modifies? I'd like to check the name of this field in those headers. libc should do what those headers do. Like @semarie says, not all OSes follow POSIX properly, and this library should reflect that (nix doesn't have to do that though).

@Mic92
Copy link
Contributor Author

Mic92 commented Feb 20, 2019

@gnzlbg I can rebase once, it becomes clear that it will be merged (i.e. 1.0 is approaching). I strongly disagree that this crate should use the same names as the underlying libc's. They have it for historically reasons and C allows to make backwards-compatible aliases using macros, which cannot be done in Rust. This only makes platform-independent development unnecessary difficult for not good reason.

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 21, 2019

This only makes platform-independent development unnecessary difficult for not good reason.

The only goal of this crate is to provide raw FFI bindings to the platform APIs "as is". This is particularly important for projects like bindgen, where they currently translate struct field names "as is" from the platform C code to Rust, and they expect that if the type exists in libc, its fields are named just like in the platform.

If we were to start translating platform API names here because we don't like their names, all C FFI binding generators would need to apply those exact translations to be able to use the libc crate.

I understand that working around platform incompatibilities is painful, and I understand that this has to be done in the nix crate to expose a portable API, but this is exactly what the nix crate is there for. If this was libc's goal, then arguably, the nix crate wouldn't have a reason to exist.

@Mic92 Mic92 closed this Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants