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

test: reproduce dune exec -w crash with pkg management #10960

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions bin/exec.ml
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,9 @@ module Command_to_exec = struct

type t =
{ get_path_and_build_if_necessary :
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps call this get_env_and_build_if_necessary.

string -> (Path.t, [ `Already_reported ]) result Fiber.t
string -> (Path.t * Env.t, [ `Already_reported ]) result Fiber.t
; prog : string
; args : string list
; env : Env.t
}

(* Helper function to spawn a new process running a command in an
Expand All @@ -90,9 +89,9 @@ module Command_to_exec = struct

(* Run the command, first (re)building the program which the command is
invoking *)
let build_and_run_in_child_process { get_path_and_build_if_necessary; prog; args; env } =
let build_and_run_in_child_process { get_path_and_build_if_necessary; prog; args } =
get_path_and_build_if_necessary prog
|> Fiber.map ~f:(Result.map ~f:(spawn_process ~args ~env))
|> Fiber.map ~f:(Result.map ~f:(fun (p, env) -> spawn_process ~args ~env p))
;;
end

Expand Down Expand Up @@ -306,8 +305,7 @@ module Exec_context = struct
Memo.run
@@
let open Memo.O in
let* env = env
and* sctx = sctx in
let* sctx = sctx in
let expand = Cmd_arg.expand ~root:(Common.root common) ~sctx in
let* prog = expand prog in
let+ args = Memo.parallel_map args ~f:expand in
Expand All @@ -316,10 +314,12 @@ module Exec_context = struct
(* 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 (fun () -> get_path_and_build_if_necessary ~prog))
build (fun () ->
let+ env = env
and+ path = get_path_and_build_if_necessary ~prog in
path, env))
; prog
; args
; env
}
in
Watch.loop ~command_to_exec
Expand Down
44 changes: 44 additions & 0 deletions test/blackbox-tests/test-cases/pkg/gh10959.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
Repro `dune exec --watch` crash with pkg management

$ . ./helpers.sh

$ cat >dune-project <<EOF
> (lang dune 3.13)
> (pin
> (url file://$PWD/_multiple)
> (package (name foo))
> (package (name bar)))
> (package
> (name main)
> (depends foo bar))
> EOF

$ cat >dune <<EOF
> (executable
> (name x))
> EOF
$ cat >x.ml <<EOF
> let () = print_endline "hello"
> EOF

$ mkdir dune.lock
$ cat >dune.lock/lock.dune <<EOF
> (lang package 0.1)
> (repositories
> (complete true))
> EOF

$ cat >dune.lock/test.pkg <<EOF
> (version 0.0.1)
> (build
> (system "echo building test"))
> EOF

$ dune exec -w ./x.exe > output.log 2>&1 &
$ PID=$!
$ sleep 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 seems long, what about 0.1? The other exec-watch tests appear to pick something like that. Although killing processes this was seems very fragile, there ought to be a more principled way to test these. Not this PRs problem however.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The watching/ tests use some better helpers. Up to you if you want to improve things. Also I would move this from pkg to exec-watch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you point me to a relevant example?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They have these start_dune stop_dune helpers from a shared script (see the dune file for where it is). A test example is: https://github.com/ocaml/dune/blob/main/test/blackbox-tests/test-cases/watching/copy-rules.t

The logic in the script handles stopping the watch mode build and the output. Presumably something like that could be adapted for exec. But if this is too much work that is also fine, since the other exec-watch/ tests are written this way.

$ cat output.log
building test
Success, waiting for filesystem changes...
hello
$ kill $PID
Loading