Support fallback of buck2.file_watcher to 'notify' if 'watchman' doesn't work?

[thoughtpolice]

As I noted in #57 (comment), I found a way to enable watchman support via the config option buck2.file_watcher. See this corresponding commit, in particular the highlighted lines: thoughtpolice/buck2-nix@e47fc8f#diff-d33e979799a45c7c51752e9c8d96a3e452015d1a40b1e4b6ec6a98e92c4d8430R92-R105 — I also recommend the commit message for a summary.

It's very handy. In short, I use direnv to automatically use systemd-run --user to launch a transient user service for watchman, then export an appropriate WATCHMAN_SOCK variable for the repository. Works great! Except...

I forgot to enable it in CI, which caused this failure:

[2022-12-22T15:45:13.628+00:00] 2022-12-22T15:45:13.626994Z  WARN buck2_server::file_watcher::watchman::core: Connecting to Watchman failed (will re-attempt): Error reconnecting to Watchman: While invoking the watchman CLI to discover the server connection details: reader error while deserializing, stderr=`2022-12-22T15:45:13,625: [watchman] while computing sockname: failed to create /usr/local/var/run/watchman/runner-state: No such file or directory
[2022-12-22T15:45:13.630+00:00] `
[2022-12-22T15:45:13.663+00:00] Build ID: 6d28aa[89](https://github.com/thoughtpolice/buck2-nix/actions/runs/3754737959/jobs/6379213447#step:8:90)-1dee-49fd-80c2-b8a54e1c63[96](https://github.com/thoughtpolice/buck2-nix/actions/runs/3754737959/jobs/6379213447#step:8:97)
[2022-12-22T15:45:13.665+00:00] Command failed: 
[2022-12-22T15:45:13.665+00:00] SyncableQueryHandler returned an error
[2022-12-22T15:45:13.665+00:00]
[2022-12-22T15:45:13.665+00:00] Caused by:
[2022-12-22T15:45:13.665+00:00]     0: No Watchman connection
[2022-12-22T15:45:13.665+00:00]     1: Error reconnecting to Watchman
[2022-12-22T15:45:13.665+00:00]     2: Error reconnecting to Watchman
[2022-12-22T15:45:13.665+00:00]     3: While invoking the watchman CLI to discover the server connection details: reader error while deserializing, stderr=`2022-12-22T15:45:13,633: [watchman] while computing sockname: failed to create /usr/local/var/run/watchman/runner-state: No such file or directory
[2022-12-22T15:45:13.665+00:00]        `
BUILD FAILED

So there's two things going on here:

• I'm using a watchman binary from the repository, the Ubuntu 22.04 one. But I didn't install it with dpkg, I installed it with Nix, since you can't rely on the user having it. A consequence of this is that some directories, such as /usr/local/var/run/watchman/, don't exist, which cause spurious failures, since watchman binaries implicitly have a STATEDIR set to /usr/local at build time. And if there is no WATCHMAN_SOCK variable set, the watchman_client Rust library will query the CLI. Therefore calls like watchman get-sock or whatever will fail, because they always probe a non-existent statedir which causes part of the stack trace. I can work around this in some way with Nix, and will file a possible bug report with upstream watchman about it; but I just wanted to clarify this since it's in the above error.
  • Note that the transient systemd unit sets an explicit path to statefile, logfile, and pidfile, which are the 3 required variables that watchman needs; if these are set, the implicit STATEDIR is not used.
  • This is why I set WATCHMAN_SOCK instead with my setup, because if it is set, the watchman_client library uses it above all else, and doesn't need to query the CLI binary. So it "just works"
• The build fails outright if the file_watcher can't be configured.

Would it be possible to optionally fallback to buck2.file_watcher=notify if watchman_client detects and captures a failure like above? This would at least allow users to continuing developing in the repository without a strange error if they don't enable watchman, even if they would detect file changes watchman would otherwise ignore. I'm trying to make my repository 'plug and play', and it feels bad if you cd $src; buck build ... and it immediately fails like this without watchman.

[thoughtpolice]

I can probably try to write a patch for this too, if it's deemed acceptable and easy to review/integrate; I don't know how difficult it is to "back-port" changes from GH into the Meta repository in this manner, so if it's harder than just making the change on all of y'alls end, I can wait.

[krallin]

I don’t think an unconditional fallback would be desirable (it’s certainly not something we’d ever want to do internally, so merging that would be undesirable), but allowing e.g. a comma separated list of file watchers to try, or a separate config value that means « watchman if available but inotify otherwise » seems like it’d be reasonable (the former seems like it’d be preferable to me). Submitting a PR is fine, it’d quite easy to pull in, but bear in mind a good portion of the team is on holiday, so it might take a little while to get back to you if you do.

…

On Thu, 22 Dec 2022 at 17:27, Austin Seipp ***@***.***> wrote: I can probably try to write a patch for this too, if it's deemed acceptable and easy to review/integrate; I don't know how difficult it is to "back-port" changes from GH into the Meta repository in this manner, so if it's harder than just making the change on all of y'alls end, I can wait. — Reply to this email directly, view it on GitHub <#59 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANIHVXIYVGTK24QVRMFGJ3WOR6QJANCNFSM6AAAAAATG4IRZM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[ndmitchell]

We do have https://github.com/facebookincubator/buck2/blob/main/CONTRIBUTING.md, but I've just revised that and put up an internal diff to make it also read:

Buck2 is currently developed in Meta's internal repositories. Code that is developed internally gets reviewed, sent through CI, committed, and then automatically mirrored out to GitHub every 15 minutes. Code that arrives through a PR is reviewed by a Meta developer on GitHub, then once accepted, moved into our internal workflow where it is reviewed, sent through CI, committed and added to the repo. We maintain both external CI (the results of which are visible on GitHub) and a more thorough internal CI (building internal projects etc). Alas, our full test suite is not yet mirrored to the open source repo, but we hope to fix that in due course.

TLDR: We welcome PRs, and they are actually remarkably easy to merge (if we are around, but as of now, I think almost no one is!)

If you want to make your repo plug and play, using inotify is probably fine. Watchman is really useful if you have a huge repo, or are using a Sapling-backed virtual disk. If not, then inotify works great. That's the reason we pick inotify as the default for open source - to increase plug and play ability.

[thoughtpolice]

I tried implementing this over the holiday break but got stuck. I initially tried a naive approach where configuration is parsed, here. Basically: split on , in the string, and try every option in a for loop until one worked.

But that failed. I think what's happening in this code is that the buck client program is telling the daemon which of the two file watcher APIs to use. It then tries to enable one, e.g. watchman, and fails, which causes the daemon to fail instead, not the client program, and that causes an unrecoverable failure. I think. Maybe it is all in the server. I don't quite understand the code layout yet and I'm pretty new at Rust, especially on a codebase this size.

I'll post my initial attempt later but needless to say I'd still quite like this. As a workaround, at the moment I simply hardcode aarch64-linux to use notify and ignore the configuration setting entirely using this patch. This roughly gets me the UX I want on multiple platforms, and once this is implemented, should be much less hacky.

[ndmitchell]

Generally the file watcher lives in the daemon, so I wouldn't expect much to happen in the client (broadly speaking, the client is just a argument parser and output console). When you say fail, do you mean with a panic or with a anyhow::Error? If it's a panic, that's probably the bug and should be fixed. If it's an Error, that should be easy to ignore and select the next one on.

Alternatively, if Watchman provided binaries for aarch64-linux, would that solve the problem? Should be easy to reach out to that team.

Separately, I am curious why you want to default to watchman at all? Wouldn't notify be sufficient? For really big repos, especially ones backed by Sapling/Mercurial, Watchman is a great idea. Otherwise, I imagine notify is good enough.

[thoughtpolice]

So, following up on this: I got on a phone call with Neil a while back and explained that one use case for watchman was integration with Sapling's virtual filesystem, which I intend to explore once it's stable and in a FOSS release. That's a major motivation for integrating watchman reliably and early on.

And recently, after a miraculous amount of work from the Nixpkgs community, a recent change added an up-to-date watchman binary, as well as all the dependencies -- and it works on aarch64-linux! See thoughtpolice/buck2-nix@81a2e8c and NixOS/nixpkgs#181787 for info.

So my need for this feature has basically gone away. Nixpkgs can provide aarch64-linux binaries for my users instead.

I also made an upstream issue about this, so non-Nix users can also have binary builds; see facebook/watchman#1094 — but it remains unresolved, due to a lack of appropriate aarch64 builders for GitHub Actions. We'll have to wait until GitHub deploys Ampere builders at some point, I guess...

Closing this for now to reduce issue clutter and because the original (niche) motivation has gone away.