From c142534cb30c44684181eace4266e7d16521a064 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ulysse=20G=C3=A9rard?= Date: Mon, 4 Sep 2023 16:38:59 +0200 Subject: [PATCH 1/3] tests: showing bad error messages when config reader is missing --- tests/test-dirs/config/no-dune.t | 37 ++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 tests/test-dirs/config/no-dune.t diff --git a/tests/test-dirs/config/no-dune.t b/tests/test-dirs/config/no-dune.t new file mode 100644 index 0000000000..30a1c35d68 --- /dev/null +++ b/tests/test-dirs/config/no-dune.t @@ -0,0 +1,37 @@ + $ mkdir bin + $ cp ../../../../install/default/bin/ocamlmerlin bin/ocamlmerlin + $ cp ../../../../install/default/bin/ocamlmerlin-server bin/ocamlmerlin-server + + $ cat >dune-project < (lang dune 2.0) + > EOF + + $ cat >main.ml < print_endline "Hello world" + > EOF + + $ PATH=bin ocamlmerlin single dump-configuration \ + > -filename main.ml output + +Not sure why merlin complains about an unknown flag here. +Fixme: we could provide a better error message + $ cat output | jq '.value.merlin.failures' + [ + "unknown flag main.ml", + "flag -filename: error, Unix.Unix_error(Unix.ENOENT, \"create_process\", \"dune\")" + ] + + $ cat >.merlin < S . + > EOF + + $ PATH=bin ocamlmerlin single dump-configuration \ + > -filename main.ml output + +Not sure why merlin complains about an unknown flag here. +Fixme: we could provide a better error message + $ cat output | jq '.value.merlin.failures' + [ + "unknown flag main.ml", + "flag -filename: error, Unix.Unix_error(Unix.ENOENT, \"create_process\", \"dot-merlin-reader\")" + ] From a13c2a4d62fd823fdd7773565e430529aa0c7aa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ulysse=20G=C3=A9rard?= Date: Mon, 4 Sep 2023 16:41:16 +0200 Subject: [PATCH 2/3] Improve error messages when config reader is missing. And close unused pipes / reset cwd when starting the reader failed. --- src/kernel/mconfig_dot.ml | 51 ++++++++++++++++++++++++-------- tests/test-dirs/config/no-dune.t | 10 ++----- 2 files changed, 40 insertions(+), 21 deletions(-) diff --git a/src/kernel/mconfig_dot.ml b/src/kernel/mconfig_dot.ml index c4e5915217..13ad8eba99 100644 --- a/src/kernel/mconfig_dot.ml +++ b/src/kernel/mconfig_dot.ml @@ -85,6 +85,8 @@ module Configurator : sig val of_string_opt : string -> t option val to_string : t -> string + exception Process_exited + module Process : sig type nonrec t = { kind: t; @@ -98,8 +100,9 @@ module Configurator : sig (* [Some] if the process is live, [None] if the process died immediately after spawning. The check is a bit fragile, but is principally there to check if `dot-merlin-reader` isn't installed or isn't on the PATH; it only needs to - be best-effort besides that. *) - val get_process : dir:string -> t -> Process.t option + be best-effort besides that. This function can raise [Process_exited] and + [Unix_error]. *) + val get_process_exn : dir:string -> t -> Process.t end = struct type t = | Dot_merlin @@ -116,6 +119,8 @@ end = struct | Dot_merlin -> "dot-merlin-reader" | Dune -> "dune" + exception Process_exited + module Process = struct type nonrec t = { kind : t; @@ -167,7 +172,17 @@ end = struct log ~title:"get_config" "Starting %s configuration provider from dir %s." (to_string cfg) dir; - let pid = Unix.create_process prog args stdin_r stdout_w stderr_w in + + let pid = + let open Unix in + try create_process prog args stdin_r stdout_w stderr_w + with Unix_error _ as err -> + Os_ipc.merlin_dont_inherit_stdio false; + chdir cwd; + List.iter ~f:close + [stdin_r; stdin_w; stdout_r; stdout_w; stderr_r; stderr_w]; + raise err + in Os_ipc.merlin_dont_inherit_stdio false; Unix.chdir cwd; Unix.close stdin_r; @@ -205,13 +220,13 @@ end = struct Hashtbl.add running_processes (dir, configurator) p; p - let get_process ~dir configurator = + let get_process_exn ~dir configurator = let p = get_process_with_pid ~dir configurator in match Unix.waitpid [ WNOHANG ] p.pid with - | 0, _ -> Some p.process + | 0, _ -> p.process | _ -> begin Hashtbl.remove running_processes (dir, configurator); - None + raise Process_exited end end @@ -270,7 +285,6 @@ type context = { process_dir: string; } -exception Process_exited exception End_of_input let get_config { workdir; process_dir; configurator } path_abs = @@ -291,11 +305,7 @@ let get_config { workdir; process_dir; configurator } path_abs = read p.stdout in try - let p = - match Configurator.get_process ~dir:process_dir configurator with - | Some p -> p - | None -> raise Process_exited - in + let p = Configurator.get_process_exn ~dir:process_dir configurator in (* Both [p.initial_cwd] and [path_abs] have gone through [canonicalize_filename] *) let path_rel = @@ -334,7 +344,7 @@ let get_config { workdir; process_dir; configurator } path_abs = | Error (Merlin_dot_protocol.Unexpected_output msg) -> empty_config, [ msg ] | Error (Merlin_dot_protocol.Csexp_parse_error _) -> raise End_of_input with - | Process_exited -> + | Configurator.Process_exited -> (* This can happen - If `dot-merlin-reader` is not installed and the project use `.merlin` files @@ -350,6 +360,21 @@ let get_config { workdir; process_dir; configurator } path_abs = program_name in empty_config, [ error ] + | Unix.Unix_error (ENOENT, "create_process", "dune") -> + let error = Printf.sprintf + "%s could not find `dune` in the PATH to get project configuration. \ + If you do not rely on Dune, make sure `.merlin` files are present in \ + the project's sources." + (Lib_config.program_name ()) + in + empty_config, [ error ] + | Unix.Unix_error (ENOENT, "create_process", "dot-merlin-reader") -> + let error = Printf.sprintf + "%s could not find `dot-merlin-reader` in the PATH. Please make sure \ + that `dot-merlin-reader` is installed and in the PATH." + (Lib_config.program_name ()) + in + empty_config, [ error ] | End_of_input -> (* This can happen - if a project using old-dune has not been built and Merlin wrongly tries to diff --git a/tests/test-dirs/config/no-dune.t b/tests/test-dirs/config/no-dune.t index 30a1c35d68..093914a4d4 100644 --- a/tests/test-dirs/config/no-dune.t +++ b/tests/test-dirs/config/no-dune.t @@ -13,12 +13,9 @@ $ PATH=bin ocamlmerlin single dump-configuration \ > -filename main.ml output -Not sure why merlin complains about an unknown flag here. -Fixme: we could provide a better error message $ cat output | jq '.value.merlin.failures' [ - "unknown flag main.ml", - "flag -filename: error, Unix.Unix_error(Unix.ENOENT, \"create_process\", \"dune\")" + "Merlin could not find `dune` in the PATH to get project configuration. If you do not rely on Dune, make sure `.merlin` files are present in the project's sources." ] $ cat >.merlin < -filename main.ml output -Not sure why merlin complains about an unknown flag here. -Fixme: we could provide a better error message $ cat output | jq '.value.merlin.failures' [ - "unknown flag main.ml", - "flag -filename: error, Unix.Unix_error(Unix.ENOENT, \"create_process\", \"dot-merlin-reader\")" + "Merlin could not find `dot-merlin-reader` in the PATH. Please make sure that `dot-merlin-reader` is installed and in the PATH." ] From ba7ceab2a25e45488784106506f93c02902b6027 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ulysse=20G=C3=A9rard?= Date: Mon, 4 Sep 2023 16:43:30 +0200 Subject: [PATCH 3/3] Add changelog entry for #1669 --- CHANGES.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 347583bec3..64e95aff4c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,12 @@ +merlin 4.11 +=========== +unreleased + + + merlin binary + - Improve error messages for missing configuration reader (#1669) + + + merlin 4.10 ========== Thu Aug 24 17:17:42 CEST 2023