Added inotify bindings. #1016
Added inotify bindings. #1016
Conversation
|
I updated the code taking your remarks into account, I also added some documentation and testing. I am not sure how to integrate my ffi code into libc, I would appreciate pointers on how to do that. |
|
Submitting a PR to libc is easy. Just checkout https://github.com/rust-lang/libc/ . Copy the format for existing code. The hardest part is sometimes figuring out which file to modify. If inotify is the same on all Linux architectures, then your changes should go into src/unix/notbsd/linux/mod.rs. |
|
I removed all the ffi code from the pull request and submitted a pull request in the libc. |
|
Could you please link this PR to the libc PR? |
|
Hi, I'm sorry for the delay, I was on vacation with very limited access to internet. The libc PR is at: rust-lang/libc#1230. |
|
The code I needed has been merged into the libc. |
|
The build is failing on Android because of undefined symbols. You'll have to figure out whether Android actually has those symbols or not. If so, then you should add them to libc. If not, then you can remove this module on Android. |
|
I'm having problems integrating the changes for android into libc; It is likely not as simple as I initially thought to support inotify for android. Maybe we can start by merging the support for Linux only? |
|
What's the problem? Does android use a different API? |
|
The Android API is mostly the same with one little exception: inotify_rm_watch takes a uint32_t as its second argument on Android and an int on Linux. Android: https://android.googlesource.com/platform/bionic/+/refs/heads/master/libc/SYSCALLS.TXT Linux: https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=sysdeps/unix/sysv/linux/sys/inotify.h;hb=HEAD |
|
That sounds pretty simple. What's the problem? |
|
Nothing really, it just took me a while to track down that slight difference in API. |
|
So the only difference between Linux and Android is signededness? Lame. Android is full of unforced errors like that. Anyway, this PR looks good to me! All it needs now is a squash. |
|
Yes just the signededness, do you know where this kind of errors comes from ? It seems very weird to me. |
|
Honestly, I don't know why Android has so many errors like that. There doesn't seem to be much excuse for it. |
|
I squashed my commits, seems like everything is ready for merging! |
|
Oh I forgot: you need a CHANGELOG entry too. |
|
The CHANGELOG looks good, but I'm afraid your commits have become unsquashed. Could you resquash them? |
|
Unfortunately that latested merge screwed up the PR history. I'm afraid you'll have to rebase onto master. |
|
Ah, I'm a little lost in my git history now. It might take me some time to figure everything out. |
|
Ok, I think the git history looks good now. |
|
bors r+ |
1016: Added inotify bindings. r=asomers a=vdagonneau Hi ! I needed inotify bindings and noticed that nix did not have any so here is a PR to add it. There was another PR from 2015 to add support for inotify that was never merged. I took some of the feedback and applied it here. Co-authored-by: Vincent Dagonneau <[email protected]>
Timed out |
|
bors retry |
|
Seems like something went wrong with bors. Any way I can help ? |
|
The evidence is gone now, but this might've been due to the Rust 1.33.0 problems. They're fixed now. bors retry |
1016: Added inotify bindings. r=asomers a=vdagonneau Hi ! I needed inotify bindings and noticed that nix did not have any so here is a PR to add it. There was another PR from 2015 to add support for inotify that was never merged. I took some of the feedback and applied it here. Co-authored-by: Vincent Dagonneau <[email protected]>
Build succeeded
|
Hi !
I needed inotify bindings and noticed that nix did not have any so here is a PR to add it.
There was another PR from 2015 to add support for inotify that was never merged. I took some of the feedback and applied it here.