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

import pyinotify on demand #6

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

gzm55
Copy link
Contributor

@gzm55 gzm55 commented Feb 28, 2021

close #2

do not depend on pyinotify to work on macos

@codecov
Copy link

codecov bot commented Feb 28, 2021

Codecov Report

Patch coverage has no change and project coverage change: +42.52% 🎉

Comparison is base (9fcc8a9) 46.84% compared to head (bf9964f) 89.36%.
Report is 2 commits behind head on master.

❗ Current head bf9964f differs from pull request most recent head afd1c41. Consider uploading reports for the commit afd1c41 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master       #6       +/-   ##
===========================================
+ Coverage   46.84%   89.36%   +42.52%     
===========================================
  Files           4        3        -1     
  Lines         269      141      -128     
===========================================
  Hits          126      126               
+ Misses        143       15      -128     

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gzm55
Copy link
Contributor Author

gzm55 commented Mar 4, 2021

@p-sherratt could u plz take some minutes to review the recent pr?

@p-sherratt
Copy link
Owner

this removes the functionality of being able to open and read from the FIFO multiple times in one session, which I'd like to keep.

I'd like to see if watchdog library might help here-- need to test if's a viable abstraction for monitoring FIFO for both linux+mac, and maybe named pipes in windows.

also there could be a "time to live"/counter argument to limit the number of times the FIFO can be opened for reading -- in this case where we haven't implemented the monitoring for a given OS we can just enforce a value of 1.

@gzm55
Copy link
Contributor Author

gzm55 commented Mar 4, 2021

even for watchdog, the inotify backend cannot catch close_nowrite, changelog.

The ttl/counter feature would be great for security. But for compatible concern, it is better to default only write to fifo once and exit, with an additional runtime flag to enable the daemon fifo and load pyinotify on demand.

@gzm55
Copy link
Contributor Author

gzm55 commented Mar 4, 2021

a delete event of the output would be another signal to break the decrypt loop, i was used to write a POSIX shell version of re-readable fifo, exiting when found that the output fifo is removed.

@gzm55 gzm55 changed the title do not depend on pyinotify WIP: do not depend on pyinotify Mar 4, 2021
@gzm55 gzm55 changed the title WIP: do not depend on pyinotify import pyinotify on demand Mar 5, 2021
@gzm55
Copy link
Contributor Author

gzm55 commented Mar 5, 2021

@p-sherratt add an option --writeonce to disable the daemon feature, and import the pyinotify on demand

@gzm55
Copy link
Contributor Author

gzm55 commented Mar 8, 2021

@p-sherratt any comments?

@gzm55
Copy link
Contributor Author

gzm55 commented Jul 29, 2021

@p-sherratt do you have time to review this pr?

@gzm55
Copy link
Contributor Author

gzm55 commented Aug 2, 2023

@p-sherratt hi, a gently ping~

@gzm55
Copy link
Contributor Author

gzm55 commented Aug 8, 2023

@p-sherratt pyinotify package is installed on linux only now. And with the new option --writeonce, the default actions keeps unchanged on linux.

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.

support macos
2 participants