From 866497a7d727664c527af3af69f61459dd1cc4a9 Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Sat, 9 Oct 2021 11:35:42 -0600 Subject: [PATCH 1/5] test: reproduce syntax error as in #4230 Signed-off-by: Rudi Grinberg --- test/blackbox-tests/test-cases/gh4230.t | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 test/blackbox-tests/test-cases/gh4230.t diff --git a/test/blackbox-tests/test-cases/gh4230.t b/test/blackbox-tests/test-cases/gh4230.t new file mode 100644 index 00000000000..8e02ed322a2 --- /dev/null +++ b/test/blackbox-tests/test-cases/gh4230.t @@ -0,0 +1,15 @@ +Syntax error inside a cram command + $ mkdir foo && cd foo + $ cat >dune-project < (lang dune 3.0) + > EOF + + $ cat >t1.t < $ foo-bar() { true; } + > EOF + + $ dune runtest --auto-promote + sh (internal) (exit 2) + (cd _build/.sandbox/cdb38568b2ab5eaeeab253debbcff1a1/default && /run/current-system/sw/bin/sh /var/folders/nc/x9_nmmsj0rjbfyzxb_kjk6qr0000gn/T/build_75e3fc_dune/dune_cram_b748db_.t1.t/main.sh) + -> required by alias runtest + [1] From 2b41333e0f27fec9e60388259434dd27b5057082 Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Sat, 9 Oct 2021 16:24:03 -0600 Subject: [PATCH 2/5] fix(cram): improve syntax errors At least make it possible to see the true. Unfortuantely, sh doesn't let us actually recover from it. Signed-off-by: Rudi Grinberg --- src/dune_rules/cram_exec.ml | 2 +- test/blackbox-tests/test-cases/gh4230.t | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dune_rules/cram_exec.ml b/src/dune_rules/cram_exec.ml index 8607861818d..5b1d69d952b 100644 --- a/src/dune_rules/cram_exec.ml +++ b/src/dune_rules/cram_exec.ml @@ -357,7 +357,7 @@ let create_sh_script cram_stanzas ~temp_dir : sh_script Fiber.t = let+ user_shell_code_output_file_sh_path = sh_path user_shell_code_output_file in - fprln oc ". %s > %s 2>&1" user_shell_code_file_sh_path + fprln oc "2>&1 . %s > %s" user_shell_code_file_sh_path user_shell_code_output_file_sh_path; fprln oc {|printf "%%d\0%%s\0" $? "$%s" >> %s|} Action_exec._BUILD_PATH_PREFIX_MAP metadata_file_sh_path; diff --git a/test/blackbox-tests/test-cases/gh4230.t b/test/blackbox-tests/test-cases/gh4230.t index 8e02ed322a2..9558dd69897 100644 --- a/test/blackbox-tests/test-cases/gh4230.t +++ b/test/blackbox-tests/test-cases/gh4230.t @@ -8,8 +8,8 @@ Syntax error inside a cram command > $ foo-bar() { true; } > EOF - $ dune runtest --auto-promote + $ dune runtest --auto-promote 2>&1 | sed -E -e 's/.+\.sh:/$SUBTEST.sh:/' -e 's/cd.*\&\&.*.sh/cd $DIR \&\& $SUBTEST.sh/' sh (internal) (exit 2) - (cd _build/.sandbox/cdb38568b2ab5eaeeab253debbcff1a1/default && /run/current-system/sw/bin/sh /var/folders/nc/x9_nmmsj0rjbfyzxb_kjk6qr0000gn/T/build_75e3fc_dune/dune_cram_b748db_.t1.t/main.sh) + (cd $DIR && $SUBTEST.sh) + $SUBTEST.sh: line 1: `foo-bar': not a valid identifier -> required by alias runtest - [1] From d3f2fbd52de6b850ead5e78791e4d00ffeece8d7 Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Sat, 9 Oct 2021 16:43:21 -0600 Subject: [PATCH 3/5] test(cram): reproduce cram test exitting early Signed-off-by: Rudi Grinberg --- test/blackbox-tests/test-cases/gh4230.t | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/blackbox-tests/test-cases/gh4230.t b/test/blackbox-tests/test-cases/gh4230.t index 9558dd69897..f7f8db74b56 100644 --- a/test/blackbox-tests/test-cases/gh4230.t +++ b/test/blackbox-tests/test-cases/gh4230.t @@ -8,8 +8,18 @@ Syntax error inside a cram command > $ foo-bar() { true; } > EOF - $ dune runtest --auto-promote 2>&1 | sed -E -e 's/.+\.sh:/$SUBTEST.sh:/' -e 's/cd.*\&\&.*.sh/cd $DIR \&\& $SUBTEST.sh/' + $ cd_sed='s/cd.*\&\&.*.sh/cd $DIR \&\& $SUBTEST.sh/' + $ dune runtest --auto-promote 2>&1 | sed -E -e 's/.+\.sh:/$SUBTEST.sh:/' -e "$cd_sed" sh (internal) (exit 2) (cd $DIR && $SUBTEST.sh) $SUBTEST.sh: line 1: `foo-bar': not a valid identifier -> required by alias runtest + + $ cat >t1.t < $ exit 1 + > $ echo foobar + > EOF + $ dune runtest --auto-promote 2>&1 | sed -E -e "$cd_sed" + sh (internal) (exit 1) + (cd $DIR && $SUBTEST.sh) + -> required by alias runtest From d32889d32d544447093b76747d2aa40efb3bbffb Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Sat, 9 Oct 2021 17:23:41 -0600 Subject: [PATCH 4/5] fix(cram): handle terminating scripts better say that commands are unreachable instead of dropping everything on the floor. Signed-off-by: Rudi Grinberg --- src/dune_rules/cram_exec.ml | 55 ++++++++++++++++--------- test/blackbox-tests/test-cases/gh4230.t | 24 +++++++---- 2 files changed, 50 insertions(+), 29 deletions(-) diff --git a/src/dune_rules/cram_exec.ml b/src/dune_rules/cram_exec.ml index 5b1d69d952b..31de067482c 100644 --- a/src/dune_rules/cram_exec.ml +++ b/src/dune_rules/cram_exec.ml @@ -185,7 +185,11 @@ type metadata_entry = ; build_path_prefix_map : string } -type full_block_result = block_result * metadata_entry +type metadata_result = + | Present of metadata_entry + | Missing_unreachable + +type full_block_result = block_result * metadata_result type sh_script = { script : Path.t @@ -198,7 +202,12 @@ let read_exit_codes_and_prefix_maps file = let s = match file with | None -> "" - | Some file -> Io.read_file ~binary:true file + | Some file -> ( + try Io.read_file ~binary:true file with + | Sys_error _ -> + (* a script where the first command immediately exits might not produce + the metadata file *) + "") in let rec loop acc = function | exit_code :: build_path_prefix_map :: entries -> @@ -231,9 +240,11 @@ let read_and_attach_exit_codes (sh_script : sh_script) : | (Cram_lexer.Comment _ as comment) :: blocks, _ -> loop (comment :: acc) entries blocks | Command block_result :: blocks, metadata_entry :: entries -> - loop (Command (block_result, metadata_entry) :: acc) entries blocks - | Cram_lexer.Command _ :: _, [] -> - Code_error.raise "command without metadata" [] + loop + (Command (block_result, Present metadata_entry) :: acc) + entries blocks + | Cram_lexer.Command block_result :: blocks, [] -> + loop (Command (block_result, Missing_unreachable) :: acc) entries blocks | [], _ :: _ -> Code_error.raise "more blocks than metadata" [] in loop [] metadata_entries sh_script.cram_to_output @@ -276,19 +287,21 @@ let rewrite_paths build_path_prefix_map ~parent_script ~command_script s = |> Re.replace_string error_msg ~by:"" let sanitize ~parent_script cram_to_output : - (block_result * metadata_entry * string) Cram_lexer.block list = + (block_result * metadata_result * string) Cram_lexer.block list = List.map cram_to_output ~f:(fun (t : (block_result * _) Cram_lexer.block) -> match t with | Cram_lexer.Comment t -> Cram_lexer.Comment t - | Command - (block_result, ({ build_path_prefix_map; exit_code = _ } as entry)) -> + | Command (block_result, metadata) -> let output = - Io.read_file ~binary:false block_result.output_file - |> Ansi_color.strip - |> rewrite_paths ~parent_script ~command_script:block_result.script - build_path_prefix_map + match metadata with + | Missing_unreachable -> "***** UNREACHABLE *****" + | Present { build_path_prefix_map; exit_code = _ } -> + Io.read_file ~binary:false block_result.output_file + |> Ansi_color.strip + |> rewrite_paths ~parent_script ~command_script:block_result.script + build_path_prefix_map in - Command (block_result, entry, output)) + Command (block_result, metadata, output)) (* Compose user written cram stanzas to output *) let compose_cram_output (cram_to_output : _ Cram_lexer.block list) = @@ -304,10 +317,8 @@ let compose_cram_output (cram_to_output : _ Cram_lexer.block list) = List.iter cram_to_output ~f:(fun block -> match (block : _ Cram_lexer.block) with | Comment lines -> List.iter lines ~f:add_line - | Command - ( { command; output_file = _; script = _ } - , { exit_code; build_path_prefix_map = _ } - , output ) -> ( + | Command ({ command; output_file = _; script = _ }, metadata, output) + -> ( List.iteri command ~f:(fun i line -> let line = sprintf "%c %s" @@ -320,9 +331,12 @@ let compose_cram_output (cram_to_output : _ Cram_lexer.block list) = add_line_prefixed_with_two_space line); String.split_lines output |> List.iter ~f:add_line_prefixed_with_two_space; - match exit_code with - | 0 -> () - | n -> add_line_prefixed_with_two_space (sprintf "[%d]" n))); + match metadata with + | Missing_unreachable + | Present { exit_code = 0; build_path_prefix_map = _ } -> + () + | Present { exit_code; build_path_prefix_map = _ } -> + add_line_prefixed_with_two_space (sprintf "[%d]" exit_code))); Buffer.contents buf let create_sh_script cram_stanzas ~temp_dir : sh_script Fiber.t = @@ -367,6 +381,7 @@ let create_sh_script cram_stanzas ~temp_dir : sh_script Fiber.t = ; script = user_shell_code_file } in + fprln oc "trap 'exit 0' EXIT"; let+ cram_to_output = Fiber.sequential_map ~f:loop cram_stanzas in close_out oc; let command_count = !i in diff --git a/test/blackbox-tests/test-cases/gh4230.t b/test/blackbox-tests/test-cases/gh4230.t index f7f8db74b56..f88df11e6b3 100644 --- a/test/blackbox-tests/test-cases/gh4230.t +++ b/test/blackbox-tests/test-cases/gh4230.t @@ -8,18 +8,24 @@ Syntax error inside a cram command > $ foo-bar() { true; } > EOF - $ cd_sed='s/cd.*\&\&.*.sh/cd $DIR \&\& $SUBTEST.sh/' - $ dune runtest --auto-promote 2>&1 | sed -E -e 's/.+\.sh:/$SUBTEST.sh:/' -e "$cd_sed" - sh (internal) (exit 2) - (cd $DIR && $SUBTEST.sh) + $ dune runtest --auto-promote 2>&1 | sed -E -e 's/.+\.sh:/$SUBTEST.sh:/' -e 's/cd.*\&\&.*.sh/cd $DIR \&\& $SUBTEST.sh/' + sh (internal) $SUBTEST.sh: line 1: `foo-bar': not a valid identifier - -> required by alias runtest + File "t1.t", line 1, characters 0-0: + Error: Files _build/default/t1.t and _build/default/t1.t.corrected differ. + Promoting _build/default/t1.t.corrected to t1.t. $ cat >t1.t < $ exit 1 > $ echo foobar > EOF - $ dune runtest --auto-promote 2>&1 | sed -E -e "$cd_sed" - sh (internal) (exit 1) - (cd $DIR && $SUBTEST.sh) - -> required by alias runtest + $ dune runtest --auto-promote + File "t1.t", line 1, characters 0-0: + Error: Files _build/default/t1.t and _build/default/t1.t.corrected differ. + Promoting _build/default/t1.t.corrected to t1.t. + [1] + $ cat t1.t + $ exit 1 + ***** UNREACHABLE ***** + $ echo foobar + ***** UNREACHABLE ***** From eb18ef8e500095f7995b05328c5f16e2ba922217 Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Sat, 9 Oct 2021 17:25:39 -0600 Subject: [PATCH 5/5] Update CHANGES Signed-off-by: Rudi Grinberg --- CHANGES.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 898c425b53f..308e48da4da 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,9 @@ Unreleased - Allow spaces in cram test paths (#4980, fixes #4162, @rgrinberg) +- Improve error handling of misbehaving cram scripts. (#4981, fix #4230, + @rgrinberg) + - Fix `foreign_stubs` inside a `tests` stanza. Previously, dune would crash when this field was present (#4942, fix #4946, @rgrinberg)