extending uiop/run-program

[Faré]

On Wed, Aug 10, 2016 at 5:53 AM, Elias Pipping <pipping.elias at icloud.com> wrote:
>
>> On 01 Aug 2016, at 04:25, Robert Goldman <rpgoldman at sift.net> wrote:
>>
>> Elias,
>>
>> If you were to move your work on uiop:run-program to a long-lived topic
>> branch, I could test it on a wide variety of implementations on Mac and
>> Linux.
>
> Dear Robert, dear Faré, dear list,
>
> an update might be in order. Currently, the run-program branch, available from
>
>   (dynamic) https://gitlab.common-lisp.net/epipping/asdf/commits/run-program
>   (static) https://gitlab.common-lisp.net/epipping/asdf/commits/a45e91e81d2e973d1541adbe37e81c242ce98c66
>
> has 7 commits over the master branch, each of which should be uncontroversial and either fix a bug or do other kinds of non-invasive cleanup.
>
Nits (to be addressed in follow up commits, or by modifying these):

1- In c2c130bc4ff4a56ca6fec5e46acf5f32d0272909, you break the
asciibetical order of #+'s. Also, you might check what does or doesn't
work in CLASP.

2- In cee1754b6f82ba791495122c6229213a42b0bcbb
I prefer a white list of platforms where the combination is known to
work, rather than a blacklist of platforms where it is known not to
work. That makes for more understandable failures on new platforms.

3- In 46009053aee3ab4c757dc57e880b5eabe61d932c it would be nice to
know since when this interface function is public in CCL, so we know
how old a CCL we are or aren't supporting. Hopefully, we keep
supporting a CCL release one or two year old.


> Then there’s the run-program-messy-with-rebasing branch
>
> (dynamic) https://gitlab.common-lisp.net/epipping/asdf/commits/run-program-messy-with-rebasing
> (static) https://gitlab.common-lisp.net/epipping/asdf/commits/61654b62b2d8cf6265ef968919fe35e9673e0c61
>
> which has additional commits that are more controversial. I’m writing this email mostly because of these controversial commits because I know too little about good design of CL libraries and would like to invite feedback.
>
> I’d like to ask the following questions:
>
> Issue #1:
>
> For CL platforms other than Allegro CL, you can inquire about the exit code of a program repeatedly once it has exited (through %wait-process-result). For Allegro CL, this is not the case: The exit-code will only be returned once. Since the plist that currently makes up a process can store the exit code, that need not be a problem: The first time it is obtained, it could be stored, and repeated calls to %wait-process-result would just hand out the value that’s been stored earlier. To keep the user from having to write code like
>
>   (setf process-info (%wait-process-result process-info))
>
> I used
>
>   (nconc process-info (list :exit-code exit-code))
>
> in https://gitlab.common-lisp.net/epipping/asdf/commit/ac2d610d77806896bc0f1543bcb8f5076b3b21c9. That might be surprising to the user and go against relevant style guides (it’s safe in %wait-process-result in the sense that the entire body is guarded by a non-null-check for :process so that in particular, process-info isn’t NIL). So maybe this should be fixed in a different manner (please note that the issue is not easily settled by documenting that %wait-process-result should only be called once: A function that queries the process status without waiting would call the same function as %wait-process-result for Allegro CL).
>
Please do NOT use nconc. I wrote at length against NCONC, that I'd
like to see deprecated:
http://cliki.net/Proposed%20ANSI%20Revisions%20and%20Clarifications

Please use (setf (getf process-info :exit-code) exit-code) and please
only use it on allegro.

If you need to side-effect a data structure preserved across function
calls, then maybe it's time to use a defstruct (or a defclass?) rather
than a plist. If you can't be bothered to refactor, pre-reserved a
plist slot to setf getf will do the job.

> Issue #2:
>
> https://gitlab.common-lisp.net/epipping/asdf/commit/1680d3e36a54717b195f672b6996fcae08d54218 and https://gitlab.common-lisp.net/epipping/asdf/commit/4b884915c4d62f07cec53e9d130ab3b99c6bb6e4 add a pair of functions, each.
>
> The former adds %terminate-process (meant to be public) and %posix-process-send-signal (not necessarily public).
> The latter adds %process-alive-p (meant to be public) and %process-status-result (not necessarily public).
>
> Are these names and interfaces acceptable? (Now that I’m writing this, I already feel that %process-status-result should probably rather be named %process-status). It also uses nconc in the same way except without the non-null check (this shall be fixed).
>
Yes, the names are acceptable. If you are going to support them as
stable interfaces, you can remove the % and export the symbols. Be
sure to have adequate docstrings, then.

Please don't use nconc. Ever.

> Issue #3:
>
> https://gitlab.common-lisp.net/epipping/asdf/commit/308faabe755d718f7fb8f2723d3615a64d50c44f adds getters for the streams contained in a process-info plist. Are these acceptable?
>
No opinion.

> Issue #4:
>
> %run-program uses asserts in quite a few places. These guard against combinations of parameters that are not supported by certain platforms. They do not provide restarts or helpful diagnostics, however. So I’m wondering if they should be replaced by different failure signalling approaches. Or if they should be removed if the underlying platform provides sufficient error handling.
>
The asserts were fine in an internal function. If the function is
exposed and supported, they should be error.

Do NOT put uiop tests in uiop/ -- put them in test/ -- that said we
should probably distinguish uiop and asdf tests. Add a uiop prefix to
the test names.

> Issue #5:
>
> To extend the failure signalling approaches mentioned above to additional cases, I’ve found it necessary to check if a stream has a file handle.
>
>   https://gitlab.common-lisp.net/epipping/asdf/commit/4628f3fb57384c8d1d62aeacb244eab9e8f978c7
>
> thus adds
>
>    (defun file-stream-p (stream) …)
>    (defun file-stream-or-synonym-p (stream) …)
>
> as exported functions to uiop/stream. Are these names and interfaces acceptable?
>
I would say file-or-synonym-stream-p

> PS: Let me know if you prefer merge requests to such emails.
You did a perfect job. Either is fine, though at least until confirmed
that merge requests send mail, please notify the list with mail when
you issue a merge request.

> PPS: I’m currently testing with the following platforms:
>
> acl-10.0-linux-x86 (+the latest updates)
> ccl-1.11-f96-linux-x64
> clisp-2.49+-unix-x64
> cmu-21a__21a_unicode_-linux-x86
> ecl-16.1.2-af65969c-linux-x64
> lw-7.0.0-linux-x64
> mkcl-1.1.10-linux-x64 (git revision c69d5fc907409803fab184ac56bb42041d42c5e4)
> sbcl-1.3.8-linux-x64

Thanks a lot for a great job!

PS: Are you interested in becoming new maintainer for UIOP?

—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org
If being against something is a phobia, then being for is mania.
Peace and understanding through slurs of mental illness.
Homomania, islamomania, etc.