Add a tests for long commands #344
Conversation
|
On windows, the limit seems to be ~8185 which seems very close to the 8192 character limit imposed by CMD.EXE. Yet, we're not longer supposed to go through cmd.exe. |
hhugo
force-pushed
the
longcmd
branch
2 times, most recently
from
9c2fbc6 to
e67e55c
Compare
|
With this PR, I believe we now have all important piece of code of fdopen's patches. The maintenance of ocamlbuild can return to its previous state :) |
To be more explicit, there are still things present in fdopen patches and absent from upstream but I don't think they are necessary.
|
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
prepare_command_for_windows is a bit of a spaghetti soup now, but I don't see an obvious way to avoid this. (We can use Fun.protect but annoyingly none of the cleanup logic fits its restricted expressivity.) Oh well.
src/my_std.ml
Outdated
| || string_exists (function ' ' | '\"'| '\t' -> true | _ -> false) f | ||
| then Filename.quote f | ||
| else f | ||
| in |
There was a problem hiding this comment.
Could we just compute the size of Filename.quote f? This over-approximates but probably not by a lot, and it makes the code simpler.
| | Unix.WEXITED _ | Unix.WSIGNALED _ | Unix.WSTOPPED _ -> | ||
| failwith (Printf.sprintf "Error while running: %s" s) in | ||
| Option.iter (fun f -> f ()) cleanup; | ||
| failwith (Printf.sprintf "Error while running: %s" s) in |
There was a problem hiding this comment.
I wonder if we would benefit from splitting the two paths more.
Untested:
let close ic = ... in
if not Sys.win32 then begin
let ic = Unix.open_process_in s in
Fun.protect ~finally:(fun () -> close ic) @@ fun () ->
kont ic
else
let args, cleanup = My_std.prepare_command_for_windows s in
let ic = Unix.open_process_args_in args.(0) args in
Fun.protect ~finally:cleanup @@ fun () ->
Fun.protect ~finally:(fun () -> close ic) @@ fun () ->
kont icThere was a problem hiding this comment.
(Or whatever style other than @@ fun () -> that you prefer or find more consistent with the codebase.)
There was a problem hiding this comment.
This patch makes the CI complain because it changes the exception raise:
- previously:
Failure ... - with this patch:
Finally_raised ..
I'll revert that part
Add a test that we can handle commands bigger that 8k characters.
Fix the windows implementation that previously had a 8K limit.