Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for eager watch mode for dune exec #6966

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

gridbugs
Copy link
Collaborator

This is the second attempt at adding this feature. The problem with the first attempt was that replacing the call to Scheduler.go with Scheduler.go_with_rpc_server_and_console_status_reporting caused occasional non-deterministic seg faults on macos. There's some info about the problem on the PR which reverts the previous attempt at adding this feature: #6867. At the time of writing we don't know the cause of this problem.

The difference this time around is that we maintain the call to Scheduler.go when running dune exec not in watch mode, and only invoke Scheduler.go_with_rpc_server_and_console_status_reporting when running in watch mode. There is some additional refactoring done to make this split more ergonomic. We may see seg faults on macos when running exec in watch mode but at least we won't introduce the potential for seg faults into the existing use case of running exec not in watch mode (assuming of course that there is a causal link between go_with_rpc_server_and_console_status_reporting and the seg fault on macos, which is not necessarily the case).

Signed-off-by: Stephen Sherratt stephen@sherra.tt

This is the second attempt at adding this feature. The problem with the
first attempt was that replacing the call to `Scheduler.go` with
`Scheduler.go_with_rpc_server_and_console_status_reporting` caused
occasional non-deterministic seg faults on macos. There's some info
about the problem on the PR which reverts the previous attempt at adding
this feature: ocaml#6867. At the time of
writing we don't know the cause of this problem.

The difference this time around is that we maintain the call to
`Scheduler.go` when running `dune exec` not in watch mode, and only
invoke `Scheduler.go_with_rpc_server_and_console_status_reporting` when
running in watch mode. There is some additional refactoring done to make
this split more ergonomic. We may see seg faults on macos when running
exec in watch mode but at least we won't introduce the potential for seg
faults into the existing use case of running exec not in watch mode
(assuming of course that there is a causal link between
`go_with_rpc_server_and_console_status_reporting` and the seg fault on
macos, which is not necessarily the case).

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs
Copy link
Collaborator Author

The original version of this change is this PR: #6507, and the PR where it is reverted is:#6867. The only difference between that change and this one is in bin/exec.ml. Here is the diff produced by running git diff gridbugs/exec-watch:bin/exec.ml gridbugs/exec-watch-2:bin/exec.ml:

diff --git a/bin/exec.ml b/bin/exec.ml
index a0b3df2af..4408b9558 100644
--- a/bin/exec.ml
+++ b/bin/exec.ml
@@ -179,6 +179,71 @@ let get_path_and_build_if_necessary sctx ~no_rebuild ~dir ~prog =
     | Some prog -> Memo.return prog
     | None -> not_found ~dir ~prog)
 
+module Exec_context = struct
+  type t =
+    { common : Common.t
+    ; config : Dune_config.t
+    ; args : string list
+    ; env : Env.t Fiber.t
+    ; get_path_and_build_if_necessary : (unit -> Path.t Memo.t) Fiber.t
+    }
+
+  let init ~common ~context ~no_rebuild ~prog ~args =
+    (* The initialization of some fields is deferred until the fiber scheduler
+       has been started. *)
+    let config = Common.init common in
+    let sctx =
+      let open Fiber.O in
+      let* setup = Import.Main.setup () in
+      let+ setup = Memo.run setup in
+      Import.Main.find_scontext_exn setup ~name:context
+    in
+    let dir =
+      Fiber.map sctx ~f:(fun sctx ->
+          let context = Dune_rules.Super_context.context sctx in
+          Path.Build.relative context.build_dir (Common.prefix_target common ""))
+    in
+    let env = Fiber.map sctx ~f:Super_context.context_env in
+    let get_path_and_build_if_necessary =
+      let open Fiber.O in
+      let* sctx = sctx in
+      let+ dir = dir in
+      fun () -> get_path_and_build_if_necessary sctx ~no_rebuild ~dir ~prog
+    in
+    { common; config; env; args; get_path_and_build_if_necessary }
+
+  let run_once { common; config; env; args; get_path_and_build_if_necessary; _ }
+      =
+    Scheduler.go ~common ~config @@ fun () ->
+    let open Fiber.O in
+    let* get_path_and_build_if_necessary = get_path_and_build_if_necessary in
+    let* env = env in
+    let+ path = Build_system.run_exn get_path_and_build_if_necessary in
+    let prog = Path.to_string path in
+    let argv = prog :: args in
+    restore_cwd_and_execve common prog argv env
+
+  let run_eager_watch
+      { common; config; env; args; get_path_and_build_if_necessary; _ } =
+    Scheduler.go_with_rpc_server_and_console_status_reporting ~common ~config
+    @@ fun () ->
+    let open Fiber.O in
+    let* get_path_and_build_if_necessary = get_path_and_build_if_necessary in
+    let* env = env in
+    let command_to_exec =
+      { Command_to_exec.get_path_and_build_if_necessary =
+          (fun () ->
+            (* TODO we should release the dune lock. But we aren't doing it
+               because we don't unload the database files we've marshalled.
+            *)
+            Build_system.run get_path_and_build_if_necessary)
+      ; args
+      ; env
+      }
+    in
+    Watch.loop ~command_to_exec
+end
+
 let term =
   let+ common = Common.term
   and+ context =
@@ -190,51 +255,16 @@ let term =
       value & flag
       & info [ "no-build" ] ~doc:"don't rebuild target before executing")
   and+ args = Arg.(value & pos_right 0 string [] (Arg.info [] ~docv:"ARGS")) in
-  let config = Common.init common in
   (* TODO we should make sure to finalize the current backend before exiting dune.
      For watch mode, we should finalize the backend and then restart it in between
      runs. *)
-  match
-    Scheduler.go_with_rpc_server_and_console_status_reporting ~common ~config
-    @@ fun () ->
-    let open Fiber.O in
-    let* setup = Import.Main.setup () in
-    let* setup = Memo.run setup in
-    let sctx = Import.Main.find_scontext_exn setup ~name:context in
-    let get_path_and_build_if_necessary =
-      let context = Dune_rules.Super_context.context sctx in
-      let dir =
-        Path.Build.relative context.build_dir (Common.prefix_target common "")
-      in
-      fun () -> get_path_and_build_if_necessary sctx ~no_rebuild ~dir ~prog
-    in
-    let env = Super_context.context_env sctx in
-    match Common.watch common with
-    | Yes Passive ->
-      User_error.raise [ Pp.textf "passive watch mode is unsupported by exec" ]
-    | Yes Eager ->
-      let command_to_exec =
-        { Command_to_exec.get_path_and_build_if_necessary =
-            (fun () ->
-              (* TODO we should release the dune lock. But we aren't doing it
-                 because we don't unload the database files we've marshalled.
-              *)
-              Build_system.run get_path_and_build_if_necessary)
-        ; args
-        ; env
-        }
-      in
-      let+ () = Watch.loop ~command_to_exec in
-      `In_watch_mode_so_should_loop_forever_and_never_get_here
-    | No ->
-      let+ path = Build_system.run_exn get_path_and_build_if_necessary in
-      `Execve_thunk
-        (fun () ->
-          let prog = Path.to_string path in
-          let argv = prog :: args in
-          restore_cwd_and_execve common prog argv env)
-  with
-  | `In_watch_mode_so_should_loop_forever_and_never_get_here -> ()
-  | `Execve_thunk thunk -> thunk ()
+  let exec_context =
+    Exec_context.init ~common ~context ~no_rebuild ~prog ~args
+  in
+  match Common.watch common with
+  | Yes Passive ->
+    User_error.raise [ Pp.textf "passive watch mode is unsupported by exec" ]
+  | Yes Eager -> Exec_context.run_eager_watch exec_context
+  | No -> Exec_context.run_once exec_context
 
 let command = Cmd.v info term

@rgrinberg rgrinberg added this to the 3.7.0 milestone Feb 16, 2023
@rgrinberg rgrinberg merged commit ec504c6 into ocaml:main Feb 16, 2023
emillon added a commit to emillon/opam-repository that referenced this pull request Feb 17, 2023
…, 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)
moyodiallo pushed a commit to moyodiallo/dune that referenced this pull request Apr 3, 2023
This is the second attempt at adding this feature. The problem with the
first attempt was that replacing the call to `Scheduler.go` with
`Scheduler.go_with_rpc_server_and_console_status_reporting` caused
occasional non-deterministic seg faults on macos. There's some info
about the problem on the PR which reverts the previous attempt at adding
this feature: ocaml#6867. At the time of
writing we don't know the cause of this problem.

The difference this time around is that we maintain the call to
`Scheduler.go` when running `dune exec` not in watch mode, and only
invoke `Scheduler.go_with_rpc_server_and_console_status_reporting` when
running in watch mode. There is some additional refactoring done to make
this split more ergonomic. We may see seg faults on macos when running
exec in watch mode but at least we won't introduce the potential for seg
faults into the existing use case of running exec not in watch mode
(assuming of course that there is a causal link between
`go_with_rpc_server_and_console_status_reporting` and the seg fault on
macos, which is not necessarily the case).

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs gridbugs deleted the exec-watch-2 branch October 11, 2023 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants