e-dant / watcher Goto Github PK

I need to create docs about contributions. Here's the flow we should generally use:

Push to next -> await ci/GitHub actions -> run tool/hone for the single header -> run tool/release

"watch will happily keep continue watching"... there was an attempt to use words, but it looks like they got mangled like symbols in a binary.

I am excited with the 0.5.3 version upgrade that wraps not only inotify but also fanotify.

On the other hand, fanotify is a relatively new feature that will not compile on slightly older kernels.
The 0.5.3 implementation of fanotify uses FAN_REPORT_DFID_NAME, so it seems to require Kernel 5.9 or later to compile.
https://man7.org/linux/man-pages/man2/fanotify_init.2.html

Is it possible to add the following condition for include/watcher/adapter/linux/watch.hpp and include/watcher/adapter/linux/fanotify/watch.hpp?

• Check for the existence of fanotify.h
• Check if FAN_REPORT_DFID_NAME is defined

Leave comments here for test cases.

I'm looking for interesting ways to attempt to break the watchers.

Our current test suite creates many thousands of files and/or directories. This is done manually while waiting for an error.

The test suite should be expanded and automated.

If anyone find themselves in a spot where they need to create compile definitions or patches to get this project working the way it should, please drop a comment here. I'm happy to address the issues.

Some possible requests you can make:

• Create version definitions within the project.

• Create a basic test program (without running the full test suite) to see if the project compiles and/or runs on some platform.

• Fix X thing on Y compiler.

Etc, etc.

Whatever you need.

Recently, on both the release and next branches, our CodeQL builds have been coming up with this error:

https://github.com/e-dant/watcher/actions/runs/6684346814/job/18161503474#step:5:74

We also found this error on a Clang build for Linux in development:

https://github.com/e-dant/watcher/actions/runs/6684014904/job/18160816032#step:4:58

This looks and feels like a kind of issue unrelated to our codebase. I am not sure why the issue has only just started to crop up.

Using Nix is always an option to make our builds reproducible and mostly deterministic. Nix was mentioned in a previous build issue: #33

All directories should have, at least, a README.md and a .tellfile. Even if they're short.

The readme offers Watcher's design goals, sure, but it doesn't offer the non-goals.

We should add a section on the things that Watcher is not interested in solving (at least at the moment). These are the OS-level caveats to certain system APIs. For inotify, that's events potentially disappearing or being duplicated. For std::filesystem, there are some places where UB might happen, although I haven't looked into exactly when they occur.

These are important caveats that deserve a place in the readme but require more research.

Thank you for the library. I have a one comment:

• I would remove the usage of static as you cannot create multiple watchers (the fact that it is an inline function does not change the nature of the static variable)

https://github.com/e-dant/watcher/blob/release/sinclude/watcher/watcher.hpp#L838

  static auto callback_hook = callback; // here
  const auto callback_adapter =
      [](ConstFSEventStreamRef, /* stream_ref
                                   (required) */
         auto*,                 /* callback_info (required) */

You can simply pass the hook in the lambda:

  const auto callback_adapter =
      [&callback_hook = callback](ConstFSEventStreamRef, /* stream_ref
                                   (required) */
         auto*,                 /* callback_info (required) */

• Secondly, I do not see an "API" to stop the watcher. In my code I need to be able to stop the watcher on command. It looks like the main README talks about it but I don't see a way.

Hi @e-dant, thanks for your work on issue #32, however, pure C++17 support is still not available.

wtr/watcher.hpp includes the bit library header, which was introduced in C++20 and is preventing compilation on older systems (for example Debian buster).

Is this header needed?

Every so often, we get an error like this: https://github.com/e-dant/watcher/actions/runs/5920128540/job/16051048104#step:5:33

I tried to address this in 38d93a4 by adding a GitHub action to install cmake. Maybe that cmake action isn't working, or maybe something else is off. The path which make is looking for cmake in hints that the brew version of cmake isn't always there. I'm not sure why make keeps looking there.

Maybe the CMake cache needs to be reset? Maybe we should use Nix and just not worry about these kinds of issues?

At some point in the last two weeks, it looks like the GitHub hosted Windows runner changed.

For some reason I do not understand, Windows is now failing to execute the test artifacts: https://github.com/e-dant/watcher/actions/runs/6426774299/job/17451441610

It is not clear to me what the error is, or what changed that may have affected this since https://github.com/e-dant/watcher/actions/runs/6140888668

Decide which one you want:

• The "stuttering" include, in which case you should move README.md and .tellfile into include/watcher
• The "shallow" include, in which case you should bring include/watcher/* up one level

Hello @toge and @e-dant,

I wanted to use this project as a header-only library via conan-center, but found out that the minimum C++ standard is set to C++20 and not C++17.

in v0.2.2 changelog, it is stated that:

C++20 Concepts are removed for being very interesting but not beneficial enough to an API with a very small surface.

However, in e28bd3c commit wtr.watcher.cmake the minimum standard was changed from 17 to 20. Is this only for the standalone program or is it also important for the header-only library files?

On Apple platforms, the user needs to link against two system libraries.

On most (all?) platforms, c++ should point to the platform's default C++ compiler.

We should change g++ to c++ and put something like $(uname | grep darwin && echo '-framework CoreFoundation -framework CoreServices') in the compile command from "Quick Start".

In 0.9.0, watcher uses "alternative operator representations".

For example, following code uses and operator.
https://github.com/e-dant/watcher/blob/release/0.9.0/include/wtr/watcher.hpp#L111-L112

MSVC doesn't understand "alternatvie operator representations" in default.
With /permissive- compiler flag, MSVC can understand it.

I think there are two way to solve it.

1. add /permissive- compiler flag to CMakeLists.txt.
2. stop using "alternative operator expressions".

I think both are effective.
Can you please decide on a solution?

Since 0.8.1, MSVC 19.33 causes compile error.
https://godbolt.org/z/qj6W84KP7

example.cpp
C:\Users\ContainerAdministrator\AppData\Local\Temp\compiler-explorer-compiler2023313-18468-1p7mtrd.iwwy\raw.githubusercontent.com/e-dant/watcher/release/0.8.3/include/wtr/watcher.hpp(191): error C3431: 'kind': a scoped enumeration cannot be redeclared as an unscoped enumeration
C:\Users\ContainerAdministrator\AppData\Local\Temp\compiler-explorer-compiler2023313-18468-1p7mtrd.iwwy\raw.githubusercontent.com/e-dant/watcher/release/0.8.3/include/wtr/watcher.hpp(177): error C3431: 'what': a scoped enumeration cannot be redeclared as an unscoped enumeration
Compiler returned: 2

I do not know the reason for this compile error.
Is there any workaround?

There is only one function that matters (and only one function that should be accessible) to the user: water::watcher::run.

The header include/watcher/event.hpp may have some types worth exposing as well.

The only items in namespace literal should be the ones mentioned above.

Necessary (or, at the least, very helpful) for testing and development are containers suitable for building and running Watcher.

I have some containers laying around for this purpose. They should be made available here.

Hi @e-dant, I am a bit unsure as to what event::effect_type values are set for which filesystem events. This might be due to my lack of experience with filesystem events, or due to my debian bookworm setup (I am using Debian Bookworm with KDE Plasma 5.27.5). Could you clarify a few points, please?

1. When I copy a file into the watched directory with cp new_file watched_dir/ command, 2 events are triggered event::effect_type::create first and then event::effect_type::modify. Is this intended or should only 1 event be set?
2. When I rename a file in the watched directory with mv old_filename new_filename command, only the event::effect_type::rename with the old path is set. How should I get the new filename? Shouldn't a event::effect_type::create be set?
3. Using mv new_file watched_dir/ command to move a file into the watched directory does not trigger any events at all. Shouldn't this set event::effect_type::create event?
4. Using mv filename .. in the watched directory triggers a event::effect_type::rename and not event::effect_type::destroy. Is this intended?
5. When I modify the watched file content event::effect_type::modify is set twice after file modifications are saved. Is this intended?
6. When modifying a file with nano, vim, or kate editors a temporary hidden file (.filename.txt.swp or .filename.txt.kate-swp for kate) is created. This file also triggers watch events, it's not hard to filter them out by checking the event::path_name filename content, but shouldn't these files not trigger any watch events?

One of the only filesystems we can't watch is the virtual /proc filesystem on Linux.

There is a way to create an ebpf program for the kernel to run which lets us know about modification events on those paths.

I've read the source code and I was disappointed not to find any usage of kernel's primitive to watch for filesystem changed (except for Darwin).

Why don't you use kernel primitives (like inotify on linux, for example)?

IMHO, it'll be a lot more efficient than computing a giant hash table of path to monitor.

We need performance benchmarks against the other filesystem watchers. I'm thinking:

• chokidar
• fswatch
• watchman
• notify-rs

We have performance tests, but they're not comparative.

The messaging syntax (including errors) should be detailed in the readme.

Messages come through event. The where field looks like this:

[s, e]/[sys, self]/[description]

Where s means "successful" (such as s/self/live@path) and e means "error" (such as e/sys/read).

self means we failed. sys means the operating system failed.

The description is usually a function name, an api, or a path. It's something extra.

These events always have the kind field as watcher (as opposed to file, dir, etc).

The what field varies. When a watcher first starts watching, what is create. When a watcher dies, it's destroy. Error messages are usually other.

We need to put this in the readme.

Looking for thoughts and solutions.

This project runs on iOS. I want to integrate iOS into our CI pipeline.

The only method I'm currently aware of is attaching a certificate and provisioning profile to this repository's secrets.

The only such secrets I have are tied to me personally.

That would be fine for now.

The problem I see is complications for future contributors access rights to this repository.

There are some problems with the sanitizers on Linux and Windows that we need to fix.

On Linux hosts, the address and memory sanitizers seem to be broken.

The address sanitizer fails here.

Which, as best I can tell, is a bug in LLVM's asan. Others seem to have similar issues, although their workarounds don't work locally, and CI time adds up, so I haven't deployed their workarounds.

The memory sanitizer is also in a bad state, saying that some standard library string functions are being used uninitialized from Catch.

The sanitizers on windows, I'm assuming, have different link options. The documentation is unhelpful because it should work as-is and seems to only support one sanitizer, asan.

We should investigate boot::asio in these places:

• The event loops, which currently use epoll (Linux), iocp (Windows) and dispatch_queue (Darwin).
• The user API, which is currently a callback-style interface
• The watch thread: Currently, every invocation of wtr::watch() spawns a new thread. This may be undesirable to some users. While we do test the efficiency of a large number of parallel watchers, and those tests are sufficiently performant, spawning a new thread for each watcher just seems a bit... well, there are better ways to do this.

We need to document what we use from each header and remove unused includes, at least in include/watcher/* and src/watcher/* files.

This library has become somewhat popular. I expect at least one other user is actively using this project. For them/those users, we need to publish upcoming API changes.

They are not fully fleshed out yet, however, at least these two things may be affected:

1. When we implemented heuristics, the template parameter on watch for delay_ms may be removed.
2. To fully support rename events, either the event.where will be changed to a tuple or another field will be added to the event object.

This will happen before the 1.0 release.