Add "{open,close}-process" predicate

[phikal]

As mentioned on Zulip, this is a general predicate that I find it would be nice to have when processing information from external commands. The command differs from system, in that is not interpreted as a shell command (that comes with all kind of quoting complications), but just a list of argv-style arguments that are not processed further. As I said, my OCaml experience is limited, so if you are even interested in accepting a patch like this, I am open to any and all criticism.

[phikal]

And as context, we are using this to generate an ELPI expression and then to read it in as a term:

system_output  ["echo", "1   + 2"] In, readterm In T.

Oh, and if you have a better idea of what to call the predicate, feel free to suggest anything.

[gares]

Thanks for proposing this change. I took the freedom to push some changes to your branch. The test file should explain how the API works. Please tell me if it is a good fit for your use case.

[phikal]

As this is more general, it should be fine. Do you think it makes sense to have a type for processes, so you don't need to match all the arguments individually when creating and closing a process?

[gares]

If I had to design an API myself I would totally agree, but this is just a translation of the Ocaml one... Anyway, I broke ci so I must have done something silly

[phikal]

If I had to design an API myself I would totally agree, but this is just a translation of the Ocaml one...

What I had in mind was a kind of thin wrapper over this "raw" API, something like

kind process type.
type process in_stream -> out_stream -> in_stream -> process.

type open-process i:list string, i:list string, o:process, o:diagnostic.
open-process Args Env (process Out In Err) Diag :-
  Argv = [Cmd | _],
  unix.open-process Cmd Argv Env Out In Err Diag.

type close-process i:process, o:diagnostic.
close-process (process Out In Err) Diag :-
  unix.close-process Out In Err Diag.

Also, I would recommend swapping Out and In to preserve the canonical ordering of the file descriptors from Unix (0 is stdin, 1 is stdout, 2 is stderr).

Anyway, I broke ci so I must have done something silly

All the jobs appear to fail with the same issue (a counter is off by ~2), but the new test is not executed. Perhaps this was a temporary CI error, could you restart the tests to make sure?

[phikal]

I have managed to resolve the issues that were introduced in the "unix" testcases, now make tests only indicates that this is broken:

KO       trace-browser-chr (trace generation)        elpi

It appears that this issue already arose with 6ec682b. The CI says that the output differs like this

-{"step" : 0,"kind" : ["Info"],"goal_id" : 0,"runtime_id" : 2,"name" : "user:assign","payload" : ["A0 := uvar frozen--480 []"]}
+{"step" : 0,"kind" : ["Info"],"goal_id" : 0,"runtime_id" : 2,"name" : "user:assign","payload" : ["A0 := uvar frozen--485 []"]}

Can the order of built-in predicates change the order in which fresh global constants are generated? If so, that sounds like a mistake in the tests, not in the changes proposed here.

[gares]

Yes, the numbers of frozen variables depend on the number of statically allocated symbols. It is a bad test.

[gares]

I confirm that the test does not work on 4.09, while works fine on 4.14. Apparently echo "foo" does generate an empty output.

[phikal]

Enrico Tassi @.***> writes:

I confirm that the test does not work on 4.09, while works fine on 4.14.

What versions are you referring to?

[phikal]

----<<---- enter:  works0
Fatal error: bad output : 
 = foo

Do we have a race condition here? Or does input_line block until there is an available line?

Nevermind, I tested it by replacing echo with this script:

#!/bin/sh
sleep 10
echo "$@"

and it did not cause any issues locally.

[gares]

There should be no race. I suspect that "echo" (not being an actual command) did not work well with the old implementation of the unix API (changed in 4.13 if I read the changelog right)

[gares]

Anyway, I'm fine merging the current code. But we need a way to tolerate the failure in the test suite. And we should document that the API is not OK on old ocaml versions.

Using a shell script instead of just echo may be a good way to fix the test suite.

[phikal]

I suspect that "echo" (not being an actual command)

Do you mean that echo is just provided as a built-in shell command, but there is no actual executable on some of the CI systems?

Using a shell script instead of just echo may be a good way to fix the test suite.

How about replacing the echo command with sh -c "echo foo" then?

[gares]

I suspect that "echo" (not being an actual command)

Do you mean that echo is just provided as a built-in shell command, but there is no actual executable on some of the CI systems?

Using a shell script instead of just echo may be a good way to fix the test suite.

How about replacing the echo command with sh -c "echo foo" then?

Yes, thanks for committing that. Let's see if CI is happy.

[gares]

Apparently the API is really broken in old ocaml versions :-/ I'll try to investigate better tomorrow

[gares]

let () =
  let o,i,e = Unix.open_process_args_full "sh" [|"sh";"-c";"echo xxx"|] (Unix.environment()) in 
  let l = input_line o in
  assert(l = "xxx");
  exit 0

fails on 4.09.1 while works fine on 4.14.0, so it is definitely an OCaml's bug, now fixed.

While

let () =
  let o,i,e = Unix.open_process_args_full "./a.sh" [|"./a.sh"|] (Unix.environment()) in 
  let l = input_line o in
  assert(l = "xxx");
  exit 0

with a.sh being

#!/bin/sh
echo xxx

works. So we have a fix for CI

[gares]

Still broken, on windows:

----<<---- enter:  works0
Fatal error: works0 : 
create_process tests/sources/echo.sh: Exec format error

Maybe we could use printf (which is not a builtin).

On Linux and MacOS:

----<<---- enter:  works0
---->>---- exit:  works0
----<<---- enter:  works1
---->>---- fail:  works1

Did not investigate, see the blind try above.

[gares]

I think we can just sneak a test on the ocaml version in tests/suite/ so that the test only run on >= 4.12

[phikal]

Maybe we could use printf (which is not a builtin).

echo is usually also provided as an actual executable, but I guess it is worth a try :/

I think we can just sneak a test on the ocaml version in tests/suite/ so that the test only run on >= 4.12

I won't object, but I don't know how to do that either ^^

[gares]

Many thanks @phikal , I think this PR can be merged as is. Further improvements can go to a separate PR, if you need them/

[phikal]

That should be fine for now, thank you for taking the time to fix the CI-related issues.

Out of curiosity, when do you expect to release a new version of ELPI, that would include the new predicates?

[gares]

I can make a point release even now, but I'd like all new stuff to be in a namespace so not to cause any incompatibility.

The data type should be unix.process The api unix.process.open/close. The low level APIs maybe should disappear. It is not hard to describe the process as an OCaml adt, see the pair, option, diagnostic ones in builtins.ml

[phikal]

I can make a point release even now, but I'd like all new stuff to be in a namespace so not to cause any incompatibility.

If it were possible to do so some time soon, I would appreciate it very much.

[gares]

I did release this change in September, didn't I?

[phikal]

Oh, I totally missed that, sorry!