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

Allow entr to watch directories (fixes #470) #492

Closed
wants to merge 4 commits into from
Closed

Conversation

timholy
Copy link
Owner

@timholy timholy commented May 25, 2020

Fixes #470, CC @ffevotte.

Also updates the devdocs for all the latest changes.

@timholy timholy marked this pull request as draft May 25, 2020 23:56
@timholy
Copy link
Owner Author

timholy commented May 25, 2020

Hmm, trickier than expected. Putting this on hold.

@ffevotte
Copy link
Collaborator

Hopefully this works, at least when Revise.watching_files[] == false:

I can think of at least 2 ways of handling the Revise.watching_files[] == true case:

  1. it seems that this option is used on platforms where it is not possible to watch directories, so that it could be argued that we don't have to support watching directories in entr on such platforms. If we go this way, I think we should simply mention this in the documentation and deactivate these tests when Revise.watching_files[] == true.
  2. preliminary tests seem to indicate that it is actually possible to make these tests work also in cases where Revise.watching_files[] == true, provided that we consider directories to be regular files. At the moment, this condition makes Revise forget about watched directories as soon as they are added, so that subsequent events have no chance being caught.

If we follow way (2), I'm not sure whether tests will be representative of what would happen on a real BSD system. In other words, I'm not sure whether we can claim that this feature is supported on all systems, because tests might very well pass only thanks to a Linux-specific behavior regarding directories-considered-as-regular-files.

@ffevotte
Copy link
Collaborator

Here is an example of preliminary exploration of way (2) above:

@timholy would you have an opinion on the best way to move forward?

@timholy
Copy link
Owner Author

timholy commented May 26, 2020

I'd say we should get entr fixes in ASAP, so perhaps split that commit out into a separate PR? We can release a 2.7.1 release once those are in.

For the more ambitous part,

it could be argued that we don't have to support watching directories in entr on such platforms

I strongly agree with this viewpoint, though not opposed to making it work if it's pretty easy.

When I started to think this was more complicated I was starting to get concerned that the emptiness of latestfiles: if there are no individual files that we're individually watching in the directory, notify will never be called. Not sure, I might have been going down a rabbit hole.

@ffevotte
Copy link
Collaborator

ffevotte commented May 27, 2020

I'd say we should get entr fixes in ASAP, so perhaps split that commit out into a separate PR? We can release a 2.7.1 release once those are in.

Hopefully tests pass for #496, and that part will be done.

I strongly agree with this viewpoint, though not opposed to making it work if it's pretty easy.

OK. I'll try to propose a PR that goes this way. Without having any easy way to test on BSD platforms, this seems to be the safest option.

When I started to think this was more complicated I was starting to get concerned that the emptiness of latestfiles: if there are no individual files that we're individually watching in the directory, notify will never be called. Not sure, I might have been going down a rabbit hole.

I'll try and keep this (valid) concern in mind, but according to my first experiments around watching directories, this was not an issue when watching_files[] == false. I'll make sure to test for this case specifically.

@timholy
Copy link
Owner Author

timholy commented Jun 1, 2020

Sorry again to be slow to respond, I've been pretty consumed by #497.

Without having any easy way to test on BSD platforms, this seems to be the safest option.

As of be0b61f, CI runs its tests on Julia>=1.3, linux, once with file-watching false and once with true. The ability to do Pkg.test(; test_args=["REVISE_TESTS_WATCH_FILES"]) was implemented in 94b029b. So we should be sensitive to any bugs. If you're running Julia >=1.3, you can try that locally too.

@ffevotte
Copy link
Collaborator

ffevotte commented Jun 7, 2020

I think this can be safely closed, since everything mentioned here was either incorporated in #493 or in #500.

@timholy timholy closed this Jun 7, 2020
@timholy timholy deleted the teh/fix_470 branch June 7, 2020 08:28
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.

Bug in entr call to watch_folder
2 participants