filewatch: rewrite inotify backend #339
Conversation
|
Currently, stage reloading fails with the following error on macOS: This is with a simple operation like removing a edit: Now with debug statements: |
Akaricchi
force-pushed
the
rewrite-filewatch
branch
from
6af0e2a to
eed6aba
Compare
|
The alignment issue smells like a bug in libinotify-kqueue. According to the Linux man page inotify(7) the name field should be appropriately zero-padded to preserve alignment. I've added a workaround; if that fixes the issue then we should file a bug report. |
|
Compiles without issue, and mostly-works now. Things that do work:
Things that are still a little bit janky:
Aside from that, it did indeed fix the bug and compilation issue. Seems like a bug report is in order for the alignment issue. |
Try editing the stage while the game is not running and then run
That sounds like it doesn't detect the dynamic library file being updated/replaced, for whatever reason. To be honest, I'm losing my patience with It's also possible that my alignment fix is just not correct and every event beyond the first in the queue is garbage. Compile with ASan and maybe add an assertion to check that Alternatively, feel free to write a macOS-specific filewatch backend using Apple's blessed APIs. |
|
It was always a long shot to get it running on macOS, so I'm fine with shipping it as-is, considering I primarily work on Linux now. If I come up with some fixes in the future, I'll create another PR. |
|
Solved one mystery: it was precompiled headers causing the constant regenerate-everything behaviour. Setting |
|
I don't understand the underlying mechanism all that thoroughly, but it seems that in In summary:
|
You should be getting a
If we have to deal with missing creation events, a better way to do it (in |
It's now based on directory monitoring, which is more complicated, but also much more reliable, especially when dealing with move events and hard links. It is also closer to the semantics we actually want (monitoring fs paths for changes, not inodes). It will still get confused if the directory itself gets moved (directly or indirectly), though, but handling that correctly is absurdly difficult and not important for our use-case. Importantly, FileWatch no longer becomes useless after reporting a FILEWATCH_FILE_DELETED event: if another file is placed onto the same path later, the FileWatch will report FILEWATCH_FILE_UPDATED. Some client logic can be simplified because of that.
This function is meant to flush any internal buffers and (hopefully) ensure the data has reached its ultimate destination (e.g. a file on disk) before returning. This is currently implemented as a crude hack and only supports stdio-based RWops. It calls fflush() on the internal FILE* pointer, and on POSIX-systems it also calls fsync() on the file descriptor.
Previously this half-worked: fflush() was called on log_fatal, but not in case of assertion failure, leaving the on-disk log truncated.
This shouldn't be needed, as according to the Linux inotify(7) man page, the entries are supposed to be aligned. It looks like libinotify-kqueue doesn't respect this and doesn't properly zero-pad the name field.
Akaricchi
force-pushed
the
rewrite-filewatch
branch
from
f716f8e to
c2f5ca5
Compare
It's now based on directory monitoring, which is more complicated, but also much more reliable, especially when dealing with move events and hard links. It is also closer to the semantics we actually want (monitoring fs paths for changes, not inodes). It will still get confused if the directory itself gets moved (directly or indirectly), though, but handling that correctly is absurdly difficult and not important for our use-case.
Importantly,
FileWatchno longer becomes useless after reporting aFILEWATCH_FILE_DELETEDevent: if another file is placed onto the same path later, theFileWatchwill reportFILEWATCH_FILE_UPDATED. Some client logic can be simplified because of that.