Allow concurrent build with RPC server #11712

gridbugs · 2025-04-30T05:22:31Z

Allow the dune build command to be run concurrently with an RPC server (in either passive or eager watch mode). This works by having the second instance of dune send RPC build command to the RPC server, and the RPC server then sends diagnostic messages back to the first instance of dune.

Currently this is only implemented for the dune build command, and promoting changes into source files is not supported. There are also some subtle differences between the output of dune build is performing the build itself and when it's acting as an RPC client, such as file paths being absolute rather than relative and the underline under code errors losing its color.

Leonidas-from-XIV

I think it's a very nice feature. I know it has some limitations (e.g. can't build in a different profile) but I was glad to see that the target can be specified and I think that covers 80% of all usecases.

It would be nice to have tests that show this mechanism working.

Leonidas-from-XIV · 2025-05-02T08:55:47Z

bin/build_cmd.ml

+    Printf.printf
+      "Error: %s\n%!"
+      (Dyn.to_string (Dune_rpc_private.Response.Error.to_dyn error))
+  | Ok Success -> print_endline "Success"


Should we just print_endline? In case of success I mostly expect Dune to stay silent.

Should actually be using Dune_console. Otherwise it would break the TUI

I've changed it to use Dune_console for now, but I don't feel strongly about changing it to not output anything on success either.

Leonidas-from-XIV · 2025-05-02T08:56:14Z

bin/build_cmd.ml

+      Console.print_user_message main);
+    User_error.raise
+      [ (match List.length errors with
+         | 0 -> Pp.textf "Build failed."


Isn't this a Code_error?

Leonidas-from-XIV · 2025-05-02T08:58:41Z

bin/build_cmd.ml

-    run_build_command ~common ~config ~request
+    match Dune_rpc_client.Where.get () with
+    | Some where ->
+      Scheduler.go_without_rpc_server ~common ~config (fun () -> build_rpc where targets)


It seems weird to call a function go_without_rpc_server that calls build_rpc. Maybe build_rpc should be build_remote to clarify that the build is happening on the far end of the RPC connection?

Renamed to build_via_rpc_server and added some clarifying comments.

Leonidas-from-XIV · 2025-05-02T08:59:17Z

bin/build_cmd.ml

-      Target.interpret_targets (Common.root common) config setup targets
-    in
-    run_build_command ~common ~config ~request
+    match Dune_rpc_client.Where.get () with


Is this how we determine whether the build is happening locally or remotely?

Yep. Clarified with a comment.

Leonidas-from-XIV · 2025-05-02T09:00:38Z

bin/import.ml

+  ;;
+
+  let go ~common ~config f =
    let f =


Maybe rename this to with_rpc to emphasize that this wrapper function will actually enable RPC?

Leonidas-from-XIV · 2025-05-02T09:07:54Z

bin/rpc/build.ml

+      "Error: %s\n%!"
+      (Dyn.to_string (Dune_rpc_private.Response.Error.to_dyn error))
+  | Ok Success -> print_endline "Success"
+  | Ok (Failure _) -> print_endline "Failure"


Failures should presumably not be printed to stdout?

Maybe but I'm not so sure. This code was just refactored in this PR, not added. dune rpc build is not a very user-friendly command (e.g. it needs to be passed sexps on the command-line) so my assumption is that it's intended to be called from scripts. As such it's not unreasonable to expect that scripts would expect the status of the build to be printed to stdout for ease of parsing.

Leonidas-from-XIV · 2025-05-02T09:08:42Z

bin/rpc/build.ml

+  let+ response = build_sexp_string_targets ~wait ~targets where in
+  match response with
+  | Error (error : Dune_rpc_private.Response.Error.t) ->
+    Printf.printf


Should probably go to stderr or even use User_error or something?

Leonidas-from-XIV · 2025-05-02T09:09:48Z

bin/rpc/build.mli

+(** Sends a command to an RPC server to build the specified targets and wait
+    for the build to complete or fail. *)
+val build
+  :  wait:bool


Is the wait argument to wait for the result or to wait for the connection?

For the connection. I've added that info to the doc comment.

otherlibs/dune-rpc/private/exported_types.ml

Leonidas-from-XIV · 2025-05-02T09:14:25Z

src/dune_rpc_impl/decl.ml

+    let open Conv in
+    let from { main; related } = main, related in
+    let to_ (main, related) = make ~main ~related in
+    let main = field "main" (required User_message.sexp_without_annots) in


Why sexp_without_annots here? What are annots?

Clarified with a doc comment on the sexp_without_annots function.

gridbugs · 2025-05-06T02:21:15Z

It would be nice to have tests that show this mechanism working.

Added a test.

gridbugs · 2025-05-06T06:07:01Z

Looks like this change has made some of our rpc tests flaky. Investigating...

gridbugs · 2025-05-06T07:08:46Z

The problem was that I moved Dune_rpc_impl.Where.get () outside of the retry loop so waiting for the server to start wasn't working properly. Should be fixed now.

gridbugs · 2025-05-06T07:11:40Z

Ah looks like something else isn't working now.

gridbugs · 2025-05-06T07:34:03Z

There's a non-deterministic crash with this error:

+|  Internal error, please report upstream including the contents of _build/log.
+|  Description:
+|    ("Memo error handler raised an exception",
+|     { exns =
+|         [ { exn =
+|               "File \"src/dune_engine/build_system.ml\", line 67, characters 11-17: Assertion failed"
+|           ; backtrace =
+|               "Raised at Dune_engine__Build_system.State.update_build_progress_exn in file \"src/dune_engine/build_system.ml\", line 67, characters 11-23\n\
+|                Called from Dune_engine__Build_system.State.add_errors in file \"src/dune_engine/build_system.ml\", lines 86-87, characters 6-73\n\
+|                Called from Dune_engine__Build_system.report_early_exn in file \"src/dune_engine/build_system.ml\", line 1110, characters 14-37\n\
+|                Called from Fiber__Scheduler.exec in file \"vendor/fiber/src/scheduler.ml\", line 76, characters 8-11\n\
+|                "
+|           }
+|         ]
+|     })
+|  Raised at Stdune__Code_error.raise in file
+|    "otherlibs/stdune/src/code_error.ml", line 10, characters 30-62
+|  Called from Fiber__Core.map.(fun) in file "vendor/fiber/src/core.ml", line
+|    68, characters 31-36
+|  Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml",
+|    line 76, characters 8-11
+|
+|    I must not crash.  Uncertainty is the mind-killer. Exceptions are the
+|  little-death that brings total obliteration.  I will fully express my cases.
+|  Execution will pass over me and through me.  And when it has gone past, I
+|  will unwind the stack along its path.  Where the cases are handled there will
+|  be nothing.  Only I will remain.

The loc it points to is:

  let update_build_progress_exn ~f =
    let current = Svar.read t in
    match current with
    | Building current -> Svar.write t @@ Building (f current)
    | _ -> assert false   (* <------ *)
  ;;

I wonder if this is a race between a rebuild by the eager watch server in response to a file change and a build due to an rpc build request. Will investigate more tomorrow.

Allow the `dune build` command to be run concurrently with an RPC server (in either passive or eager watch mode). This works by having the second instance of dune send RPC build command to the RPC server, and the RPC server then sends diagnostic messages back to the first instance of dune. Currently this is only implemented for the `dune build` command, and promoting changes into source files is not supported. There are also some subtle differences between the output of `dune build` is performing the build itself and when it's acting as an RPC client, such as file paths being absolute rather than relative and the underline under code errors losing its color. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

This function evaluates a fiber concurrently with an RPC server, so renaming to clarify this fact. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

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

gridbugs · 2025-05-07T04:06:22Z

I wonder if this is a race between a rebuild by the eager watch server in response to a file change and a build due to an rpc build request.

This turned out to be the issue. I've fixed this but now it looks like it's getting stuck running tests. I can't reproduce locally but I'll try setting up a VM with a single core in case that makes the difference.

gridbugs · 2025-05-07T05:55:08Z

looks like it's getting stuck running tests

This issue seems to have gone away on its own? Maybe I needed to be more patient...

@Alizter

CHANGES: ### Fixed - Fixed a bug that was causing cram tests attached to multiple aliases to be run multiple times. (ocaml/dune#11547, @Alizter) - Fix: pass pkg-config (extra) args in all pkgconfig invocations. A missing --personality flag would result in pkgconf not finding libraries in some contexts. (ocaml/dune#11619, @MisterDA) - Fix: Evaluate `enabled_if` when computing the stubs for stanzas such as `foreign_library` (ocaml/dune#11707, @Alizter, @rgrinberg) - Fix $ dune describe pp for libraries in the presence of `(include_subdirs unqualified)` (ocaml/dune#11729, fixes ocaml/dune#10999, @rgrinberg) - Fix `$ dune subst` in sub directories of a git repository (ocaml/dune#11760, fixes ocaml/dune#11045, @Richard-Degenne) - Fix a crash involving `Path.drop_prefix` when using Melange on Windows (ocaml/dune#11767, @nojb) ### Added - Added detection and warning for common typos in package dependency constraints (ocaml/dune#11600, fixes ocaml/dune#11575, @kemsguy7) - Added `(extra_objects)` field to `(foreign_library)` stanza with `(:include)` support. (ocaml/dune#11683, @Alizter) ### Changed - Allow build RPC messages to be handled by dune's RPC server in eager watch mode (ocaml/dune#11622, @gridbugs) - Allow concurrent build with RPC server (ocaml/dune#11712, @gridbugs)

@Alizter

CHANGES: ### Fixed - Fixed a bug that was causing cram tests attached to multiple aliases to be run multiple times. (ocaml/dune#11547, @Alizter) - Fix: pass pkg-config (extra) args in all pkgconfig invocations. A missing --personality flag would result in pkgconf not finding libraries in some contexts. (ocaml/dune#11619, @MisterDA) - Fix: Evaluate `enabled_if` when computing the stubs for stanzas such as `foreign_library` (ocaml/dune#11707, @Alizter, @rgrinberg) - Fix $ dune describe pp for libraries in the presence of `(include_subdirs unqualified)` (ocaml/dune#11729, fixes ocaml/dune#10999, @rgrinberg) - Fix `$ dune subst` in sub directories of a git repository (ocaml/dune#11760, fixes ocaml/dune#11045, @Richard-Degenne) - Fix a crash involving `Path.drop_prefix` when using Melange on Windows (ocaml/dune#11767, @nojb) ### Added - Added detection and warning for common typos in package dependency constraints (ocaml/dune#11600, fixes ocaml/dune#11575, @kemsguy7) - Added `(extra_objects)` field to `(foreign_library)` stanza with `(:include)` support. (ocaml/dune#11683, @Alizter) ### Changed - Allow build RPC messages to be handled by dune's RPC server in eager watch mode (ocaml/dune#11622, @gridbugs) - Allow concurrent build with RPC server (ocaml/dune#11712, @gridbugs)

gridbugs requested review from Alizter, Leonidas-from-XIV, maiste and rgrinberg April 30, 2025 05:22

gridbugs force-pushed the allow-concurrent-build-with-rpc-server branch from 4b4630c to e7751bb Compare April 30, 2025 05:25

Leonidas-from-XIV reviewed May 2, 2025

View reviewed changes

rgrinberg approved these changes May 3, 2025

View reviewed changes

gridbugs force-pushed the allow-concurrent-build-with-rpc-server branch 2 times, most recently from 4b01bdc to 4fe5dbf Compare May 6, 2025 02:20

gridbugs force-pushed the allow-concurrent-build-with-rpc-server branch from 4f51f49 to 5753bdf Compare May 6, 2025 06:58

maiste mentioned this pull request May 6, 2025

chore: bump dune to 3.19 #11717

Merged

gridbugs added 4 commits May 7, 2025 11:54

Rename Scheduler.go to go_with_rpc_server

96f9e90

This function evaluates a fiber concurrently with an RPC server, so renaming to clarify this fact. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

Document some [Conv] parser combinators

75e3707

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

Replace an [assert false] with a [Code_error]

a59bb63

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

gridbugs force-pushed the allow-concurrent-build-with-rpc-server branch from 5753bdf to a59bb63 Compare May 7, 2025 01:55

gridbugs force-pushed the allow-concurrent-build-with-rpc-server branch from c11e179 to a59bb63 Compare May 7, 2025 05:39

gridbugs merged commit e88e045 into ocaml:main May 7, 2025
41 of 42 checks passed

maiste mentioned this pull request May 20, 2025

[new release] dune (15 packages) (3.19.0~alpha0) ocaml/opam-repository#27905

Closed

maiste mentioned this pull request May 21, 2025

[new release] dune (15 packages) (3.19.0) ocaml/opam-repository#27910

Merged

ElectreAAS mentioned this pull request Jul 2, 2025

[RPC] Allow formatting while watch server is running #11958

Closed

ElectreAAS mentioned this pull request Oct 14, 2025

Running tests and exec-ing executables should be possible while build server is running #8114

Closed

Allow concurrent build with RPC server #11712

Allow concurrent build with RPC server #11712

Uh oh!

Conversation

gridbugs commented Apr 30, 2025

Uh oh!

Leonidas-from-XIV left a comment

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gridbugs commented May 6, 2025

Uh oh!

gridbugs commented May 6, 2025

Uh oh!

gridbugs commented May 6, 2025

Uh oh!

gridbugs commented May 6, 2025

Uh oh!

gridbugs commented May 6, 2025

Uh oh!

gridbugs commented May 7, 2025

Uh oh!

gridbugs commented May 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants