fix: disallow multiple build commands #6360

rgrinberg · 2022-10-30T01:58:31Z

Okay this improves on #6244 by providing some lock file functionality. @Alizter do you mind giving this some thorough testing?

Alizter

Looks good. I've tested doing simultaneous dune buildss both with and without watch mode and I am getting the correct error message

Error: Dune already running in this build directory. If this is not the case,
delete _build/.rpc/dune

Now there is a good reason to actually want to do a simultaneous build, say you are running dune build @check -w and it is waiting, but you want to build some other target. At the moment you will have to stop the build in watch mode to do this, or modify it.

I suppose there is room for improvement here where other Dune build calls can send build requests to the currently running RPC server. But I guess that can come in a later PR.

src/dune_rpc_impl/server.ml

Alizter · 2022-10-30T20:25:32Z

Also this does not fix #5449. I imagine making dune clean observe the lock file wouldn't be too difficult to do.

rgrinberg · 2022-10-31T15:42:11Z

@gridbugs I've added you as a reviewer as well. It will be good to familiarize with other parts of the codebase. It's a fairly large PR, so there's plenty to review for both you and Ali.

A good background reading is:

Stabilizing the Action Plugin #5681 - justifies why we need to be able to run the rpc server on demand
running multiple instances of dune in parallel fails #236 - the various locking schemes proposed.

Alizter · 2022-11-01T01:33:12Z

I wasn't able to reproduce the issue from before, so it is was probably my mistake.

I've tested the new version both in watch mode and without and there are no issues and the error message is displayed correctly. Builds in different workspaces can continue together also.

Alizter

I'm happy with everything apart from the flock C code, since I don't understand it just yet.

I see that #5449 is now fixed!

rgrinberg · 2022-11-02T14:31:31Z

ping @gridbugs

@Alizter I've updated a few more things. Worth another look.

src/dune_util/global_lock.ml

Running dune concurrently is no longer allowed because we create the rpc socket file eagerly and treat it as a lock Signed-off-by: Rudi Grinberg <[email protected]> ps-id: 491f09b0-a316-43b8-8df5-a05f53fa331a

rgrinberg · 2022-11-07T14:48:34Z

Will merge this tomorrow unless anyone objects.

emillon · 2022-11-08T16:17:52Z

Should this get a changelog entry?

Blaisorblade · 2022-11-10T15:30:17Z

@emillon 👍 I was about to ask the same — since this is a user-visible behavior change IIUC?

rgrinberg · 2022-11-10T15:31:16Z

A CHANGES entry was added in a subsequent PR. Rudi.

…

On Nov 10, 2022, 9:30 AM -0600, Paolo G. Giarrusso ***@***.***>, wrote: @emillon 👍 I was about to ask the same — since this is a user-visible behavior change IIUC? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

@rgrinberg

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.6.0) CHANGES: - Forbid multiple instances of dune running concurrently in the same workspace. (ocaml/dune#6360, fixes ocaml/dune#236, @rgrinberg) - Allow promoting into source directories specified by `subdir` (ocaml/dune#6404, fixes ocaml/dune#3502, @rgrinberg) - Make dune describe workspace return the correct root path (ocaml/dune#6380, fixes ocaml/dune#6379, @esope) - Introduce a `$ dune ocaml top-module` subcommand to load modules directly without sealing them behind the signature. (ocaml/dune#5940, @rgrinberg) - [ctypes] do not mangle user written names in the ctypes stanza (ocaml/dune#6374, fixes ocaml/dune#5561, @rgrinberg) - Support `CLICOLOR` and `CLICOLOR_FORCE` to enable/disable/force ANSI colors. (ocaml/dune#6340, fixes ocaml/dune#6323, @MisterDA). - Forbid private libraries with `(package ..)` set from depending on private libraries that don't belong to a package (ocaml/dune#6385, fixes ocaml/dune#6153, @rgrinberg) - Allow `Byte_complete` binaries to be installable (ocaml/dune#4873, @AltGr, @rgrinberg) - Revive `$ dune external-lib-deps` under `$ dune describe external-lib-deps`. (ocaml/dune#6045, @moyodiallo) - Fix running inline tests in bytecode mode (ocaml/dune#5622, fixes ocaml/dune#5515, @dariusf) - [ctypes] always re-run `pkg-config` because we aren't tracking its external dependencies (ocaml/dune#6052, @rgrinberg) - [ctypes] remove dependency on configurator in the generated rules (ocaml/dune#6052, @rgrinberg) - Build progress status now shows number of failed jobs (ocaml/dune#6242, @Alizter) - Allow absolute build directories to find public executables. For example, those specified with `(deps %{bin:...})` (ocaml/dune#6326, @anmonteiro) - Create a fake socket file `_build/.rpc/dune` on windows to allow rpc clients to connect using the build directory. (ocaml/dune#6329, @rgrinberg) - Prevent crash if absolute paths are used in the install stanza and in recursive globs. These cases now result in a user error. (ocaml/dune#6331, @gridbugs) - Add `(glob_files <glob>)` and `(glob_files_rec <glob>)` terms to the `files` field of the `install` stanza (ocaml/dune#6250, closes ocaml/dune#6018, @gridbugs) - Allow `:standard` in the `(modules)` field of the `coq.pp` stanza (ocaml/dune#6229, fixes ocaml/dune#2414, @Alizter) - Fix passing of flags to dune coq top (ocaml/dune#6369, fixes ocaml/dune#6366, @Alizter) - Extend the promotion CLI to a `dune promotion` group: `dune promote` is moved to `dune promotion apply` (the former still works) and the new `dune promotion diff` command can be used to just display the promotion without applying it. (ocaml/dune#6160, fixes ocaml/dune#5368, @emillon)

nojb · 2022-11-19T20:09:36Z

src/dune_util/dune_flock.c

+
+CAMLprim value dune_flock_lock(value v_fd, value v_block, value v_exclusive) {
+#ifdef _WIN32
+  caml_failwith("no flock on win32");


There's LockFileEx on Windows https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-lockfileex which seems pretty similar to flock. Perhaps it can be used to implement this C stub on Windows with similar semantics which would allow using a single codepath in the OCaml side.

That would be useful indeed.

rgrinberg mentioned this pull request Oct 30, 2022

feature: lazy initialize RPC #6244

Closed

rgrinberg requested a review from Alizter October 30, 2022 01:59

rgrinberg changed the title ~~fix: disallow multiple rpc servers~~ fix: disallow multiple rpc servers and build commands Oct 30, 2022

rgrinberg linked an issue Oct 30, 2022 that may be closed by this pull request

running multiple instances of dune in parallel fails #236

Closed

rgrinberg added this to the 3.6.0 milestone Oct 30, 2022

rgrinberg force-pushed the ps/rr/fix__disallow_multiple_rpc_servers branch from aa4fa9d to 30de814 Compare October 30, 2022 02:03

rgrinberg changed the title ~~fix: disallow multiple rpc servers and build commands~~ fix: disallow multiple build commands Oct 30, 2022

Alizter approved these changes Oct 30, 2022

View reviewed changes

src/dune_rpc_impl/server.ml Outdated Show resolved Hide resolved

src/dune_rpc_impl/server.ml Outdated Show resolved Hide resolved

rgrinberg force-pushed the ps/rr/fix__disallow_multiple_rpc_servers branch 6 times, most recently from cf597a3 to 205a082 Compare October 31, 2022 13:49

rgrinberg requested a review from gridbugs October 31, 2022 15:37

This comment was marked as resolved.

Sign in to view

rgrinberg force-pushed the ps/rr/fix__disallow_multiple_rpc_servers branch 3 times, most recently from 9815f43 to 7919af4 Compare November 1, 2022 01:26

Alizter approved these changes Nov 1, 2022

View reviewed changes

rgrinberg force-pushed the ps/rr/fix__disallow_multiple_rpc_servers branch 7 times, most recently from cbbb9e2 to 0b71972 Compare November 1, 2022 20:58

rgrinberg force-pushed the ps/rr/fix__disallow_multiple_rpc_servers branch 2 times, most recently from efd816a to b5d4221 Compare November 2, 2022 01:44

rgrinberg mentioned this pull request Nov 2, 2022

Corner case: executing dune build & dune utop at the same time fails #6286

Closed

rgrinberg force-pushed the ps/rr/fix__disallow_multiple_rpc_servers branch from b5d4221 to 18e69a9 Compare November 2, 2022 14:28

gridbugs reviewed Nov 3, 2022

View reviewed changes

src/dune_util/global_lock.ml Outdated Show resolved Hide resolved

rgrinberg force-pushed the ps/rr/fix__disallow_multiple_rpc_servers branch 2 times, most recently from 93a7a0b to 64090d5 Compare November 3, 2022 17:36

fix: disallow multiple build commands

9f97019

Running dune concurrently is no longer allowed because we create the rpc socket file eagerly and treat it as a lock Signed-off-by: Rudi Grinberg <[email protected]> ps-id: 491f09b0-a316-43b8-8df5-a05f53fa331a

rgrinberg force-pushed the ps/rr/fix__disallow_multiple_rpc_servers branch from 64090d5 to 9f97019 Compare November 4, 2022 21:09

rgrinberg merged commit 833e99a into main Nov 8, 2022

emillon mentioned this pull request Nov 8, 2022

dune 3.0 crashes badly when doing dune build -w, and dune clean in same repo #5449

Closed

Alizter deleted the ps/rr/fix__disallow_multiple_rpc_servers branch November 12, 2022 15:15

emillon mentioned this pull request Nov 14, 2022

[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.6.0) ocaml/opam-repository#22498

Merged

nojb mentioned this pull request Nov 19, 2022

Fix MSVC build #6517

Merged

nojb reviewed Nov 19, 2022

View reviewed changes

nojb mentioned this pull request Nov 19, 2022

Implement flock on Windows with LockFileEx + 'dune clean' fix on Windows #6523

Merged

jchavarri mentioned this pull request Dec 3, 2022

dune build -w restricting job number for no reason #5549

Open

emillon mentioned this pull request Jan 30, 2023

Making _build dir locking opt-in #6967

Closed

jchavarri mentioned this pull request Mar 13, 2023

Enable running two builds in separate terminals (a.k.a. dune monitor) #7308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: disallow multiple build commands #6360

fix: disallow multiple build commands #6360

rgrinberg commented Oct 30, 2022 •

edited

Loading

Alizter left a comment

Alizter commented Oct 30, 2022

rgrinberg commented Oct 31, 2022

This comment was marked as resolved.

Alizter commented Nov 1, 2022

Alizter left a comment

rgrinberg commented Nov 2, 2022

rgrinberg commented Nov 7, 2022

emillon commented Nov 8, 2022

Blaisorblade commented Nov 10, 2022

rgrinberg commented Nov 10, 2022 via email

nojb Nov 19, 2022

rgrinberg Nov 19, 2022

nojb Nov 19, 2022

fix: disallow multiple build commands #6360

fix: disallow multiple build commands #6360

Conversation

rgrinberg commented Oct 30, 2022 • edited Loading

Alizter left a comment

Choose a reason for hiding this comment

Alizter commented Oct 30, 2022

rgrinberg commented Oct 31, 2022

This comment was marked as resolved.

Alizter commented Nov 1, 2022

Alizter left a comment

Choose a reason for hiding this comment

rgrinberg commented Nov 2, 2022

rgrinberg commented Nov 7, 2022

emillon commented Nov 8, 2022

Blaisorblade commented Nov 10, 2022

rgrinberg commented Nov 10, 2022 via email

nojb Nov 19, 2022

Choose a reason for hiding this comment

rgrinberg Nov 19, 2022

Choose a reason for hiding this comment

nojb Nov 19, 2022

Choose a reason for hiding this comment

rgrinberg commented Oct 30, 2022 •

edited

Loading