Added a simple version of a polling loop. by kodek16 · Pull Request #1140 · ocaml/dune

kodek16 · 2018-08-15T09:50:55Z

dune build -w now behaves in a similar way to Jenga polling mode,
automatically rebuilding the project when changes are detected.
dune runtest -w is similar, but also runs tests after successful
build.

Note that on platforms other than Linux, you need to have fswatch
installed for watch mode to work.

ghost · 2018-08-15T12:20:52Z

bin/main.ml

      targets)

+let fswatch_command root_path =
+  let excludes = [ {|/\.#|}


A few elements in this list only seem relevant when not doing out-of-source builds. Let's restrict it to:

[ {|\.#|} ; {|_build|} ; {|\.hg|} ; {|\.git|} ; {|~$|} ; {|/#[^#]*#$|} ; {|\.install$|} ]

ghost · 2018-08-15T12:45:24Z

bin/main.ml

    let log = Log.create common in
-    Scheduler.go ~log ~common
-      (Main.setup ~log common >>= fun setup ->
+


The polling mode is useful for both dune build and dune runtest. Could you factorize the code so that it works with both commands?

ghost · 2018-08-15T12:46:38Z

bin/main.ml

+         >>= fun () ->
+         fswatch_command File_tree.(Dir.path (root setup.file_tree))
+         >>= fun (fswatch, args) ->
+         let fswatch =


We should do this outside of the loop. Dir.path (root setup.file_tree) is always Path.root

ghost · 2018-08-15T12:51:49Z

One thing I just remember we need to handle is promotion. Sometimes, dune records files that needs to be promoted to the source tree. When --auto-promote is passed, these files should be promoted at the end of the build and we should loop immediately. The Promotion module records the files that need to be promoted

Khady · 2018-08-15T13:26:39Z

Does it detect changes during the compilation? I had a quick look at the code, but it's not obvious to me. As soon as the compilation takes more than a few seconds, this is a feature I feel is required.

kodek16 · 2018-08-15T15:16:47Z

@Khady, file changes while build is active are not detected yet, but I agree this is definitely something to be improved in near future.

kodek16 · 2018-08-15T15:54:23Z

@diml, auto-rebuilds after promotions will be solved in a natural way once build restart on changes is implemented. For now I've added a simple hackish flag to retrigger build if at least one file was promoted.

Khady · 2018-08-16T02:24:53Z

Might be interesting to look at https://github.com/watchexec/watchexec then. It is multi platform, and it handles interruptions.

Anyway, thanks for working on this feature!

emillon · 2018-08-16T07:48:42Z

bin/main.ml

+  in
+  let path = Path.to_string_maybe_quoted root_path in
+  let uname =
+    match Bin.which "uname" with


Rather than doing this in two steps (and relying on uname), can we first check if inotifywait exists and use it if it does?

Good point.

emillon · 2018-08-16T07:49:57Z

bin/main.ml

+      Format.eprintf "Error: executable not found: %s\n" fswatch;
+      exit 1
+  in
+  Process.run_capture Strict fswatch args ~env:Env.initial


I'm not sure that this is intended, but it seems that all call sites of watch_changes pipe it into ignore, so it might be worth returning unit directly instead.

Or even: Process.run ~stdout:Config.dev_null

I've added >>| ignore for now, but in the future we're likely to need the actual names of files that were modified, so this is going to change anyway.

For the time being let's use Process.run ~stdout:Config.dev_null. It's likely that in the future we'll need to keep the process alive and read its output continously, so we'll need something else than Process.run.

ghost · 2018-08-16T08:47:37Z

bin/main.ml

+      >>= fun _ ->
+      main_loop ()
+    in
+    try


We can simplify this code with a single loop: let rec loop f = try Scheduler.go ... (f ()) ...

ghost · 2018-08-16T08:54:21Z

bin/main.ml

+      let targets = resolve_targets_exn ~log common setup targets in
+      do_build setup targets
+    in
+    if common.watch then


This code looks very similar to the one for dune runtest. It can be factorized a bit more

ghost · 2018-08-23T16:57:32Z

I tried it again and it seems to work well when combined with --auto-promote. However, the messages Success, polling ... are now gone.

ghost · 2018-08-29T14:58:13Z

Never mind, I tried again and it works. I was using dune exec -- dune runtest -w. This must be a bug in dune exec

kodek16 · 2018-08-29T15:05:50Z

One other thing that we should consider is the naming: do you think we should stick with -w for "watch mode", or switch to -p for polling mode as in Jenga?

ghost · 2018-08-29T15:07:24Z

bin/main.ml

+       fswatch, ["-r"; path; "-1"] @ excludes
+     | None ->
+      (* Exit immediately to prevent a loop of these errors. *)
+      Format.eprintf "Error: fswatch not found. \


To match the style of other errors, could you use:

die "@{<error>Error@}: fswatch ..."

The message needs to mention inotify as well since we look for either of these tools

ghost · 2018-08-29T15:15:25Z

src/scheduler.mli


 open! Stdune

+type status_line_config = string option * [`Show_jobs | `Don't_show_jobs]


This type seems simpler to me:

type status_line_config = { message : string option ; show_jobs : bool }

ghost · 2018-08-29T15:21:46Z

src/scheduler.ml

+      Format.eprintf "Internal error: setup failed.\n";
+      exit 1
+  in
+  let init =


This should be init (). In general we assume that a given Fiber.t value is executed only once

ghost · 2018-08-29T15:33:25Z

Could you try adding a module Hooks with the following signature:

(** Register a function called at the end of every build *)
val at_end_of_build : (unit -> unit) -> unit

(** Regsiter a function called at the end of the current built only *)
val at_end_of_current_build : (unit -> unit) -> unit

(** Invoke all registered hooks *)
val end_of_build : (unit -> unit)

and use it to deal with the various cleanup functions? I feel like this would simplify a bit the code

ghost · 2018-08-29T15:36:57Z

One other thing that we should consider is the naming: do you think we should stick with -w for "watch mode", or switch to -p for polling mode as in Jenga?

Unfortunately, we are already using -p for release builds. I.e. the build command packages must use when built with opam is: dune build -p <package>.

kodek16 · 2018-08-31T20:21:41Z

@diml I tried added the module, since it's a much better idea than at_exit anyway, but I feel really reluctant about introducing more global state, especially if we consider interrupting builds in the future, which would make it not trivial to distinguish between leftover state (potentially including hooks) from old builds.

I would stick with passing Build_system.t to finally, which would do the clean-up for the specific build it's called with.

rgrinberg · 2018-09-01T18:09:38Z

bin/main.ml

+    (match Bin.which "fswatch" with
+     | Some fswatch ->
+       let excludes =
+         List.(map ~f:(fun x -> ["--exclude"; x]) excludes |> concat)


You can use List.concat_map here.

rgrinberg · 2018-09-01T18:12:02Z

bin/main.ml

+  if common.watch then
+    Scheduler.poll ~cache_init:false ~log ~common ~init ~once ~finally ()
+  else begin
+    (try


This bit (and preferably the entire file) needs to be ocp-indented

rgrinberg · 2018-09-01T18:20:07Z

src/scheduler.ml

+    | Some c -> c
+    | None ->
+      Format.eprintf "Internal error: setup failed.\n";
+      exit 1


Why not raise a code_error here?

ghost · 2018-09-03T14:27:05Z

bin/main.ml

+                      One of them needs to be installed for watch mode to work.\n")
+
+let watch_changes () =
+  let watch, args = watch_command Path.root in


We should cache the result of this call with a lazy

ghost · 2018-09-03T14:34:06Z

bin/main.ml

    | Ok targets ->
      targets)

+let clear_all_caches () =


Can't we put these in hooks?

ghost · 2018-09-03T14:38:04Z

src/scheduler.mli

+  :  ?log:Log.t
+  -> ?config:Config.t
+  -> ?cache_init:bool
+  -> init:(unit -> 'a Fiber.t)


I believe we can now change this to unit -> unit Fiber.t, which should simplify a bit the implementation. It also seems to me that once and finally should have type ... -> unit Fiber.t, given that we currently do nothing with 'b or 'c.

ghost · 2018-09-03T14:47:28Z

@kodek16, it's possible that we'll need to provide more granularity in hooks. For instance to allows to recognize the end of the a build vs an interruption of the current build. For long running builds, it is also likely that we will need to add a mechanism to dump the databases at regular intervals. The hooks interface allows to keep all the logic related to a system in its own module, which is nice.

BTW, We should put End_of_build.run in an at_exit, to make sure it's always called.

emillon · 2018-09-03T16:49:26Z

I tried this and it looks like it will be very useful! I have some remarks:

when fswatch and inotifywait are not installed, this outputs errors in an infinite loop, taking 100% CPU.
when running dune build -w, after fixing an error, the only feedback that the error is fixed is the "Success" message that is on the same line as the "polling" message. May I suggest clearing the terminal (using Bin.which "clear" if available), or at least displaying a message on its own line? The same happens if we go from a success state to a success state: there's little feedback that the build has completed.
"polling filesystem" in the message makes it look like we're actively polling the filesystem rather than subscribed to events (some watch tools have a fallback mode like that). What do you think about a more neutral "waiting for filesystem changes"?
can you rebase this on top of master? the history is a bit messy at the moment.

And once again thanks for working on this! ✨

commit ca3c1b7 Merge: f1744b6 610b20d Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Tue Sep 4 10:28:04 2018 -0400 Merge branch 'master' into watch-mode commit f1744b6 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Fri Aug 31 16:13:53 2018 -0400 Add Hooks module. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit 09e0de6 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Fri Aug 31 15:08:28 2018 -0400 Fix review suggestions. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit a59d70d Merge: c7aa8ed 6da1f19 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Fri Aug 31 14:35:50 2018 -0400 Merge branch 'master' into watch-mode commit c7aa8ed Merge: aa15c6c 6dedc68 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Fri Aug 31 09:37:28 2018 -0400 Merge branch 'master' into watch-mode commit aa15c6c Merge: ce562e4 265adc8 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Wed Aug 29 11:04:25 2018 -0400 Merge branch 'master' into watch-mode commit ce562e4 Merge: e8b55b4 c0eebce Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Mon Aug 27 15:05:51 2018 -0400 Merge branch 'master' into watch-mode commit e8b55b4 Merge: 1a1289f 9d61c59 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Mon Aug 27 11:31:54 2018 -0400 Merge branch 'master' into watch-mode commit 1a1289f Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Fri Aug 24 10:40:19 2018 +0100 Fixed build after merge. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit c2288e0 Merge: d5fd1c4 b7da030 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Fri Aug 24 10:26:27 2018 +0100 Merge branch 'master' into watch-mode commit d5fd1c4 Merge: d69b6c5 b424ba9 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Tue Aug 21 11:38:07 2018 +0100 Merge branch 'master' into watch-mode commit d69b6c5 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Tue Aug 21 11:26:21 2018 +0100 Moved polling logic inside Scheduler module. This prevents us from calling Scheduler.go multiple times which may be problematic because of scheduler state. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit f76d3d1 Merge: e4935fb 5b6496f Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Tue Aug 21 09:45:58 2018 +0100 Merge branch 'master' into watch-mode commit e4935fb Merge: 584b9e0 6045568 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Mon Aug 20 17:05:14 2018 +0100 Merge branch 'master' into watch-mode commit 584b9e0 Merge: 6740ec8 99d82c2 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Mon Aug 20 09:51:10 2018 +0100 Merge branch 'master' into watch-mode commit 6740ec8 Merge: a3cbc56 309dd5f Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Fri Aug 17 15:21:22 2018 +0100 Merge branch 'master' into watch-mode commit a3cbc56 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Fri Aug 17 14:59:19 2018 +0100 Fixed non-polling build. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit 882c5d5 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Fri Aug 17 12:59:13 2018 +0100 Refactored polling to only call finalize once. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit 3e4f17a Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Thu Aug 16 15:27:51 2018 +0100 Made status line nicer and fixed auto-promote polling. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit f84cda3 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Thu Aug 16 14:59:50 2018 +0100 Updated tests, removed duplicate finalize call. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit 88a1373 Merge: 8f05ef5 166dd06 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Thu Aug 16 14:31:04 2018 +0100 Merge branch 'master' into watch-mode commit 8f05ef5 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Thu Aug 16 14:27:43 2018 +0100 Fixed promotion behavior. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit b01f603 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Thu Aug 16 10:23:38 2018 +0100 Refactored. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit 40f63cb Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Thu Aug 16 09:51:16 2018 +0100 Removed call to uname. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit ac2aaa7 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Wed Aug 15 16:47:03 2018 +0100 Made polling mode auto-rebuild after promotions. This is a temporary solution until automatic build restarts are implemented. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit 55fc699 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Wed Aug 15 16:10:53 2018 +0100 Refactored polling to be more abstract. Polling now has better scheduler API, which allows to use it in `dune runtest` without duplicating too much code. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit 1146d42 Author: Jeremie Dimino <jeremie@dimino.org> Date: Wed Aug 15 13:44:45 2018 +0100 Refactor a bit fswatch_command Avoid computing both commands eagerly. Signed-off-by: Jeremie Dimino <jeremie@dimino.org> commit c046b95 Merge: fe7968d 9804526 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Wed Aug 15 11:03:09 2018 +0100 Merge branch 'master' into watch-mode commit fe7968d Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Wed Aug 15 10:59:27 2018 +0100 Fixed compilation error. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> commit b6612b1 Author: Pavel Senchanka <pavel.senchanka@gmail.com> Date: Wed Aug 15 10:17:04 2018 +0100 Added a simple version of a polling loop. `dune build -w` now behaves in a similar way to Jenga polling mode, automatically rebuilding the project when changes are detected. Note that on platforms other than Linux, you need to have `fswatch` installed for watch mode to work. Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com> Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com>

Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com>

kodek16 · 2018-09-04T16:11:25Z

I've squashed the previous commits (it's nice that Github kept the review history reasonably clean, I wasn't sure it can do this), and fixed all of the suggestions above. @emillon Adding a newline to the message seems to preserve previous lines of Success/Failure, does it look better now?

emillon

The new feedback on build seems good to me. If you can amend the commit message so that it's only the "Add a simple version of the polling loop" it'd be perfect otherwise the concatenated messages are probably good enough 👍

rgrinberg · 2018-09-05T07:29:49Z

@kodek16 give this another rebase and we'll merge after CI.

ghost · 2018-09-05T09:53:39Z

I did "Update branch" followed by "Squash and merge"

@diml

CHANGES: - Ignore stderr output when trying to find out the number of jobs available (ocaml/dune#1118, fix ocaml/dune#1116, @diml) - Fix error message when the source directory of `copy_files` does not exist. (ocaml/dune#1120, fix ocaml/dune#1099, @emillon) - Highlight error locations in error messages (ocaml/dune#1121, @emillon) - Display actual stanza when package is ambiguous (ocaml/dune#1126, fix ocaml/dune#1123, @emillon) - Add `dune unstable-fmt` to format `dune` files. The interface and syntax are still subject to change, so use with caution. (ocaml/dune#1130, fix ocaml/dune#940, @emillon) - Improve error message for `dune utop` without a library name (ocaml/dune#1154, fix ocaml/dune#1149, @emillon) - Fix parsing `ocamllex` stanza in jbuild files (ocaml/dune#1150, @rgrinberg) - Highlight multi-line errors (ocaml/dune#1131, @anuragsoni) - Do no try to generate shared libraries when this is not supported by the OS (ocaml/dune#1165, fix ocaml/dune#1051, @diml) - Fix `Flags.write_{sexp,lines}` in configurator by avoiding the use of `Stdune.Path` (ocaml/dune#1175, fix ocaml/dune#1161, @rgrinberg) - Add support for `findlib.dynload`: when linking an executable using `findlib.dynload`, automatically record linked in libraries and findlib predicates (ocaml/dune#1172, @bobot) - Add support for promoting a selected list of files (ocaml/dune#1192, @diml) - Add an emacs mode providing helpers to promote correction files (ocaml/dune#1192, @diml) - Improve message suggesting to remove parentheses (ocaml/dune#1196, fix ocaml/dune#1173, @emillon) - Add `(wrapped (transition "..message.."))` as an option that will generate wrapped modules but keep unwrapped modules with a deprecation message to preserve compatibility. (ocaml/dune#1188, fix ocaml/dune#985, @rgrinberg) - Fix the flags passed to the ppx rewriter when using `staged_pps` (ocaml/dune#1218, @diml) - Add `(env var)` to add a dependency to an environment variable. (ocaml/dune#1186, @emillon) - Add a simple version of a polling mode: `dune build -w` keeps running and restarts the build when something change on the filesystem (ocaml/dune#1140, @kodek16) - Cleanup the way we detect the library search path. We no longer call `opam config var lib` in the default build context (ocaml/dune#1226, @diml) - Make test stanzas honor the -p flag. (ocaml/dune#1236, fix ocaml/dune#1231, @emillon) - Test stanzas take an optional (action) field to customize how they run (ocaml/dune#1248, ocaml/dune#1195, @emillon) - Add support for private modules via the `private_modules` field (ocaml/dune#1241, fix ocaml/dune#427, @rgrinberg) - Add support for passing arguments to the OCaml compiler via a response file when the list of arguments is too long (ocaml/dune#1256, @diml) - Do not print diffs by default when running inside dune (ocaml/dune#1260, @diml) - Interpret `$ dune build dir` as building the default alias in `dir`. (ocaml/dune#1259, @rgrinberg) - Make the `dynlink` library available without findlib installed (ocaml/dune#1270, fix ocaml/dune#1264, @rgrinberg)

ghost suggested changes Aug 15, 2018

View reviewed changes

emillon reviewed Aug 16, 2018

View reviewed changes

ghost suggested changes Aug 16, 2018

View reviewed changes

bobot mentioned this pull request Aug 21, 2018

Display of ignore-* when an error occurs #1161

Open

kodek16 requested a review from rgrinberg as a code owner August 24, 2018 09:26

ghost suggested changes Aug 29, 2018

View reviewed changes

rgrinberg requested changes Sep 1, 2018

View reviewed changes

ghost suggested changes Sep 3, 2018

View reviewed changes

kodek16 added 2 commits September 4, 2018 10:32

Fix review suggestions.

da8d58a

Signed-off-by: Pavel Senchanka <pavel.senchanka@gmail.com>

ghost approved these changes Sep 4, 2018

View reviewed changes

emillon approved these changes Sep 5, 2018

View reviewed changes

rgrinberg approved these changes Sep 5, 2018

View reviewed changes

Merge branch 'master' into watch-mode

30a4daf

ghost merged commit 8f2a4c0 into ocaml:master Sep 5, 2018

kodek16 deleted the watch-mode branch September 5, 2018 13:57

kodek16 referenced this pull request Sep 6, 2018

Merge branch 'master' into project-digests

e74e752

rgrinberg mentioned this pull request Sep 14, 2018

[new release] dune (1.2.0) ocaml/opam-repository#12622

Merged

This pull request was closed.


		open! Stdune

		type status_line_config = string option * [`Show_jobs \| `Don't_show_jobs]

Conversation

kodek16 commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 15, 2018

Uh oh!

Khady commented Aug 15, 2018 via email

Uh oh!

kodek16 commented Aug 15, 2018

Uh oh!

kodek16 commented Aug 15, 2018

Uh oh!

Khady commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 23, 2018

Uh oh!

ghost commented Aug 29, 2018

Uh oh!

kodek16 commented Aug 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 29, 2018

Uh oh!

ghost commented Aug 29, 2018

Uh oh!

kodek16 commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 3, 2018

Uh oh!

emillon commented Sep 3, 2018

Uh oh!

kodek16 commented Sep 4, 2018

Uh oh!

emillon left a comment

Choose a reason for hiding this comment

Uh oh!

rgrinberg commented Sep 5, 2018

Uh oh!

ghost commented Sep 5, 2018

kodek16 commented Aug 15, 2018 •

edited

Loading