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

feat(lsp): drop fswatch, use inotifywait #29374

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cryptomilk
Copy link
Contributor

@cryptomilk cryptomilk commented Jun 17, 2024

This patch replaces fswatch with inotifywait from inotify-toools:

https://github.com/inotify-tools/inotify-tools

fswatch takes ~1min to set up recursively for the Samba source code directory. inotifywait needs less than a second to do the same thing.

emcrisostomo/fswatch#321

Also fswatch seems to be unmaintained in the meantime.

@cryptomilk
Copy link
Contributor Author

cryptomilk commented Jun 17, 2024

The package inotify-tools needs to be installed on CI images.

@@ -54,7 +54,9 @@ EVENTS

LSP

• TODO
• The watchfunc backend was implemented with fswatch, but deactivated by
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we need to mention this here since it was deactivated by default, and so it's not like a breaking change or anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where do I activate it by default? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found it and enabled it.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to enable it. Anything that uses inotify is going to have the same problems. It wasn't startup-time of fswatch that was the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What was the problem?

Copy link
Member

Choose a reason for hiding this comment

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

Users kept hitting the limits of inotify and getting error messages.

Note libuv also uses inotify internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it and improved the documentation.

However I think it should be documented how to enable inotify as watchfunc. I haven't found anything yet. Any suggestion where to do that?

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

test failures look related?

runtime/doc/lua.txt Outdated Show resolved Hide resolved
@cryptomilk
Copy link
Contributor Author

cryptomilk commented Jun 20, 2024

test failures look related?

inotify-tools needs to be installed on the CI image, I'm not sure how to do that.

Update: Might have found it.

This patch replaces fswatch with inotifywait from inotify-toools:

https://github.com/inotify-tools/inotify-tools

fswatch takes ~1min to set up recursively for the Samba source code
directory. inotifywait needs less than a second to do the same thing.

emcrisostomo/fswatch#321

Also it fswatch seems to be unmaintained in the meantime.

Signed-off-by: Andreas Schneider <[email protected]>
@justinmk justinmk changed the title feat(lsp): Replace fswatch with inotifywait feat(lsp): replace fswatch with inotifywait Jun 20, 2024
@justinmk justinmk changed the title feat(lsp): replace fswatch with inotifywait feat(lsp): drop fswatch, use inotifywait Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants