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

Modify PollWatcher to support pseudo filesystems like sysfs/procfs #396

Merged
merged 13 commits into from
Apr 28, 2022

Conversation

jasta
Copy link
Contributor

@jasta jasta commented Mar 17, 2022

This means that you can now construct an instance of PollWatcher that
performs full file contents hashing to augment the mtime comparison.
This works well for pseudo filesystems that often don't adhere to durable
filesystem metadata norms like accurate file sizes, modification times, etc.

Closes #391

@jasta
Copy link
Contributor Author

jasta commented Mar 17, 2022

Note that I opted to use the built-in hasher after realizing that the likelihood of collision on human readable content is extremely low, bordering on impossible. See this toy project I created to test that theory: https://github.com/jasta/hash-collisions

src/poll.rs Outdated Show resolved Hide resolved
Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

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

Thanks for following up so fast. I left some minor comments.

src/poll.rs Outdated Show resolved Hide resolved
src/poll.rs Show resolved Hide resolved
src/poll.rs Outdated Show resolved Hide resolved
@0xpr03
Copy link
Member

0xpr03 commented Mar 18, 2022

Also looks like we're not 1.47 compatible any more with some code.

@JohnTitor any preference for keeping/pushing MSRV ?

@jasta
Copy link
Contributor Author

jasta commented Mar 18, 2022

Also looks like we're not 1.47 compatible any more with some code.

@JohnTitor any preference for keeping/pushing MSRV ?

I can also drop these extra features if we need to support older rustc for some reason. Again, apologies, I'm new to rust and just exploring as I go here :)

@jasta
Copy link
Contributor Author

jasta commented Mar 18, 2022

Also thanks so much for the fast review, I'm loving working in the rust community so far, y'all are great!

@JohnTitor
Copy link
Member

Also looks like we're not 1.47 compatible any more with some code.

@JohnTitor any preference for keeping/pushing MSRV ?

I think it's totally okay to bump MSRV while we're on beta, and we may have another reason to bump MSRV (#395 (comment)) so feel free to do so if it makes sense here :)

@0xpr03
Copy link
Member

0xpr03 commented Mar 18, 2022

Then we'll bump it to 1.50 for now, so jasta can test against the CI. And we can then change it for #395 if wanted.

@jasta
Copy link
Contributor Author

jasta commented Mar 18, 2022

Should be good to go now

@jasta
Copy link
Contributor Author

jasta commented Mar 20, 2022

Snuck in another commit that also shows this feature working in practice using /sys/class/net/lo/statistics. I wanted to use this to further test the implementation but also spell out the use case for folks more clearly

@jasta
Copy link
Contributor Author

jasta commented Mar 21, 2022

Oh I see the test is failing on windows for predictable reasons, but I'm not sure exactly how we should solve it. The test definitely only works on UNIX systems, but I'm not sure if that includes OS X or not.

@0xpr03
Copy link
Member

0xpr03 commented Mar 21, 2022

That should do the trick, let's see if that is running on more than linux. (granted the docs about cond comp don't list#!, only the reference - so I was also stunned at first)

@jasta
Copy link
Contributor Author

jasta commented Mar 31, 2022

Sorry all, didn't notice that I still left it in a broken state on Windows. Working on fixing this now

Copy link
Contributor Author

@jasta jasta left a comment

Choose a reason for hiding this comment

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

Should be ready to go again but for some reason CI isn't re-running the failing jobs.

tests/poll-watcher-hashing.rs Show resolved Hide resolved
@0xpr03
Copy link
Member

0xpr03 commented Apr 1, 2022

CI isn't re-running the failing jobs

Anti-Mining system from github. First PR of a person to a repo has to be approved for every run by owners.
Also sorry for the slow response, I'm on my way to catch a flight..

@0xpr03
Copy link
Member

0xpr03 commented Apr 12, 2022

Hmm weird, now macos and bsd fail for seemingly unrelated reasons. But at least windows builds.

@0xpr03 0xpr03 mentioned this pull request Apr 16, 2022
@jasta
Copy link
Contributor Author

jasta commented Apr 28, 2022

Can you kick the CI builds to check that they now succeed? I'm anxious to get this PR finally merged so I can stop maintaining my local fork (well, that and I think it will be useful to others of course! 😁 )

@0xpr03
Copy link
Member

0xpr03 commented Apr 28, 2022

As you can see this won't help. You could help me out by just rebasing your stuff on top of master, that should resolve the issues with invalid code, as #399 fixed this on master.

jasta and others added 9 commits April 28, 2022 15:53
Technically this means that you can now construct an instance of
PollWatcher that performs full file contents hashing to augment the
mtime comparison.  This works well for pseudo filesystems that often
don't adhere to durable filesystem metadata norms like accurate file
sizes, modification times, etc.

Closes notify-rs#391
The example shows how to use this to effectively watch pseudo
filesystems like those through sysfs (i.e. /sys/).  The example
by default will only work on Linux but does demonstrate well that it
works where the previous metadata only approach would not.

Sample output:

  Trying "/sys/class/net/lo/statistics/tx_bytes", use `ping localhost` to see changes!
  watching ["/sys/class/net/lo/statistics/tx_bytes"]...
  changed: Event { kind: Modify(Data(Any)), paths: ["/sys/class/net/lo/statistics/tx_bytes"], attr:tracker: None, attr:flag: None, attr:info: None, attr:source: None }
  changed: Event { kind: Modify(Data(Any)), paths: ["/sys/class/net/lo/statistics/tx_bytes"], attr:tracker: None, attr:flag: None, attr:info: None, attr:source: None }
  changed: Event { kind: Modify(Data(Any)), paths: ["/sys/class/net/lo/statistics/tx_bytes"], attr:tracker: None, attr:flag: None, attr:info: None, attr:source: None }
  changed: Event { kind: Modify(Data(Any)), paths: ["/sys/class/net/lo/statistics/tx_bytes"], attr:tracker: None, attr:flag: None, attr:info: None, attr:source: None }
@jasta
Copy link
Contributor Author

jasta commented Apr 28, 2022

My mistake, I misread github's feed and thought you had updated the branch. Just rebased and pushed now

@0xpr03
Copy link
Member

0xpr03 commented Apr 28, 2022

No worries, I'm just a little bit short on time and didn't have the time to try rebasing this correctly. Should've left a note.

@0xpr03 0xpr03 merged commit 480de9d into notify-rs:main Apr 28, 2022
@0xpr03
Copy link
Member

0xpr03 commented Apr 28, 2022

Thanks for hanging in there, I'll make a proper release tomorrow

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

Successfully merging this pull request may close these issues.

Missing strategy for watching pseudo filesystems like those in /sys or /proc.
3 participants