Skip to content

Commit ed09871

Browse files
mbarbinmbarbin
authored
Merge pull request #19 from mbarbin/improve-ci
Improve ci failures
2 parents 330a882 + 6f25aa9 commit ed09871

File tree

7 files changed

+125
-68
lines changed

7 files changed

+125
-68
lines changed

‎CHANGES.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@
1414

1515
### Fixed
1616

17+
- Disable failing build with 5.4 alpha release (#19, @mbarbin).
18+
- Adapt grep implementation for portability to MacOS (#19, @mbarbin).
19+
1720
### Removed
1821

1922
- No longer depend on `re2` (#17, @mbarbin).

‎crs.opam

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ doc: "https://mbarbin.github.io/crs/"
99
bug-reports: "https://github.com/mbarbin/crs/issues"
1010
depends: [
1111
"dune" {>= "3.17"}
12-
"ocaml" {>= "5.2"}
12+
"ocaml" {>= "5.2" & < "5.4~"}
1313
"base" {>= "v0.17"}
1414
"cmdlang" {>= "0.0.9"}
1515
"file-rewriter" {>= "0.0.3"}

‎dune-project

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@
2525
"A tool for managing code review comments embedded in source code")
2626
(depends
2727
(ocaml
28-
(>= 5.2))
28+
(and
29+
(>= 5.2)
30+
(< 5.4~)))
2931
(base
3032
(>= v0.17))
3133
(cmdlang

‎lib/crs_cli/src/cmd__grep.ml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ let main =
6161
| None -> Vcs.Path_in_repo.root
6262
| Some path -> Common_helpers.relativize ~repo_root ~cwd ~path
6363
in
64+
let () = Stdlib.Sys.set_signal Stdlib.Sys.sigpipe Stdlib.Sys.Signal_ignore in
6465
let crs = Crs_parser.grep ~vcs ~repo_root ~below in
6566
Git_pager.run ~f:(fun git_pager ->
6667
let oc = Git_pager.write_end git_pager in

‎lib/crs_parser/src/crs_parser.ml

Lines changed: 91 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -90,73 +90,100 @@ module Exit_status = struct
9090
[@@deriving sexp_of]
9191
end
9292

93+
let null_separator = String.make 1 (Char.of_int_exn 0)
94+
9395
let grep ~vcs ~repo_root ~below =
94-
let files_to_grep = Vcs.ls_files vcs ~repo_root ~below in
95-
let stdin_text =
96-
files_to_grep |> List.map ~f:Vcs.Path_in_repo.to_string |> String.concat ~sep:"\n"
97-
in
9896
let files_to_grep =
99-
match
100-
let prog =
101-
match Lazy.force find_xargs with
102-
| Some prog -> prog
103-
| None -> failwith "Cannot find xargs in PATH" [@coverage off]
104-
in
105-
let stdin_reader, stdin_writer = Spawn.safe_pipe () in
106-
let stdout_reader, stdout_writer = Spawn.safe_pipe () in
107-
let stderr_reader, stderr_writer = Spawn.safe_pipe () in
108-
let pid =
109-
Spawn.spawn
110-
~cwd:(Path (Vcs.Repo_root.to_string repo_root))
111-
~prog
112-
~argv:
113-
[ "xargs"
114-
; "-r"
115-
; "-d"
116-
; "\n"
117-
; "grep"
118-
; "--no-messages"
119-
; "-E"
120-
; "-l"
121-
; "--binary-files=without-match"
122-
; cr_pattern_egrep
123-
]
124-
~stdin:stdin_reader
125-
~stdout:stdout_writer
126-
~stderr:stderr_writer
127-
()
128-
in
129-
Unix.close stdin_reader;
130-
Unix.close stdout_writer;
131-
Unix.close stderr_writer;
132-
let () =
133-
let stdin_oc = Unix.out_channel_of_descr stdin_writer in
134-
Out_channel.output_string stdin_oc stdin_text;
135-
Out_channel.flush stdin_oc;
136-
Unix.close stdin_writer
97+
match Vcs.ls_files vcs ~repo_root ~below with
98+
| [] -> []
99+
| _ :: _ as files_to_grep ->
100+
let stdin_text =
101+
files_to_grep
102+
|> List.map ~f:Vcs.Path_in_repo.to_string
103+
|> String.concat ~sep:null_separator
137104
in
138-
let stdout = read_all_from_fd stdout_reader in
139-
let (_ : string) = read_all_from_fd stderr_reader in
140-
let pid', process_status = waitpid_non_intr pid in
141-
assert (pid = pid');
142-
match process_status with
143-
| Unix.WEXITED (0 | 123) -> `Output stdout
144-
| Unix.WEXITED n -> `Exit_status (`Exited n)
145-
| Unix.WSIGNALED n -> `Exit_status (`Signaled n) [@coverage off]
146-
| Unix.WSTOPPED n -> `Exit_status (`Stopped n) [@coverage off]
147-
with
148-
| `Output stdout -> stdout |> String.split_lines |> List.map ~f:Vcs.Path_in_repo.v
149-
| `Exit_status exit_status ->
150-
raise
151-
(Err.E
152-
(Err.create
153-
[ Pp.text "Process xargs exited abnormally."
154-
; Err.sexp [%sexp { exit_status : Exit_status.t }]
155-
]))
156-
| exception exn ->
157-
raise
158-
(Err.E (Err.create [ Pp.text "Error while running xargs process."; Err.exn exn ]))
159-
[@coverage off]
105+
let stdout_ref = ref "<Unknown>" in
106+
let stderr_ref = ref "<Unknown>" in
107+
(match
108+
let prog =
109+
match Lazy.force find_xargs with
110+
| Some prog -> prog
111+
| None -> failwith "Cannot find xargs in PATH" [@coverage off]
112+
in
113+
let stdin_reader, stdin_writer = Spawn.safe_pipe () in
114+
let stdout_reader, stdout_writer = Spawn.safe_pipe () in
115+
let stderr_reader, stderr_writer = Spawn.safe_pipe () in
116+
let pid =
117+
Spawn.spawn
118+
~cwd:(Path (Vcs.Repo_root.to_string repo_root))
119+
~prog
120+
~argv:
121+
[ "xargs"
122+
; "-0"
123+
; "grep"
124+
; "--no-messages"
125+
; "-E"
126+
; "-l"
127+
; "--binary-files=without-match"
128+
; cr_pattern_egrep
129+
]
130+
~stdin:stdin_reader
131+
~stdout:stdout_writer
132+
~stderr:stderr_writer
133+
()
134+
in
135+
Unix.close stdin_reader;
136+
Unix.close stdout_writer;
137+
Unix.close stderr_writer;
138+
let () =
139+
let stdin_oc = Unix.out_channel_of_descr stdin_writer in
140+
Out_channel.output_string stdin_oc stdin_text;
141+
Out_channel.flush stdin_oc;
142+
Unix.close stdin_writer
143+
in
144+
let stdout = read_all_from_fd stdout_reader in
145+
stdout_ref := stdout;
146+
let stderr = read_all_from_fd stderr_reader in
147+
stderr_ref := stderr;
148+
let pid', process_status = waitpid_non_intr pid in
149+
assert (pid = pid');
150+
match process_status with
151+
| Unix.WEXITED n ->
152+
(* The exit code of [xargs] is not consistent on all of the platforms
153+
that we'd like to support. While it always returns [0] in case of
154+
a match, when the inner [grep] doesn't find a match and returns
155+
[1], the outer call to [xargs] may return [1] or [123] depending
156+
on things like the OS. *)
157+
if Int.equal n 0
158+
then (
159+
let files = stdout |> String.split_lines |> List.map ~f:Vcs.Path_in_repo.v in
160+
`Files files)
161+
else if
162+
(Int.equal n 123 || (Int.equal n 1 [@coverage off] (* On MacOS *)))
163+
&& String.is_empty stdout
164+
&& String.is_empty stderr
165+
then `Files []
166+
else `Error (`Exited n)
167+
| Unix.WSIGNALED n -> `Error (`Signaled n) [@coverage off]
168+
| Unix.WSTOPPED n -> `Error (`Stopped n) [@coverage off]
169+
with
170+
| `Files files -> files
171+
| `Error exit_status ->
172+
let stdout = !stdout_ref in
173+
let stderr = !stderr_ref in
174+
raise
175+
(Err.E
176+
(Err.create
177+
[ Pp.text "Process xargs exited abnormally."
178+
; Err.sexp
179+
[%sexp
180+
{ exit_status : Exit_status.t; stdout : string; stderr : string }]
181+
]))
182+
| exception exn ->
183+
raise
184+
(Err.E
185+
(Err.create [ Pp.text "Error while running xargs process."; Err.exn exn ]))
186+
[@coverage off])
160187
in
161188
List.concat_map files_to_grep ~f:(fun path_in_repo ->
162189
let file_contents =

‎test/cram/grep.t

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ file, we make use of some trick.
1010
$ export CR="CR"
1111
$ export XCR="XCR"
1212

13+
We disable the pager in this test.
14+
15+
$ export GIT_PAGER=cat
16+
1317
Let's add some files to the tree.
1418

1519
$ cat > hello << EOF
@@ -146,6 +150,24 @@ You may restrict the search to a subdirectory only.
146150
Error: Path "/tmp" is not in repo.
147151
[123]
148152

153+
$ crs grep --below not-a-directory
154+
Context:
155+
(Vcs.ls_files
156+
(repo_root
157+
$TESTCASE_ROOT)
158+
(below not-a-directory))
159+
((prog git) (args (ls-files --full-name)) (exit_status Unknown)
160+
(cwd
161+
$TESTCASE_ROOT/not-a-directory)
162+
(stdout "") (stderr ""))
163+
Error:
164+
(Unix.Unix_error "No such file or directory" chdir
165+
$TESTCASE_ROOT/not-a-directory)
166+
[123]
167+
168+
$ mkdir empty-directory
169+
$ crs grep --below empty-directory
170+
149171
There's also an option to display the results as summary tables.
150172

151173
$ crs grep --summary
@@ -179,12 +201,14 @@ matching. This involves running [xargs]. Let's cover for some failures there.
179201

180202
$ cat > xargs <<EOF
181203
> #!/bin/bash -e
204+
> # Read and discard all stdin to avoid broken pipe
205+
> cat > /dev/null
182206
> echo "Hello Fake xargs"
183207
> exit 42
184208
> EOF
185209
$ chmod +x ./xargs
186210

187211
$ PATH=".:$PATH" crs grep
188212
Error: Process xargs exited abnormally.
189-
(exit_status (Exited 42))
213+
((exit_status (Exited 42)) (stdout "Hello Fake xargs\n") (stderr ""))
190214
[123]

‎test/cram/hg.t

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
First we need to setup a repo in a way that satisfies the test environment. This
22
includes specifics required by the GitHub Actions environment.
33

4-
$ hg init
4+
$ hg init 2> /dev/null
55

66
To make sure the CRs are not mistaken for actual cr comments in this
77
file, we make use of some trick.

0 commit comments

Comments
 (0)