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

[thoughtpolice]

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

[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.