On Wed, Aug 10, 2016 at 5:53 AM, Elias Pipping <pipping.el...@icloud.com> wrote: > >> On 01 Aug 2016, at 04:25, Robert Goldman <rpgold...@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.
