Add native polling mode support on Windows #7010

nojb · 2023-02-06T00:46:18Z

This PR builds on #6087, simplifying the implementation, adding a small test driver, tests, some documentation, etc.

I recommend reading the comments in fswatch_win.mli to get a sense of the API and the comment at the top of fswatch_win_stubs.c to get a sense of the implementation.

I would appreciate a review of the changes to dune_file_watcher.ml. In particular I am not sure if the stuff involving Fsevents.is_special_file_fsevents and the sync_table is correct (I mostly copied this from fsevents and am not sure what role the sync_table plays).

There is a small test driver in src/fswatch_win/bin/dune_fswatch_bin.exe. Some tests were added to test/unit-tests/fswatch_win.

src/dune_file_watcher/dune_file_watcher.ml

src/fswatch_win/fswatch_win.ml

src/dune_file_watcher/dune_file_watcher.ml

rgrinberg · 2023-02-06T01:01:34Z

Can we get a look from @dra27, @jonahbeckford, @MisterDA on the C side? And possibly some testing on Windows

src/fswatch_win/fswatch_win_stubs.c

nojb · 2023-02-06T14:04:05Z

Correct me if I'm wrong because I'm not 100% sure, but since you allocate with malloc you should free with free rather than caml_stat_free, or use caml_stat_alloc here.

You are right, good catch. I replaced malloc with caml_stat_alloc. Thanks!

nojb · 2023-02-07T15:53:03Z

Could I enroll anyone to test this in some real-life examples? @MisterDA, @jonahbeckford? Help appreciated!

jonahbeckford · 2023-02-07T16:58:18Z

Could I enroll anyone to test this in some real-life examples? @MisterDA, @jonahbeckford? Help appreciated!

I've switched to using it (so far no issues). But this needs some testing on a dune universe monorepo, which I can do hopefully in ~48 hours.

rgrinberg

LGTM. A better design for watcher API would allow us to create new ones without touching the old ones, but I'm pretty convinced that this new watcher does not change the behavior of the old ones.

A CHANGES entry would be welcome.

nojb · 2023-02-07T18:02:53Z

Could I enroll anyone to test this in some real-life examples? @MisterDA, @jonahbeckford? Help appreciated!

I've switched to using it (so far no issues). But this needs some testing on a dune universe monorepo, which I can do hopefully in ~48 hours.

Great, thanks!

.ocamlformat

emillon · 2023-02-08T08:40:23Z

In terms of releasing, I'm ok with merging this and including this in the next alpha for easier testing. That would be once the current known regressions are fixed.

nojb · 2023-02-08T08:47:51Z

In terms of releasing, I'm ok with merging this and including this in the next alpha for easier testing. That would be once the current known regressions are fixed.

OK, thanks! But let's wait until we get some feedback from @jonahbeckford before merging...

jonahbeckford · 2023-02-10T11:46:22Z

This looks good; wasn't able to break it.

Tested it on a small dune universe (75 packages rooted from capnp).
Mass deletion of half and then restoration of those files ... it recovered.
And it works better than my fswatch shim (which could not respect the Dune (dirs ...) inclusion/exclusion rules).

Thanks!

nojb · 2023-02-10T15:20:18Z

This looks good; wasn't able to break it.

Tested it on a small dune universe (75 packages rooted from capnp).

Mass deletion of half and then restoration of those files ... it recovered.

And it works better than my fswatch shim (which could not respect the Dune (dirs ...) inclusion/exclusion rules).

Thanks!

Thanks for testing, @jonahbeckford!

I pushed some further changes (avoid watching the same directory twice and fixing some minor memory leaks). Apologies for asking, but if it is not too much work, would you mind running your test on the latest version? (I promise not to push any other changes to this PR.)

I have done some testing on my side in any case, so if you can't do the testing, I would propose to go ahead and merge the present state.

MisterDA

Is it unnecessary to check the result of malloc in this use-case?

src/fswatch_win/fswatch_win_stubs.c

nojb · 2023-02-10T15:44:57Z

Thanks @MisterDA, I fixed the three issues.

Is it unnecessary to check the result of malloc in this use-case?

Sorry, which part of the code are you referring to?

MisterDA · 2023-02-10T17:56:09Z

Is it unnecessary to check the result of malloc in this use-case?

Sorry, which part of the code are you referring to?

You're using malloc without checking that the return value is non-NULL, which indicates an out of memory error. You can assume it won't ever happen, I just wanted to be sure it's an assumption you're willing to make.

nojb · 2023-02-10T18:14:24Z

Is it unnecessary to check the result of malloc in this use-case?

Sorry, which part of the code are you referring to?

You're using malloc without checking that the return value is non-NULL, which indicates an out of memory error. You can assume it won't ever happen, I just wanted to be sure it's an assumption you're willing to make.

Ah, yes, indeed. To be addressed in a future PR :)

jonahbeckford · 2023-02-10T23:43:45Z

@nojb Now nothing is triggering changes to the build graph. When I go back to commit a7602baff3587481ffaa08267eb84223457b9f73 it works.

nojb · 2023-02-10T23:53:04Z

@nojb Now nothing is triggering changes to the build graph. When I go back to commit a7602baff3587481ffaa08267eb84223457b9f73 it works.

Strange, seems to work on my side. Did you try with the very last commit 2226a0a?

jonahbeckford · 2023-02-11T00:04:07Z

@nojb Now nothing is triggering changes to the build graph. When I go back to commit a7602baff3587481ffaa08267eb84223457b9f73 it works.

Strange, seems to work on my side. Did you try with the very last commit 2226a0a?

I tried (again) with that last commit. I'll start a bisect.

jonahbeckford · 2023-02-11T00:35:32Z

In order of bisection:

2226a0a marked bad
a7602ba marked good
5b38653 Error: Please install fswatch to enable watch mode.. Sigh, this may be a false negative but I'll be conservative and keep search in the older half of commits. marked bad
808a0e0 Error: Please install fswatch to enable watch mode. marked bad
df834cb Error: Please install fswatch to enable watch mode. marked bad
14daaf6 Error: Please install fswatch to enable watch mode. marked bad
a808932 Error: Please install fswatch to enable watch mode. marked bad

The last one was the first known bad commit. However I didn't realize until after the bisection
that this is meaningless ... it isn't a linear history; there must be some merges happening.

Perhaps it would be better if you could suggest a commit.

nojb · 2023-02-11T00:44:55Z

it isn't a linear history; there must be some merges happening.

Oops, yes, sorry about that. I rebased to eliminate the merges. The old "good" commit is now d9a779e.

Perhaps it would be better if you could suggest a commit.

I will go over the code tomorrow and try to figure out what is going on; it is too late right now :) Thanks for your help!

nojb · 2023-02-13T15:57:03Z

@jonahbeckford any idea how we can investigate this issue? Is it possible to reproduce your test rig on my side?

jonahbeckford · 2023-02-14T00:41:00Z

I found the problem while trying to create a minimum reproducing test case. I vendor directories inside a local opam switch. Anything inside a _opam directory is not watched by the newer commits to this PR ... it no longer respects what has been explicitly vendored.

The test rig is https://github.com/jonahbeckford/dune/releases/download/3.2.0/dune-root-causing.zip . As long as you have a working OCaml compiler, you can run dune build -w and edit a file (ex. _opam\res.5.0.1\src\pres_intf.ml) and it won't trigger a rebuild. Any subsequent dune build will pick up the edit though.

nojb · 2023-02-14T07:01:40Z

I found the problem while trying to create a minimum reproducing test case. I vendor directories inside a local opam switch. Anything inside a _opam directory is not watched by the newer commits to this PR ... it no longer respects what has been explicitly vendored.

The test rig is https://github.com/jonahbeckford/dune/releases/download/3.2.0/dune-root-causing.zip . As long as you have a working OCaml compiler, you can run dune build -w and edit a file (ex. _opam\res.5.0.1\src\pres_intf.ml) and it won't trigger a rebuild. Any subsequent dune build will pick up the edit though.

Thanks for the detective work! I will investigate and post back here.

nojb · 2023-02-14T09:16:44Z

Thanks for the detective work! I will investigate and post back here.

It was a trivial issue due to re-using the "ignore" regexps that were used with the fswatch backend. I pushed a fix. I tested and things seem to work well in your test case. Can you confirm? Thanks a lot for helping to track down this issue!

jonahbeckford · 2023-02-14T15:56:28Z

Verified! Thanks.

nojb · 2023-02-14T17:56:49Z

Verified! Thanks.

Thanks for the confirmation! There are a few more improvements that I have in mind, but they can be left for later, so that we make the cut for the next release.

I'll merge as soon as the CI passes.

nojb · 2023-02-14T20:10:12Z

The Linux and OS X are failing because I changed it so that _opam is no longer systematically ignored, which seems the right behaviour in general. @rgrinberg: do you agree?

rgrinberg · 2023-02-14T20:46:06Z

Hmm but why shouldn't it be ignored? If dune is using an opam switch, it will discover the paths by running opam env.

nojb · 2023-02-14T21:35:14Z

Hmm but why shouldn't it be ignored? If dune is using an opam switch, it will discover the paths by running opam env.

Normally yes, but one can also explicitly include this directory in the Dune build (eg by using (vendored_dirs ...)). See eg the comment by @jonahbeckford above #7010 (comment)

nojb · 2023-02-15T10:37:13Z

As discussed, I removed the change to the _opam (and _esy) exclusions from this PR and will open a separate PR for that.

Planning to merge as soon as the CI passes.

Signed-off-by: Uma Kothuri <[email protected]> Signed-off-by: nojebar <[email protected]>

@gridbugs

…, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.7.0) CHANGES: - Allow running `$ dune exec` in watch mode (with the `-w` flag). In watch mode, `$ dune exec` the executed binary whenever it is recompiled. (ocaml/dune#6966, @gridbugs) - `coqdep` is now called once per theory, instead of one time per Coq file. This should significantly speed up some builds, as `coqdep` startup time is often heavy (ocaml/dune#7048, @Alizter, @ejgallego) - Add `map_workspace_root` dune-project stanza to allow disabling of mapping of workspace root to `/workspace_root`. (ocaml/dune#6988, fixes ocaml/dune#6929, @richardlford) - Fix handling of support files generated by odoc. (ocaml/dune#6913, @jonludlam) - Fix parsing of OCaml errors that contain code excerpts with `...` in them. (ocaml/dune#7008, @rgrinberg) - Pre-emptively clear screen in watch mode (ocaml/dune#6987, fixes ocaml/dune#6884, @rgrinberg) - Fix cross compilation configuration when a context with targets is itself a host of another context (ocaml/dune#6958, fixes ocaml/dune#6843, @rgrinberg) - Fix parsing of the `<=` operator in *blang* expressions of `dune` files. Previously, the operator would be interpreted as `<`. (ocaml/dune#6928, @tatchi) - Fix `--trace-file` output. Dune now emits a single *complete* event for every executed process. Unterminated *async* events are no longer written. (ocaml/dune#6892, @rgrinberg) - Fix preprocessing with `staged_pps` (ocaml/dune#6748, fixes ocaml/dune#6644, @rgrinberg) - Use colored output with MDX when Dune colors are enabled. (ocaml/dune#6462, @MisterDA) - Make `dune describe workspace` return consistent dependencies for executables and for libraries. By default, compile-time dependencies towards PPX-rewriters are from now not taken into account (but runtime dependencies always are). Compile-time dependencies towards PPX-rewriters can be taken into account by providing the `--with-pps` flag. (ocaml/dune#6727, fixes ocaml/dune#6486, @esope) - Print missing newline after `$ dune exec`. (ocaml/dune#6821, fixes ocaml/dune#6700, @rgrinberg, @Alizter) - Fix binary corruption when installing or promoting in parallel (ocaml/dune#6669, fixes ocaml/dune#6668, @edwintorok) - Use colored output with GCC and Clang when compiling C stubs. The flag `-fdiagnostics-color=always` is added to the `:standard` set of flags. (ocaml/dune#4083, @MisterDA) - Fix the parsing of decimal and hexadecimal escape literals in `dune`, `dune-package`, and other dune s-expression based files (ocaml/dune#6710, @shym) - Report an error if `dune init ...` would create a "dune" file in a location which already contains a "dune" directory (ocaml/dune#6705, @gridbugs) - Fix the parsing of alerts. They will now show up in diagnostics correctly. (ocaml/dune#6678, @rginberg) - Fix the compilation of modules generated at link time when `implicit_transitive_deps` is enabled (ocaml/dune#6642, @rgrinberg) - Allow `$ dune utop` to load libraries defined in data only directories defined using `(subdir ..)` (ocaml/dune#6631, @rgrinberg) - Format dune files when they are named `dune-file`. This occurs when we enable the alternative file names project option. (ocaml/dune#6566, @rgrinberg) - Move `$ dune ocaml-merlin -dump-config=$dir` to `$ dune ocaml merlin dump-config $dir`. (ocaml/dune#6547, @rgrinberg) - Allow compilation rules to be impacted by `(env ..)` stanzas that modify the environment or set binaries. (ocaml/dune#6527, @rgrinberg) - Coq native mode is now automatically detected by Dune starting with Coq lang 0.7. `(mode native)` has been deprecated in favour of detection from the configuration of Coq. (ocaml/dune#6409, @Alizter) - Print "Leaving Directory" whenever "Entering Directory" is printed. (ocaml/dune#6419, fixes ocaml/dune#138, @cpitclaudel, @rgrinberg) - Allow `$ dune ocaml dump-dot-merlin` to run in watch mode. Also this command shouldn't print "Entering Directory" mesages. (ocaml/dune#6497, @rgrinberg) - `dune clean` should no longer fail under Windows due to the inability to remove the `.lock` file. Also, bring the implementation of the global lock under Windows closer to that of Unix. (ocaml/dune#6523, @nojb) - Remove "Entering Directory" messages for `$ dune install`. (ocaml/dune#6513, @rgrinberg) - Stop passing `-q` flag in `dune coq top`, which allows for `.coqrc` to be loaded. (ocaml/dune#6848, fixes ocaml/dune#6847, @Alizter) - Fix missing dependencies when detecting the kind of C compiler we're using (ocaml/dune#6610, fixes ocaml/dune#6415, @emillon) - Allow `(include_subdirs qualified)` for OCaml projects. (ocaml/dune#6594, fixes ocaml/dune#1084, @rgrinberg) - Accurately determine merlin configuration for all sources selected with `copy#` and `copy_files#`. The old heuristic of looking for a module in parent directories is removed (ocaml/dune#6594, @rgrinberg) - Fix inline tests with *js_of_ocaml* and whole program compilation mode enabled (ocaml/dune#6645, @hhugo) - Fix *js_of_ocaml* separate compilation rules when `--enable=effects` ,`--enable=use-js-string` or `--toplevel` is used. (ocaml/dune#6714, ocaml/dune#6828, ocaml/dune#6920, @hhugo) - Fix *js_of_ocaml* separate compilation in presence of linkall (ocaml/dune#6832, ocaml/dune#6916, @hhugo) - Remove spurious build dir created when running `dune init proj ...` (ocaml/dune#6707, fixes ocaml/dune#5429, @gridbugs) - Allow `--sandbox` to affect `ocamldep` invocations. Previously, they were wrongly marked as incompatible (ocaml/dune#6749, @rgrinberg) - Validate the command line arguments for `$ dune ocaml top-module`. This command requires one positional argument (ocaml/dune#6796, fixes ocaml/dune#6793, @rgrinberg) - Add a `dune cache size` command for displaying the size of the cache (ocaml/dune#6638, @Alizter) - Fix dependency cycle when installing files to the bin section with `glob_files` (ocaml/dune#6764, fixes ocaml/dune#6708, @gridbugs) - Handle "Too many links" errors when using Dune cache on Windows (ocaml/dune#6993, @nojb) - Allow the `cinaps` stanza to set a custom alias. By default, if the alias is not set then the cinaps actions will be attached to both `@cinaps` and `@runtest` (ocaml/dune#6991, @rgrinberg) - Add `(using ctypes 0.3)`. When used, paths in `(ctypes)` are interpreted relative to where the stanza is defined. (ocaml/dune#6883, fixes ocaml/dune#5325, @emillon) - Auto-detect `dune-workspace` files as `dune` files in Emacs (ocaml/dune#7061, @ilankri) - Add native support for polling mode on Windows (ocaml/dune#7010, @yams-yams, @nojb)

This reverts commit 73506c2.

Co-authored-by: Uma Kothuri <[email protected]> Signed-off-by: Uma Kothuri <[email protected]> Signed-off-by: nojebar <[email protected]>

nojb mentioned this pull request Feb 6, 2023

Adds native polling mode support on Windows #6087

Closed

nojb added the windows label Feb 6, 2023