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

Remake PollWatcher #12

Closed
passcod opened this issue Jun 6, 2015 · 20 comments
Closed

Remake PollWatcher #12

passcod opened this issue Jun 6, 2015 · 20 comments

Comments

@passcod
Copy link
Member

passcod commented Jun 6, 2015

For those not familiar with the background who want to help:

What was done:

  • Support was achieved for both Linux and OS X.
  • Windows native support (windows implementation #4) may be worked on.
  • The polling backend requires the ability to get the mtime of a file, which cannot currently be done in stable Rust, so it was dropped.

What is needed:

  • To get a replacement for stat.modified().
  • To implement anew (or adapt from the old code) a polling backend.
@passcod passcod added the Z-needs implementation Needs an implementation, will accept PRs label Jun 6, 2015
@octplane
Copy link
Contributor

octplane commented Jun 6, 2015

Hello,

Two ideas for that:

  • Finish integrating fsevent in the package (I've started working on this tonight and this is going more or less ok). Then deprecate poll until we have a working solution with stable rust. Most of our users, I reckon, are using OSX or an enabled inotify linux, so leaving out the poll solution could be acceptable (or not ?)
  • Use ffi to connect to fstat api from rsnotify directly and provide a working solution that allows poll to work correctly in stable rust.

Without poll, the lib seems to compile pretty much ok (cf https://github.com/octplane/rsnotify/tree/fsevent_integration ) and there doesn't seem to be any outdated package.

@passcod
Copy link
Member Author

passcod commented Jun 7, 2015

I think most of the flack could come from Windows users who'll be left out, but that's better than nothing working at all.

@passcod
Copy link
Member Author

passcod commented Jun 7, 2015

I'm going to rip out Poll straight up, though. It will make this library not much more than an inotify wrapper until fsevent lands, but that's how it was for a while before, too. Then the work will be to rebuild a polling implementation or two. I'm thinking of applying some actual CS theory to the problem, when I have time.

@passcod
Copy link
Member Author

passcod commented Jun 7, 2015

I've ripped out Poll and released a 2.0.0-pre1 with a NullWatcher that… does nothing. That should at least help things along, something that compiles but does nothing on unsupported platforms is still better than something that just fails.

passcod referenced this issue in andelf/rsnotify Jun 7, 2015
@andelf
Copy link
Contributor

andelf commented Jun 7, 2015

@octplane I copied your fsevent code and made some changes. Now its works. (unwatch , drop left unimplemented) => https://github.com/andelf/rsnotify
It was cloned before NullWatcher introduced,so “4 commits ahead, 1 commit behind passcod:master”.
Maybe we can start from it?

@octplane
Copy link
Contributor

octplane commented Jun 7, 2015

@andelf indeed! I think the merge is not too difficult to perform. The unwatch/drop might be more complicated. I had to fight against threads to make things work correctly. I will not be available until tomorrow morning CET, so go ahead if you have some cycles.

@maurizi
Copy link
Contributor

maurizi commented Jun 7, 2015

I have been using the poll backend on Windows, so I'll be sad to see it go.
That said, I might try and whip up an initial implementation of using winapi.rs to do real file watching and fix #4.

@retep998
Copy link

retep998 commented Jun 7, 2015

For getting the file modified time on Windows call GetFileAttributesExW if you have a Path or GetFileInformationByHandle if you have a HANDLE or FindNextFile if you're iterating over directory entries, at which point you'll get some file information including the last modified time as a FILETIME which is "the number of 100-nanosecond intervals since January 1, 1601 (UTC)."

@maurizi
Copy link
Contributor

maurizi commented Jun 7, 2015

@retep998 I was looking at using ReadDirectoryChangesW to just directly get file modification events, so I won't really need to get file modification time.

That said, I need to get a directory handle to use ReadDirectoryChangesW, which I think means I need to call CreateFileW. Do you know a simple way to go from a &Path to the LPCWSTR CreateFileW wants?

@retep998
Copy link

retep998 commented Jun 7, 2015

let x: Vec<_> = somepath.as_os_str().encode_wide().chain(Some(0)).collect();
CreateFileW(x.as_ptr(), ...);

@blaenk
Copy link
Contributor

blaenk commented Jun 7, 2015

Niiice thanks for helping us out @retep998 :)

Pretty sure that should be a Some(0).into_iter() @maurizi

@blaenk
Copy link
Contributor

blaenk commented Jun 7, 2015

Nevermind! You don't need the into_iter() because chain takes an IntoIterator :D

passcod added a commit that referenced this issue Jun 8, 2015
Add FSEvent support
Fix #7
Also see #12
@passcod
Copy link
Member Author

passcod commented Jun 8, 2015

Just published v2.0.0, breaking change is the removal of PollWatcher. Keeping this issue to track progress on re-crafting a polling implementation.

@passcod passcod changed the title Fix for Rust 1.0 Remake PollWatcher Jun 8, 2015
@BurntSushi
Copy link

I think once rust-lang/rust#25844 lands, you should be able to use crate filetime to resolve your last modified issue.

@passcod
Copy link
Member Author

passcod commented Jun 8, 2015

Yes indeed. Ok, so putting this on hold until Rust 1.1 lands. Just a few more weeks, hopefully!

@blaenk
Copy link
Contributor

blaenk commented Jun 14, 2015

I think @alexcrichton managed to make this (file modified time access) work on stable, in part thanks to his filetime package. See this PR

@blaenk
Copy link
Contributor

blaenk commented Jun 14, 2015

Nevermind, it's once 1.1 is released, like we already knew. At least we know what the solution will look like :)

@blaenk
Copy link
Contributor

blaenk commented Jun 25, 2015

So 1.1 is out, we can move forward on this.

@blaenk
Copy link
Contributor

blaenk commented Jun 25, 2015

Heads up I'm working on this. I got it working already, but gonna go over it one more time to make sure everything's correct.

@passcod passcod added A-enhancement and removed Z-needs implementation Needs an implementation, will accept PRs labels Jun 26, 2015
passcod added a commit that referenced this issue Jun 26, 2015
Reintroduce PollWatcher

With hearty thanks to @blaenk 
Fixes #12
@octplane
Copy link
Contributor

That's great !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants