Dear list, I’ve recently made the switch from external-program to uiop/run-program::%run-program for asynchronous process execution after I read that its interface should remain essentially stable. I’ve also had to use uiop/run-program::%wait-process-result although I’m not sure if its interface in turn as subject to the same close-to-guarantee. So far the two are serving me well and I’m not looking back. I ended up writing two functions that I needed because uiop/run-program does not currently provide such functionality while external-program did. I’m sometimes interested in testing whether a process is running; I use the following function for that: (defun process-running-p (process-info) (let ((process (getf process-info :process))) #+clozure (eq :running (ccl:external-process-status process)) #+cmu (eq :running (ext:process-status process)) #+ecl (eq :running (ext:external-process-status process)) #+mkcl (eq :running (mk-ext:process-status process)) #+sbcl (eq :running (sb-ext:process-status process)) #-(or clozure cmu ecl mkcl sbcl) (error "Cannot determine if a process is running."))) Maybe something like this could be incorporated into UIOP as well? (or a function that returns the process status, or a function that checks that a process is alive, etc.) Sometimes, I also want to terminate a process before it finishes. To that end, I use the following: (alexandria:define-constant +kill-signal+ 9 :test #'=) (alexandria:define-constant +term-signal+ 15 :test #'=) (defun terminate-process (process-info &key force) (let ((process (getf process-info :process)) (sig (if force +kill-signal+ +term-signal+))) #+allegro (progn ; FIXME: untested #+os-unix (excl.osi:kill process sig) #+os-windows (uiop/run-program::%run-program (format nil "taskkill /f /pid ~a" process) :wait t) #-(or os-unix os-windows) (error "Cannot terminate a process.") (sys:reap-os-subprocess :pid process)) #+clozure (ccl:signal-external-process process sig :error-if-exited nil) #+cmu (ext:process-kill process sig) #+sbcl (sb-ext:process-kill process sig) #+scl (ext:process-kill process sig) ; FIXME: untested #+mkcl (mk-ext:terminate-process process :force force) #-(or allegro clozure cmu mkcl sbcl) (error "Cannot terminate a process."))) This is a special case of a more general signalling facility that many lisps have but which from my understanding can only work on unix. mkcl does not provide such a facility but it does provide a terminate-process function that works on windows, too. The Allegro CL documentation states that one should run (format nil "taskkill /f /pid ~a" process) on such platforms, which could also be done with other lisps, assuming that the PID is known or can be extracted from the process. Maybe a function of this type could be incorporated into UIOP as well? Elias Pipping
Dear Elias, this kind of functionality is welcome in UIOP. However, the responsibility is yours to ensure that it will work on every implementation on every platform. Please make reasonably sure it works before you submit the patch. You may contact various vendors for a test license as appropriate. Note that there are several run-program style libraries around. If you're going to do this, I suggest you look at each and every of these libraries (notably executor and external-program) and make sure you have at least feature-parity and implementation-parity with them. Also make sure that you explicitly raise an error on unsupported implementations, at the start of every function that doesn't support them. Also, if you are going to support these interface, you can graduate them out of % namespace (but keep the % name around for backward compatibility for a year or two). Would you be interested in becoming official maintainer for UIOP? —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org The trouble with opportunity is that it always comes disguised as hard work. — Herbert V. Prochnow On Sun, Jul 31, 2016 at 1:57 PM, Elias Pipping <pipping.elias@icloud.com> wrote:
Dear list,
I’ve recently made the switch from external-program to uiop/run-program::%run-program for asynchronous process execution after I read that its interface should remain essentially stable. I’ve also had to use uiop/run-program::%wait-process-result although I’m not sure if its interface in turn as subject to the same close-to-guarantee. So far the two are serving me well and I’m not looking back.
I ended up writing two functions that I needed because uiop/run-program does not currently provide such functionality while external-program did.
I’m sometimes interested in testing whether a process is running; I use the following function for that:
(defun process-running-p (process-info) (let ((process (getf process-info :process))) #+clozure (eq :running (ccl:external-process-status process)) #+cmu (eq :running (ext:process-status process)) #+ecl (eq :running (ext:external-process-status process)) #+mkcl (eq :running (mk-ext:process-status process)) #+sbcl (eq :running (sb-ext:process-status process)) #-(or clozure cmu ecl mkcl sbcl) (error "Cannot determine if a process is running.")))
Maybe something like this could be incorporated into UIOP as well? (or a function that returns the process status, or a function that checks that a process is alive, etc.)
Sometimes, I also want to terminate a process before it finishes. To that end, I use the following:
(alexandria:define-constant +kill-signal+ 9 :test #'=) (alexandria:define-constant +term-signal+ 15 :test #'=)
(defun terminate-process (process-info &key force) (let ((process (getf process-info :process)) (sig (if force +kill-signal+ +term-signal+))) #+allegro (progn ; FIXME: untested #+os-unix (excl.osi:kill process sig) #+os-windows (uiop/run-program::%run-program (format nil "taskkill /f /pid ~a" process) :wait t) #-(or os-unix os-windows) (error "Cannot terminate a process.") (sys:reap-os-subprocess :pid process)) #+clozure (ccl:signal-external-process process sig :error-if-exited nil) #+cmu (ext:process-kill process sig) #+sbcl (sb-ext:process-kill process sig) #+scl (ext:process-kill process sig) ; FIXME: untested #+mkcl (mk-ext:terminate-process process :force force) #-(or allegro clozure cmu mkcl sbcl) (error "Cannot terminate a process.")))
This is a special case of a more general signalling facility that many lisps have but which from my understanding can only work on unix. mkcl does not provide such a facility but it does provide a terminate-process function that works on windows, too. The Allegro CL documentation states that one should run (format nil "taskkill /f /pid ~a" process) on such platforms, which could also be done with other lisps, assuming that the PID is known or can be extracted from the process. Maybe a function of this type could be incorporated into UIOP as well?
Elias Pipping
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. Currently, my Windows-testing VM is out of commission, so I need to fix it, but at that point I could probably test there, as well. I'd like to see some test cases for these additional facilities, as well. If we are going to work on RUN-PROGRAM, it occurs to me that it might be a good idea to wrap the PROCESS-INFO plist in a trivial DEFSTRCUCT that would support type-checking like (typep x 'process-info) Best, Robert
Dear Robert,
On 01 Aug 2016, at 04:25, Robert Goldman <rpgoldman@sift.net> wrote:
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.
Sure, that sounds good. I’ve requested an account on https://gitlab.common-lisp.net for this purpose.
Currently, my Windows-testing VM is out of commission, so I need to fix it, but at that point I could probably test there, as well.
Great!
I'd like to see some test cases for these additional facilities, as well.
I’m a fan of tests. I typically end up writing too many rather than too few. Currently, UIOP does not have any dedicated tests, yes? It’s only indirectly tested through the ASDF test suite as far as I can see. And I suppose even the test suite should not have any external dependencies so that everything needs to be done manually, right?
If we are going to work on RUN-PROGRAM, it occurs to me that it might be a good idea to wrap the PROCESS-INFO plist in a trivial DEFSTRCUCT that would support type-checking like
(typep x 'process-info)
Sounds good. That’s what external-program does, too (at least on some platforms). Elias
On 8/1/16 Aug 1 -5:06 PM, Elias Pipping wrote:
Dear Robert,
On 01 Aug 2016, at 04:25, Robert Goldman <rpgoldman@sift.net> wrote:
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.
Sure, that sounds good. I’ve requested an account on
https://gitlab.common-lisp.net
for this purpose.
Currently, my Windows-testing VM is out of commission, so I need to fix it, but at that point I could probably test there, as well.
Great!
My Windows box is now fixed. Sort of. But I really don't understand Windows, and have no desire to learn. I can run the tests, and check their results, but that's about the limit. I have no intention of fussing around with cmd.exe and the like... Some time ago, Faré suggested that it might be worth replacing use of cmd.exe with powershell in UIOP/run-program. I don't really know what powershell is, but I get the impression it's a shell that's less horrible than cmd.exe. Best, r
My Windows box is now fixed. Sort of. But I really don't understand Windows, and have no desire to learn. I can run the tests, and check their results, but that's about the limit. I have no intention of fussing around with cmd.exe and the like...
Some time ago, Faré suggested that it might be worth replacing use of cmd.exe with powershell in UIOP/run-program.
I don't really know what powershell is, but I get the impression it's a shell that's less horrible than cmd.exe. Powershell is an improved shell that ships with all versions of windows from 7 on, (and server 2008R2 on). It is available going as far back as XP as an optional download, so it would be an external requirement for UIOP for Vista and older.
Best, r
On 8/1/16 Aug 1 -7:06 PM, Jason Miller wrote:
My Windows box is now fixed. Sort of. But I really don't understand Windows, and have no desire to learn. I can run the tests, and check their results, but that's about the limit. I have no intention of fussing around with cmd.exe and the like...
Some time ago, Faré suggested that it might be worth replacing use of cmd.exe with powershell in UIOP/run-program.
I don't really know what powershell is, but I get the impression it's a shell that's less horrible than cmd.exe. Powershell is an improved shell that ships with all versions of windows from 7 on, (and server 2008R2 on). It is available going as far back as XP as an optional download, so it would be an external requirement for UIOP for Vista and older.
Do any Windows users out there have judgments about how onerous it would be to impose this requirement?
From what Jason says, it seems like it might be a nuisance now, but as we move forward, this should be an acceptable change.
Best, r
I considered using PowerShell indeed, but concluded against it in the end: PowerShell is better than CMD.exe for about everything. But, putting aside the fact that it's only recently universally available (1) it's also very slow to start, which would add a lot of latency to run-program, even though (2) we are not using any of the functionality of the shell, beside starting a program, which CMD does well enough, and (3) on many implementations, CMD is what is used anyway, and adding one more layer of indirection can only introduce discrepancy and bugs. What exactly do you want to use from PowerShell? If you want to specifically call PowerShell, doesn't the current interface already slow you to do it? Unless of course you mean using it for scripts that come with ASDF, but there aren't any except the shims to run asdf-tools. On Tue, Aug 2, 2016, 10:07 Robert Goldman <rpgoldman@sift.net> wrote:
On 8/1/16 Aug 1 -7:06 PM, Jason Miller wrote:
My Windows box is now fixed. Sort of. But I really don't understand Windows, and have no desire to learn. I can run the tests, and check their results, but that's about the limit. I have no intention of fussing around with cmd.exe and the like...
Some time ago, Faré suggested that it might be worth replacing use of cmd.exe with powershell in UIOP/run-program.
I don't really know what powershell is, but I get the impression it's a shell that's less horrible than cmd.exe. Powershell is an improved shell that ships with all versions of windows from 7 on, (and server 2008R2 on). It is available going as far back as XP as an optional download, so it would be an external requirement for UIOP for Vista and older.
Do any Windows users out there have judgments about how onerous it would be to impose this requirement?
From what Jason says, it seems like it might be a nuisance now, but as we move forward, this should be an acceptable change.
Best, r
On 8/2/16 Aug 2 -10:04 AM, Faré wrote:
I considered using PowerShell indeed, but concluded against it in the end: PowerShell is better than CMD.exe for about everything. But, putting aside the fact that it's only recently universally available (1) it's also very slow to start, which would add a lot of latency to run-program, even though (2) we are not using any of the functionality of the shell, beside starting a program, which CMD does well enough, and (3) on many implementations, CMD is what is used anyway, and adding one more layer of indirection can only introduce discrepancy and bugs.
What exactly do you want to use from PowerShell? If you want to specifically call PowerShell, doesn't the current interface already slow you to do it?
Unless of course you mean using it for scripts that come with ASDF, but there aren't any except the shims to run asdf-tools.
These sound like good reasons to stick with CMD.EXE. I just wondered if PS would make things easier. I'll report my bugs with respect to running on Windows in a separate message. More issues with cygwin clashing with windows executables. Best, r
On 02 Aug 2016, at 00:06, Elias Pipping <pipping.elias@icloud.com> wrote:
Dear Robert,
On 01 Aug 2016, at 04:25, Robert Goldman <rpgoldman@sift.net> wrote:
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.
Sure, that sounds good. I’ve requested an account on
https://gitlab.common-lisp.net
for this purpose.
The branch is now up at https://gitlab.common-lisp.net/epipping/asdf/tree/run-program with the first bug fix. I’ve also pushed to another branch https://gitlab.common-lisp.net/epipping/asdf/tree/run-program-messy-with-reb... the name of which is supposed to convey how messy it could potentially be (and how I might `push —force` to it). I’ve added a test script (to the messy branch only) that’s not currently connected in any way to the existing testing infrastructure. It serves me as a survey tool, since a test failure at this point can mean either a problem with run-program or with my understanding of it (I didn’t know about ccl’s (with-open-file (… :sharing :lock)) until yesterday e.g. and there’s an odd issue with string streams under cmucl that I wrote to cmucl-help to about because I’m not sure if cmucl is doing something wrong or I am). The output currently looks like this for me: https://gitlab.common-lisp.net/epipping/asdf/commit/3e92222f97dddb2a6e838b64... A quick glance reveals that clisp stands out as being particularly badly supported by run-program and that there’s an error with run-program on cmucl (the one I mentioned a few lines earlier). Among the tests that are skipped, there’s some where run-program has asserts in place to catch combinations of parameters that aren’t supported by the underlying implementation but also some where run-program fails to catch problems and the implementation either generates an error or silently misbehaves (both are marked with FIXMEs in the test script). Elias
On 01 Aug 2016, at 04:25, Robert Goldman <rpgoldman@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/a45e91e81d2e973d1541adb... 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. Then there’s the run-program-messy-with-rebasing branch (dynamic) https://gitlab.common-lisp.net/epipping/asdf/commits/run-program-messy-with-... (static) https://gitlab.common-lisp.net/epipping/asdf/commits/61654b62b2d8cf6265ef968... 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/ac2d610d77806896bc0f1543.... 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). Issue #2: https://gitlab.common-lisp.net/epipping/asdf/commit/1680d3e36a54717b195f672b... and https://gitlab.common-lisp.net/epipping/asdf/commit/4b884915c4d62f07cec53e9d... 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). Issue #3: https://gitlab.common-lisp.net/epipping/asdf/commit/308faabe755d718f7fb8f272... adds getters for the streams contained in a process-info plist. Are these acceptable? 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. 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/4628f3fb57384c8d1d62aeac... 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? Elias PS: Let me know if you prefer merge requests to such emails. 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
I'm fully occupied through Monday, but will then get back to you on this. I should be able to work through your bug fix commits and get them into the master then. Best, R
On Wed, Aug 10, 2016 at 5:53 AM, Elias Pipping <pipping.elias@icloud.com> wrote:
On 01 Aug 2016, at 04:25, Robert Goldman <rpgoldman@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/a45e91e81d2e973d1541adb...
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-... (static) https://gitlab.common-lisp.net/epipping/asdf/commits/61654b62b2d8cf6265ef968...
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/ac2d610d77806896bc0f1543.... 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/1680d3e36a54717b195f672b... and https://gitlab.common-lisp.net/epipping/asdf/commit/4b884915c4d62f07cec53e9d... 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/308faabe755d718f7fb8f272... 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/4628f3fb57384c8d1d62aeac...
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?
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
I would say file-or-synonym-stream-p 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.
On 11 Aug 2016, at 17:07, Faré <fahree@gmail.com> wrote:
On Wed, Aug 10, 2016 at 5:53 AM, Elias Pipping <pipping.elias@icloud.com> wrote:
On 01 Aug 2016, at 04:25, Robert Goldman <rpgoldman@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 Faré, dear Robert, dear list, because Faré has already provided me with feedback (thanks!), I’ve now force-pushed to https://gitlab.common-lisp.net/epipping/asdf/commits/run-program-messy-with-... again (which is at revision d60e8c1e429e8cff7d84cce7f5c4271026399669 at the time of this writing), so that future feedback (still very much welcome!) can refer to this revision where past issues have already been addressed. Here’s a direct link: https://gitlab.common-lisp.net/epipping/asdf/commit/d60e8c1e429e8cff7d84cce7... The run-program branch is now gone (if I had started force-pushing it, too, it would’ve lost its purpose yet I didn’t want to have any unnecessary fix-up commits on it). I’ve also opened a merge request at https://gitlab.common-lisp.net/asdf/asdf/merge_requests/3 so that feedback can now more easily be linked with relevant lines of code.
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.
That’s fixed now. The new commit is https://gitlab.common-lisp.net/epipping/asdf/commit/e9e8db50fbc7023cf7714825... and I’ve also restored the alphabetical order in a few other places through https://gitlab.common-lisp.net/epipping/asdf/commit/8a27eb6ba7aecdc6122ba35f... As for CLASP: Getting it to compile is currently rather difficult and as promising and interesting as it sounds as a project: I would currently describe it as experimental. Even though I’ve now (after multiple attempts on multiple different linux distributions) managed to compile it, it currently has issues that keep me from even loading my test script. I’ve spoken to Nicolas Hafner aka Shinmera (on irc and github) about this and he allowed me to quote him as having said: [..] I wouldn't bother with clasp right now. If you break UIOP in any substantial way we'll notice it. If not, then, well, you can check back once things are running more smoothly again. [..] ASDF is being tested somewhat automatically. If you break anything seriously, we'll notice [..]
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.
Fixed in https://gitlab.common-lisp.net/epipping/asdf/commit/61608a271ca1b6e66649d7e8...
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.
The function has been exported since at least January of 2008: that’s how far back the history of the file that contains such exports goes here: https://github.com/Clozure/ccl/commits/master/lib/ccl-export-syms.lisp Potentially longer since said file was moved in ’08. I’ve added a comment to the commit message of https://gitlab.common-lisp.net/epipping/asdf/commit/6fcd69cc57fb76f4e2503f79...
Then there’s the run-program-messy-with-rebasing branch
(dynamic) https://gitlab.common-lisp.net/epipping/asdf/commits/run-program-messy-with-... (static) https://gitlab.common-lisp.net/epipping/asdf/commits/61654b62b2d8cf6265ef968...
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/ac2d610d77806896bc0f1543.... 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.
I’ve gone with a class now (please let me know if you prefer a struct). That gets rid of nconc and (Robert had already suggested it earlier for this reason) allows one to check if an object is a process). (Actually, I’ve called it process-info because it’s more than just a process but that’s all up for debate of course).
Issue #2:
https://gitlab.common-lisp.net/epipping/asdf/commit/1680d3e36a54717b195f672b... and https://gitlab.common-lisp.net/epipping/asdf/commit/4b884915c4d62f07cec53e9d... 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.
These are two outstanding issues: I have not exported anything yet and nothing’s documented. On my TODO list.
Please don't use nconc. Ever.
Fixed (please see above)
Issue #3:
https://gitlab.common-lisp.net/epipping/asdf/commit/308faabe755d718f7fb8f272... 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.
I’ve now introduced two condition classes and put them to use: https://gitlab.common-lisp.net/epipping/asdf/commit/5b5d38fb9253505a5219d8cd... https://gitlab.common-lisp.net/epipping/asdf/commit/ac0e77fc01717dd7fa7cc413...
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.
I’ve now moved the test to uiop/test. Is that also acceptable? (I didn’t mean to deviate from what you said, I just hadn’t read it carefully enough) Up until now, I only used the tests to check for myself if the code I’d written works as expected. I’ve now given the test suite a workover so that it’ll handle unexpected errors, too, and provide a summary. The issue is, though: A test suite that anyone further downstream (be it end users or package maintainers for a distribution) would want to run should have a certain character. You wouldn’t want it to sometimes skip 80% of the subtests and you wouldn’t want it to fail any tests (that upstream already knows about). Yet that’s precisely what happens right now. CLISP’s ext:run-program only supports a tiny subset of the %run-program interface.(*) CMU CL and MKCL both have multiple bugs (I’ve filed issues for those) that currently make quite a few tests fail, hang, or even lead to a crash of the interpreter. I’ve thus had to explicitly skip the hanging and crashing ones. A summary of the tests results is contained in the commit message of https://gitlab.common-lisp.net/epipping/asdf/commit/d60e8c1e429e8cff7d84cce7... So I’m not convinced that it makes a lot of sense to encourage downstream to run these tests at this point. (*) There’s also the private ext::launch which does many things ext:run-program doesn’t but then also doesn’t sport an actual superset of features (e.g. you cannot have it read from a file. unless you do it manually by opening a file stream and passing that stream back to the process. but then you’re in charge of closing the stream, also with :wait nil…) While we’re on that topic: ABCL has a sys:run-program function which is not currently put to use by uiop/run-program. I mean to look into that, too.
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/4628f3fb57384c8d1d62aeac...
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
Thanks, that’s a better name indeed. That’s https://gitlab.common-lisp.net/epipping/asdf/commit/d288f5557fe73f2d5383ea17... now.
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
I’ve tried to download the free version of Scieneer Lisp by the way but that apparently requires approval by someone in the company and they’ve never contacted me. So I hope to be able to add clasp to that list at some point and also abcl, but scl probably won’t make it.
PS: Are you interested in becoming new maintainer for UIOP?
I’d certainly like to help and invest time. I’m not sure if you’re asking “a maintainer” or “the maintainer”. Because there are certainly large parts of UIOP that I know nothing about and issues such as the re-initialisation of argv after dumping an image of cmucl sound intimidating, requiring a lot more understanding of common lisp, cmucl, or uiop (I don’t even know which one of those three I know too little about, potentially all three) than I currently have. So I think my answer should be: “a maintainer”: yes, but (pardon if you that’s not what you asked) “the maintainer”: not at this point in time. Elias
Dear Elias, first, I'd like to reiterate how positively impressed I am by the quality of your work on improving uiop:run-program. Thanks a lot!
https://gitlab.common-lisp.net/epipping/asdf/commits/run-program-messy-with-...
I’ve also opened a merge request at
Remaining issues I see: * missing docstrings for file-stream-p & file-or-synonym-stream-p (and any other exported function you introduce). * to avoid code duplication in 5cd51f5cd26cd29d14e5a57dfaed2b0b5602a685 I would use nest or something to separate the lispworks 7 specific code without duplicating the common code. * Also, is #+lispworks7 future-proof? Is there a feature for 7-or-later? * I was going to suggest adding a not-implemented error. I see you did. Congrats! Could you put it in utility.lisp, though? It's a general-purpose error to have. Same for parameter-error.
As for CLASP: Getting it to compile is currently rather difficult and as promising and interesting as it sounds as a project: I would currently describe it as experimental. Even though I’ve now (after multiple attempts on multiple different linux distributions) managed to compile it, it currently has issues that keep me from even loading my test script.
I’ve spoken to Nicolas Hafner aka Shinmera (on irc and github) about this and he allowed me to quote him as having said:
[..] I wouldn't bother with clasp right now. If you break UIOP in any substantial way we'll notice it. If not, then, well, you can check back once things are running more smoothly again. [..] ASDF is being tested somewhat automatically. If you break anything seriously, we'll notice [..]
Excellent! It's pleasing to see both you and they have your shit together.
The function has been exported since at least January of 2008: that’s how far back the history of the file that contains such exports goes here:
https://github.com/Clozure/ccl/commits/master/lib/ccl-export-syms.lisp
Wow, I was going to say I'm impressed they migrated from their badly-done svn to git, but I see it's a mirror and the original is still svn. Meh.
I’ve gone with a class now (please let me know if you prefer a struct). That gets rid of nconc and (Robert had already suggested it earlier for this reason) allows one to check if an object is a process). (Actually, I’ve called it process-info because it’s more than just a process but that’s all up for debate of course).
Perfect, especially with the semi-reflective use of slot-value. Much cleaner. Don't kid yourself into believing that it's either faster or less (source or binary) code, though.
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.
These are two outstanding issues: I have not exported anything yet and nothing’s documented. On my TODO list.
Thanks a lot!
I’ve now moved the test to uiop/test. Is that also acceptable? (I didn’t mean to deviate from what you said, I just hadn’t read it carefully enough)
Well, then you have to modify the test framework to find those tests where they are. (And if you're courageous, you can also update the alternate test framework I wrote.)
Up until now, I only used the tests to check for myself if the code I’d written works as expected. I’ve now given the test suite a workover so that it’ll handle unexpected errors, too, and provide a summary. The issue is, though: A test suite that anyone further downstream (be it end users or package maintainers for a distribution) would want to run should have a certain character. You wouldn’t want it to sometimes skip 80% of the subtests and you wouldn’t want it to fail any tests (that upstream already knows about). Yet that’s precisely what happens right now.
The ASDF test framework has some mechanism for having known-failures.
CLISP’s ext:run-program only supports a tiny subset of the %run-program interface.(*) CMU CL and MKCL both have multiple bugs (I’ve filed issues for those) that currently make quite a few tests fail, hang, or even lead to a crash of the interpreter. I’ve thus had to explicitly skip the hanging and crashing ones.
Welcome to the world of CL portability hacking :-(
(*) There’s also the private ext::launch which does many things ext:run-program doesn’t but then also doesn’t sport an actual superset of features (e.g. you cannot have it read from a file. unless you do it manually by opening a file stream and passing that stream back to the process. but then you’re in charge of closing the stream, also with :wait nil…) While we’re on that topic: ABCL has a sys:run-program function which is not currently put to use by uiop/run-program. I mean to look into that, too.
I hadn't looked at CLISP's ext::launch (wasn't aware of it). I believe I gave a cursory look at ABCL's sys:run-program before I decided that (at least at the time) it didn't have enough features to do what I want and I might as well fall back to using system.
I’ve tried to download the free version of Scieneer Lisp by the way but that apparently requires approval by someone in the company and they’ve never contacted me. So I hope to be able to add clasp to that list at some point and also abcl, but scl probably won’t make it.
Last I knew, Douglas Crosher seems to have stopped hacking Scieneer and to be angry at me and/or free software hackers in general.
PS: Are you interested in becoming new maintainer for UIOP?
I’d certainly like to help and invest time. I’m not sure if you’re asking “a maintainer” or “the maintainer”. Because there are certainly large parts of UIOP that I know nothing about and issues such as the re-initialisation of argv after dumping an image of cmucl sound intimidating, requiring a lot more understanding of common lisp, cmucl, or uiop (I don’t even know which one of those three I know too little about, potentially all three) than I currently have. So I think my answer should be: “a maintainer”: yes, but (pardon if you that’s not what you asked) “the maintainer”: not at this point in time.
I was thinking about "the maintainer", but you could start being "a maintainer", i.e. have commit rights. I still think you should get your code reviewed in a branch, though, especially since you're doing such a good job at it! You probably know more about the issue with argv on CMUCL than I do at this point: it's mostly fallen off my cache, and any knowledge I had was reified in uiop + cl-launch. Robert, are you OK with granting Elias the commit bit on ASDF? —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org Be yourself, everybody else is taken. — Oscar Wilde
On 13 Aug 2016, at 02:59, Faré <fahree@gmail.com> wrote:
Dear Elias,
Dear Faré,
first, I'd like to reiterate how positively impressed I am by the quality of your work on improving uiop:run-program.
thank you!
https://gitlab.common-lisp.net/epipping/asdf/commits/run-program-messy-with-...
I’ve also opened a merge request at
Remaining issues I see: * missing docstrings for file-stream-p & file-or-synonym-stream-p (and any other exported function you introduce). * to avoid code duplication in 5cd51f5cd26cd29d14e5a57dfaed2b0b5602a685 I would use nest or something to separate the lispworks 7 specific code without duplicating the common code.
I’ve now arrived at https://gitlab.common-lisp.net/epipping/asdf/commit/ffd8d8564bdc6551f2b682e7... (to be squashed into an earlier commit) that I’m rather happy with.
* Also, is #+lispworks7 future-proof? Is there a feature for 7-or-later?
I don’t think so, no. It seems that each version has a feature corresponding to its own version but not others. We could maybe provide our own, though. We cannot know what versioning scheme they’ll use in the future but it shouldn’t be too difficult to find out what features lispworks has defined in the past. So a (lispworks7-or-greater-p) could probably be implemented as (not (or #+(or lispworks4 lispworks5 lispworks6) t)) or something along those lines.
* I was going to suggest adding a not-implemented error. I see you did. Congrats! Could you put it in utility.lisp, though? It's a general-purpose error to have. Same for parameter-error.
Sure, I’ll move it. There are unfortunately quite a few more issues that I’ve become aware of and I’m not quite sure how to handle them. So I’ll describe them here for now. At the very least, they’re then archived somewhere. (A) The first is with the :overwrite default for :if-output-exists. If :output is a file, it will be opened, and open will be passed :overwrite for :if-exists. That answers the question of what one should expect to be able to specify for :if-output-exists, too. What the user has no direct control over through the run-program interface is what happens if the file does not exist. There is :if-output-does-not-exist even though open has :if-does-not-exist. Such a flag is not strictly necessary, however, because if left unspecified, :if-does-not-exist takes a value that depends on :if-exists. If :if-exists is :overwrite, :if-does-not-exist will default to :error. There’s also the :supersede argument to :if-output-exists that is identical to :overwrite except that the default for :if-does-not-exist will not be :error but :create. I would assume that these terms are well established in the CL community so that anyone who reads “the default for :if-output-exists is :overwrite” will know (1) he/she needs to make sure the file exists, that (2) he/she can expect an error to occur if it does not and (3) that :overwrite can be replaced with :supersede if (1) and (2) are undesirable. Unfortunately, that’s not always what happens with run-program. Clozure CL and SBCL behave exactly this way. Others do not. In particular, CLISP, e.g. does not accept :supersede. Instead, its :overwrite behaves like :supersede. In general, things are a bit murky. CLISP was the only platform I tested on where :supersede was not accepted. But other platforms simply do not appear to distinguish between :supersede and :overwrite, so that no error will occur with either, if the file already exists (this affects at least Allegro CL, CMU CL, ECL, LispWorks). So since all of them appear to support the behaviour of :supersede (even though not necessarily by that name) and not all of them support :overwrite, it might make sense to change the default from :overwrite to :supersede and then (like other normaliser functions) have a translator that turns :supersede into :overwrite for CLISP. This would unfortunately also affect the (exported) run-program function. On the plus side, it would only change behaviour that previously could not be relied upon anyway. (B) Process termination Even though it’s possible to kill a process on nearly ever platform somehow (not on CLISP, except for processes spawned through ext::launch) there are multiple tiers of support for that: From native to external: If you have a process in MKCL, you can terminate it through its process object, on unix or windows. That’s the ideal situation. The worst-possible situation that can still be handled if needed is when all you have is the PID: you end up calling `taskkill` on windows, through cmd.exe, or `kill` on unix, through the shell. I only recently started thinking about potential issues that this might involve other than performance, not only in this extreme case but also in cases that lie half-way between the MKCL situation and the worst-possible scenario, e.g. SBCL’s. If you obtain the PID of an arbitrary external process and then try to kill it through that PID at a later point in time, the process may no longer be running. It may not even exist anymore. Worse yet, its PID may have been recycled by the system and assigned to a newly spawned process that you end up killing by accident! Fortunately, I am not trying to solve this general problem but the simpler problem of killing a process spawned through run-program, which will necessarily be a child process. Such a process will signal its parent with SIGCHLD when it terminates and stick around as a zombie until it is waited for. So while the (interesting but not critical) transition from the running state to the zombie state lies beyond our control, the transition from the zombie state (in which we can safely send signals to the process without any effect) to no state (where the PID can be recycled by the OS whenever it deems it appropriate and sending signals could be dangerous) only occurs when requested by the user. The fact that processes become zombies after termination and require cleanup is made explicit in Allegro CL and MKCL. In both, you need to call sys:reap-os-subprocess or mkcl:join-process, respectively. If you do not, you will eventually run out of PIDs, even if only a single process is ever running at the same time. But at least you do not send signals where they don’t belong. ((B') I’m not sure what the best way to handle this requirement of reaping is. %wait-process-result currently calls both functions, so it would be possible to require that asynchronously spawned processes are waited on at some point, mirroring to some extent the requirement that every C program that calls fork() should call wait() at some point. So the name would be rather appropriate even.) On other platforms, e.g. CMU CL, ECL, and SBCL, the handler for the SIGCHLD signal calls wait() so that the user has no control over the zombie->gone transition. Both in ECL and SBCL, there are locks around the list of active processes that the handler acquires(*). SBCL’s process-kill does not currently acquire this lock, however, and a process termination facility is only at the planning stage for ECL at this point. CMU CL does not even have such a lock. So even even though CMU CL and SBCL come with a process-kill function, you cannot currently be sure if you’re sending your signal to the process that you meant to send it to (it might hit the process in running or zombie state, no process at all, or a new process!). As unlikely as it may be that such a problem will occur (an OS should not be too eager to hand out the same PID right away after all), since there is no upper limit on how much damage this could cause, I’m thinking we might want to have another function (process-termination-safe-p) for users who don’t just want to run UIOP on their NetBSD toaster. (*) I would like to thank Stas Boukarev (stassats on irc/github) and Daniel Kochmanski (jackdaniel/dkochmanski on irc/github) publicly here for helping me understand this issue in #sbcl and #ecl, respectively. (C) We’ll also need a process-close function, I think, to close any stream requested through :stream arguments to :input/:output/:error-output. I’m adding one right now.
As for CLASP: Getting it to compile is currently rather difficult and as promising and interesting as it sounds as a project: I would currently describe it as experimental. Even though I’ve now (after multiple attempts on multiple different linux distributions) managed to compile it, it currently has issues that keep me from even loading my test script.
I’ve spoken to Nicolas Hafner aka Shinmera (on irc and github) about this and he allowed me to quote him as having said:
[..] I wouldn't bother with clasp right now. If you break UIOP in any substantial way we'll notice it. If not, then, well, you can check back once things are running more smoothly again. [..] ASDF is being tested somewhat automatically. If you break anything seriously, we'll notice [..]
Excellent! It's pleasing to see both you and they have your shit together.
The function has been exported since at least January of 2008: that’s how far back the history of the file that contains such exports goes here:
https://github.com/Clozure/ccl/commits/master/lib/ccl-export-syms.lisp
Wow, I was going to say I'm impressed they migrated from their badly-done svn to git, but I see it's a mirror and the original is still svn. Meh.
I’ve gone with a class now (please let me know if you prefer a struct). That gets rid of nconc and (Robert had already suggested it earlier for this reason) allows one to check if an object is a process). (Actually, I’ve called it process-info because it’s more than just a process but that’s all up for debate of course).
Perfect, especially with the semi-reflective use of slot-value. Much cleaner. Don't kid yourself into believing that it's either faster or less (source or binary) code, though.
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.
These are two outstanding issues: I have not exported anything yet and nothing’s documented. On my TODO list.
Thanks a lot!
I’ve now moved the test to uiop/test. Is that also acceptable? (I didn’t mean to deviate from what you said, I just hadn’t read it carefully enough)
Well, then you have to modify the test framework to find those tests where they are. (And if you're courageous, you can also update the alternate test framework I wrote.)
Up until now, I only used the tests to check for myself if the code I’d written works as expected. I’ve now given the test suite a workover so that it’ll handle unexpected errors, too, and provide a summary. The issue is, though: A test suite that anyone further downstream (be it end users or package maintainers for a distribution) would want to run should have a certain character. You wouldn’t want it to sometimes skip 80% of the subtests and you wouldn’t want it to fail any tests (that upstream already knows about). Yet that’s precisely what happens right now.
The ASDF test framework has some mechanism for having known-failures.
CLISP’s ext:run-program only supports a tiny subset of the %run-program interface.(*) CMU CL and MKCL both have multiple bugs (I’ve filed issues for those) that currently make quite a few tests fail, hang, or even lead to a crash of the interpreter. I’ve thus had to explicitly skip the hanging and crashing ones.
Welcome to the world of CL portability hacking :-(
(*) There’s also the private ext::launch which does many things ext:run-program doesn’t but then also doesn’t sport an actual superset of features (e.g. you cannot have it read from a file. unless you do it manually by opening a file stream and passing that stream back to the process. but then you’re in charge of closing the stream, also with :wait nil…) While we’re on that topic: ABCL has a sys:run-program function which is not currently put to use by uiop/run-program. I mean to look into that, too.
I hadn't looked at CLISP's ext::launch (wasn't aware of it). I believe I gave a cursory look at ABCL's sys:run-program before I decided that (at least at the time) it didn't have enough features to do what I want and I might as well fall back to using system.
That’s probably because it does not have :input, :output, :error specifiers (or :if-*-exists/does-not-exist): It behaves as if all three were set to :stream. The three streams are consequently always returned. It does not fit very well with what the other implementations do, indeed.
I’ve tried to download the free version of Scieneer Lisp by the way but that apparently requires approval by someone in the company and they’ve never contacted me. So I hope to be able to add clasp to that list at some point and also abcl, but scl probably won’t make it.
Last I knew, Douglas Crosher seems to have stopped hacking Scieneer and to be angry at me and/or free software hackers in general.
PS: Are you interested in becoming new maintainer for UIOP?
I’d certainly like to help and invest time. I’m not sure if you’re asking “a maintainer” or “the maintainer”. Because there are certainly large parts of UIOP that I know nothing about and issues such as the re-initialisation of argv after dumping an image of cmucl sound intimidating, requiring a lot more understanding of common lisp, cmucl, or uiop (I don’t even know which one of those three I know too little about, potentially all three) than I currently have. So I think my answer should be: “a maintainer”: yes, but (pardon if you that’s not what you asked) “the maintainer”: not at this point in time.
I was thinking about "the maintainer", but you could start being "a maintainer", i.e. have commit rights. I still think you should get your code reviewed in a branch, though, especially since you're doing such a good job at it!
Feature branches are perfect for me. Elias PS: I posted some of my early thoughts on the race condition that code such as (when (process-alive-p p) (terminate-process p)) would be affected by as a comment to the merge request. But I would assume no notification emails were sent out. (I’ve now removed the comment)
You probably know more about the issue with argv on CMUCL than I do at this point: it's mostly fallen off my cache, and any knowledge I had was reified in uiop + cl-launch.
Robert, are you OK with granting Elias the commit bit on ASDF?
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org Be yourself, everybody else is taken. — Oscar Wilde
https://gitlab.common-lisp.net/epipping/asdf/commit/ffd8d8564bdc6551f2b682e7...
(to be squashed into an earlier commit) that I’m rather happy with.
Uh, your comment suggests that on lispworks6, your destructuring-bind is wrong. Note that I'm OK with declaring we don't support lispworks6 anymore, but that must be made explicit. Also, ask Robert what he thinks about it.
* Also, is #+lispworks7 future-proof? Is there a feature for 7-or-later?
I don’t think so, no. It seems that each version has a feature corresponding to its own version but not others. We could maybe provide our own, though. We cannot know what versioning scheme they’ll use in the future but it shouldn’t be too difficult to find out what features lispworks has defined in the past. So a
(lispworks7-or-greater-p)
could probably be implemented as
(not (or #+(or lispworks4 lispworks5 lispworks6) t))
or something along those lines.
Yuck. I'd rather query SYSTEM::*MAJOR-VERSION-NUMBER* in a #..
(A) The first is with the :overwrite default for :if-output-exists. [...] So since all [implementations] appear to support the behaviour of :supersede (even though not necessarily by that name) and not all of them support :overwrite, it might make sense to change the default from :overwrite to :supersede and then (like other normaliser functions) have a translator that turns :supersede into :overwrite for CLISP. This would unfortunately also affect the (exported) run-program function. On the plus side, it would only change behaviour that previously could not be relied upon anyway.
Yes, that would be the Right Thing(tm). The entire purpose of UIOP is to provide some portable abstractions with stable semantics abstracting over implementation specifics, rather than functions the meaning of which is actually not portable. See my ASDF3 essay on this topic.
(B) Process termination
Even though it’s possible to kill a process on nearly ever platform somehow (not on CLISP, except for processes spawned through ext::launch) there are multiple tiers of support for that: From native to external: If you have a process in MKCL, you can terminate it through its process object, on unix or windows. That’s the ideal situation.
If it helps with CLISP, maybe ext::launch should be the default on CLISP. I admit I don't care too much about CLISP, that hasn't been actively maintained for years. (It still ships with ASDF 2.33, for John McCarthy's sake.)
The worst-possible situation that can still be handled if needed is when all you have is the PID: you end up calling `taskkill` on windows, through cmd.exe, or `kill` on unix, through the shell. I only recently started thinking about potential issues that this might involve other than performance, not only in this extreme case but also in cases that lie half-way between the MKCL situation and the worst-possible scenario, e.g. SBCL’s.
If you obtain the PID of an arbitrary external process and then try to kill it through that PID at a later point in time, the process may no longer be running. It may not even exist anymore. Worse yet, its PID may have been recycled by the system and assigned to a newly spawned process that you end up killing by accident!
Note that on Unix, the race condition between killing a process and the process dying then its PID being recycled is inherent, whatever abstraction MKCL may try to provide that SBCL doesn't. The only mitigating behavior would be for the kernel to insert a pause before a PID could be used, and hope that programs never try kill a PID more than that pause duration after having checked the process was alive (and even then, if the process is kill -STOP'ped between the check and the kill, there's nothing that can prevent the race condition). Of course, 16-bit PIDs make the situation particularly tight on Unix
((B') I’m not sure what the best way to handle this requirement of reaping is. %wait-process-result currently calls both functions, so it would be possible to require that asynchronously spawned processes are waited on at some point, mirroring to some extent the requirement that every C program that calls fork() should call wait() at some point. So the name would be rather appropriate even.)
Yes, it is fair to require that asynchronous UIOP:RUN-PROGRAM users should always call wait-process (or whatever you call the API function) and not rely on the implementation doing it for them. —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org Solipsism is a lonely place. Psychopaths crave love, but can't get no satisfaction: even elected by millions, it's still non-people voting for them.
On 15 Aug 2016, at 15:07, Faré <fahree@gmail.com> wrote:
https://gitlab.common-lisp.net/epipping/asdf/commit/ffd8d8564bdc6551f2b682e7...
(to be squashed into an earlier commit) that I’m rather happy with.
Uh, your comment suggests that on lispworks6, your destructuring-bind is wrong.
Oh, the comment may be doing more harm than good then. The code is actually correct. It’s just that in the if-branch, lispworks 6 will return (io,err,pid), and in the else-branch (pid), whereas lispworks7 will return (io,err,pid) in both branches.
Note that I'm OK with declaring we don't support lispworks6 anymore, but that must be made explicit. Also, ask Robert what he thinks about it.
I don’t think this will be necessary. I haven’t been able to test with lispworks 5 but lispworks 6 works pretty well.
* Also, is #+lispworks7 future-proof? Is there a feature for 7-or-later?
I don’t think so, no. It seems that each version has a feature corresponding to its own version but not others. We could maybe provide our own, though. We cannot know what versioning scheme they’ll use in the future but it shouldn’t be too difficult to find out what features lispworks has defined in the past. So a
(lispworks7-or-greater-p)
could probably be implemented as
(not (or #+(or lispworks4 lispworks5 lispworks6) t))
or something along those lines.
Yuck. I'd rather query SYSTEM::*MAJOR-VERSION-NUMBER* in a #..
I didn’t know about that variable. The resulting code would be nicer but it’s still a private symbol which might disappear in the future. Personally, I’d prefer to go with the uglier but more official version.
(A) The first is with the :overwrite default for :if-output-exists. [...] So since all [implementations] appear to support the behaviour of :supersede (even though not necessarily by that name) and not all of them support :overwrite, it might make sense to change the default from :overwrite to :supersede and then (like other normaliser functions) have a translator that turns :supersede into :overwrite for CLISP. This would unfortunately also affect the (exported) run-program function. On the plus side, it would only change behaviour that previously could not be relied upon anyway.
Yes, that would be the Right Thing(tm). The entire purpose of UIOP is to provide some portable abstractions with stable semantics abstracting over implementation specifics, rather than functions the meaning of which is actually not portable. See my ASDF3 essay on this topic.
(B) Process termination
Even though it’s possible to kill a process on nearly ever platform somehow (not on CLISP, except for processes spawned through ext::launch) there are multiple tiers of support for that: From native to external: If you have a process in MKCL, you can terminate it through its process object, on unix or windows. That’s the ideal situation.
If it helps with CLISP, maybe ext::launch should be the default on CLISP. I admit I don't care too much about CLISP, that hasn't been actively maintained for years. (It still ships with ASDF 2.33, for John McCarthy's sake.)
It appears there are some attempts to breathe new life into CLISP, though. See e.g. https://sourceforge.net/u/musteresel/clisp/
The worst-possible situation that can still be handled if needed is when all you have is the PID: you end up calling `taskkill` on windows, through cmd.exe, or `kill` on unix, through the shell. I only recently started thinking about potential issues that this might involve other than performance, not only in this extreme case but also in cases that lie half-way between the MKCL situation and the worst-possible scenario, e.g. SBCL’s.
If you obtain the PID of an arbitrary external process and then try to kill it through that PID at a later point in time, the process may no longer be running. It may not even exist anymore. Worse yet, its PID may have been recycled by the system and assigned to a newly spawned process that you end up killing by accident!
Note that on Unix, the race condition between killing a process and the process dying then its PID being recycled is inherent, whatever abstraction MKCL may try to provide that SBCL doesn’t.
Yes, but we’re not dealing with this general problem as I tried to outline in the paragraph after that (quoting myself): Fortunately, I am not trying to solve this general problem but the simpler problem of killing a process spawned through run-program, which will necessarily be a child process. Such a process will signal its parent with SIGCHLD when it terminates and stick around as a zombie until it is waited for. So the issue is solvable here, no?
The only mitigating behavior would be for the kernel to insert a pause before a PID could be used, and hope that programs never try kill a PID more than that pause duration after having checked the process was alive (and even then, if the process is kill -STOP'ped between the check and the kill, there's nothing that can prevent the race condition). Of course, 16-bit PIDs make the situation particularly tight on Unix
((B') I’m not sure what the best way to handle this requirement of reaping is. %wait-process-result currently calls both functions, so it would be possible to require that asynchronously spawned processes are waited on at some point, mirroring to some extent the requirement that every C program that calls fork() should call wait() at some point. So the name would be rather appropriate even.)
Yes, it is fair to require that asynchronous UIOP:RUN-PROGRAM users should always call wait-process (or whatever you call the API function) and not rely on the implementation doing it for them.
Okay, great. Elias
Elias, I've lost track a little bit: will you please tell me how to get the uncontroversial patches from your fork? I'd like to try those first. Thanks, r
On 15 Aug 2016, at 15:44, Robert Goldman <rpgoldman@sift.net> wrote:
Elias,
I've lost track a little bit: will you please tell me how to get the uncontroversial patches from your fork?
I'd like to try those first.
Dear Robert, I’m sorry, the branch is not actually a complete mess but it may look like it at first. I’ve kept the commits arranged such that the pure bug fixes come first and then later what’s still under construction. So there are three segments: # Entirely uncontroversial 8a27eb6 Restore alphabetical order 7e0aec2 Bug fix: Pass :if-*-exists on to Allegro CL e9e8db5 Use ext: instead of si: on Embeddable Common-Lisp f294a3f Signal an error on unsupported platforms 61608a2 Restrict assertion to cases where it is necessary 6fcd69c Replace a private Clozure CL function call 4673c13 Remove superfluous "not implemented" error c661d89 Bug fix: Exit status with Allegro CL after signal # Could use a second opinion (the first two, the other two just depend on them) e1cb30d Add process-info class d288f55 Add and use file-stream*-p 14c9859 Bug fix: Exit code with LispWorks 7 98f929a Bug fix: Store exit code with Allegro CL # WIP f92d032 Add and use not-implemented-error 2706cbb Add and use parameter-error 9d5b7ba Move parameter error handling for CLISP 5957b74 Add %terminate-process and %posix-process-send-signal 8fddc7a Add %process-alive-p and %process-status 7e53d49 Add %process-info-*put getters 5c089c3 Add %process-close 9e8c0d5 Add a preliminary test. Current output: The order here is the same as on the branch. Elias
Dear Faré, dear Robert, dear list,
On 15 Aug 2016, at 15:25, Elias Pipping <pipping.elias@icloud.com> wrote:
* Also, is #+lispworks7 future-proof? Is there a feature for 7-or-later?
I don’t think so, no. It seems that each version has a feature corresponding to its own version but not others. We could maybe provide our own, though. We cannot know what versioning scheme they’ll use in the future but it shouldn’t be too difficult to find out what features lispworks has defined in the past. So a
(lispworks7-or-greater-p)
could probably be implemented as
(not (or #+(or lispworks4 lispworks5 lispworks6) t))
or something along those lines.
Yuck. I'd rather query SYSTEM::*MAJOR-VERSION-NUMBER* in a #..
I didn’t know about that variable. The resulting code would be nicer but it’s still a private symbol which might disappear in the future. Personally, I’d prefer to go with the uglier but more official version.
Indeed, http://www.lispworks.com/documentation/lw70/LW/html/lw-373.htm explicitly mentions that (defvar *feature-added-in-LispWorks-7.0* #+(or lispworks4 lispworks5 lispworks6) nil #-(or lispworks4 lispworks5 lispworks6) t) is the proper way to check for LispWorks 7.0 and above. Based on that, I’ve added (defmacro %if-on-lispworks7+ (action-if action-else) #+(and lispworks (not (or lispworks4 lispworks5 lispworks6))) action-if #-(and lispworks (not (or lispworks4 lispworks5 lispworks6))) action-else) in https://gitlab.common-lisp.net/epipping/asdf/commit/cbeb4b3c75ad777f668a48d0... and started using it. The name might not be ideal. We might also want to replace this with a more general version checking macro for LispWorks in the future but I don’t yet see the need for that. I wasn’t sure where to put it: os.lisp is currently most fitting but still wrong and I was reluctant to add a new module so I left it in run-program.lisp for now.
(A) The first is with the :overwrite default for :if-output-exists. [...] So since all [implementations] appear to support the behaviour of :supersede (even though not necessarily by that name) and not all of them support :overwrite, it might make sense to change the default from :overwrite to :supersede and then (like other normaliser functions) have a translator that turns :supersede into :overwrite for CLISP. This would unfortunately also affect the (exported) run-program function. On the plus side, it would only change behaviour that previously could not be relied upon anyway.
Yes, that would be the Right Thing(tm). The entire purpose of UIOP is to provide some portable abstractions with stable semantics abstracting over implementation specifics, rather than functions the meaning of which is actually not portable. See my ASDF3 essay on this topic.
(B) Process termination
Even though it’s possible to kill a process on nearly ever platform somehow (not on CLISP, except for processes spawned through ext::launch) there are multiple tiers of support for that: From native to external: If you have a process in MKCL, you can terminate it through its process object, on unix or windows. That’s the ideal situation.
If it helps with CLISP, maybe ext::launch should be the default on CLISP. I admit I don't care too much about CLISP, that hasn't been actively maintained for years. (It still ships with ASDF 2.33, for John McCarthy's sake.)
It appears there are some attempts to breathe new life into CLISP, though. See e.g.
https://sourceforge.net/u/musteresel/clisp/
The worst-possible situation that can still be handled if needed is when all you have is the PID: you end up calling `taskkill` on windows, through cmd.exe, or `kill` on unix, through the shell. I only recently started thinking about potential issues that this might involve other than performance, not only in this extreme case but also in cases that lie half-way between the MKCL situation and the worst-possible scenario, e.g. SBCL’s.
If you obtain the PID of an arbitrary external process and then try to kill it through that PID at a later point in time, the process may no longer be running. It may not even exist anymore. Worse yet, its PID may have been recycled by the system and assigned to a newly spawned process that you end up killing by accident!
Note that on Unix, the race condition between killing a process and the process dying then its PID being recycled is inherent, whatever abstraction MKCL may try to provide that SBCL doesn’t.
Yes, but we’re not dealing with this general problem as I tried to outline in the paragraph after that (quoting myself):
Fortunately, I am not trying to solve this general problem but the simpler problem of killing a process spawned through run-program, which will necessarily be a child process. Such a process will signal its parent with SIGCHLD when it terminates and stick around as a zombie until it is waited for.
So the issue is solvable here, no?
I still think that killing a child process can be safe if the SIGCHLD handler (which would call wait()) acquires the same lock as the process killing function. But maybe I’m missing something.
The only mitigating behavior would be for the kernel to insert a pause before a PID could be used, and hope that programs never try kill a PID more than that pause duration after having checked the process was alive (and even then, if the process is kill -STOP'ped between the check and the kill, there's nothing that can prevent the race condition).
From my understanding, the user has control over this pause: Until he or she calls wait(), the process will be in zombie state and its PID will be unavailable to the OS for recycling, no?
Of course, 16-bit PIDs make the situation particularly tight on Unix
((B') I’m not sure what the best way to handle this requirement of reaping is. %wait-process-result currently calls both functions, so it would be possible to require that asynchronously spawned processes are waited on at some point, mirroring to some extent the requirement that every C program that calls fork() should call wait() at some point. So the name would be rather appropriate even.)
Yes, it is fair to require that asynchronous UIOP:RUN-PROGRAM users should always call wait-process (or whatever you call the API function) and not rely on the implementation doing it for them.
I’ve now documented this. Indeed, there’s documentation for - the now exported function wait-process-result: https://gitlab.common-lisp.net/epipping/asdf/commit/7243fe25bb8bf9164d3bd659... - the new, exported terminate-process: https://gitlab.common-lisp.net/epipping/asdf/commit/fa289e53700d92ff8f7c2da3... - the new, exported process-alive-p: https://gitlab.common-lisp.net/epipping/asdf/commit/4a1339e8e8ce83408251c926... - the new, exported process-close: https://gitlab.common-lisp.net/epipping/asdf/commit/0cc3f20e7fcc1ae9250ce241... I’m not yet sure what the best way to export the asynchronous process spawning functionality is. %run-program is not supposed to replace run-program after all. So I either need to extend run-program or add another wrapper that calls %run-program in a different fashion. In terms of return values, the latter would be more consistent: run-program would return an exit code and the new function (`start`?) would return a process-info object. I’ve also carried out miscellaneous smaller fixes(*). Parameter errors now occur early and in the same place: https://gitlab.common-lisp.net/epipping/asdf/commit/cf47cd04a509cd923f2368f9... I’ve plugged the test into the existing test suite, so that it can be run e.g. via make; ./test/run-tests.sh sbcl run-program-unix-tests.script I’ll still have to mark all the known failures as such. Come to think of it, I should probably handle those known issues not just in the test suite but also in run-program itself, albeit maybe through another error class (e.g. 'known-bug rather than 'parameter-error). Or maybe not (does it help to know that a feature is not missing but broken on your platform? it doesn’t really…). (*) Especially after all the rebasing, I think the merge request at https://gitlab.common-lisp.net/asdf/asdf/merge_requests/3 continues to be the best way to inspect the changes I’ve made, be it commit-by commit or in terms of overall changes. Elias
Dear Faré,
On 31 Jul 2016, at 21:56, Faré <fahree@gmail.com> wrote:
this kind of functionality is welcome in UIOP.
I’m happy to hear that.
However, the responsibility is yours to ensure that it will work on every implementation on every platform.
For Linux and if necessary also other unix-like systems (macOS, FreeBSD) I think I can do that myself without access to build bots. For anything else (that leaves mostly windows I guess), I’d need help since I have zero knowledge of command line access and automation on that platform.
Please make reasonably sure it works before you submit the patch. You may contact various vendors for a test license as appropriate.
I’ve installed cl-launch (I’m no longer quite sure how I managed to live without it); so far I’ve got allegro cl, clozure cl, cmu cl, ecl, and sbcl working. I’ll have to look into clisp and mkcl a bit to get them to work properly (no asdf / old asdf). I’ve also contacted LispWorks.
Note that there are several run-program style libraries around. If you're going to do this, I suggest you look at each and every of these libraries (notably executor and external-program) and make sure you have at least feature-parity and implementation-parity with them. Also make sure that you explicitly raise an error on unsupported implementations, at the start of every function that doesn't support them.
Thanks for the pointer, I had not heard of executor (I’m familiar with the source code of external-program, though).
Also, if you are going to support these interface, you can graduate them out of % namespace (but keep the % name around for backward compatibility for a year or two).
Understood.
Would you be interested in becoming official maintainer for UIOP?
Yes, I would. I’m confident that I’d be able to respond to issues in a timely fashion and I’m willing to learn what I think I’d need to learn. Elias
participants (4)
-
Elias Pipping -
Faré -
Jason Miller -
Robert Goldman