> On 11 Aug 2016, at 17:07, Faré <fah...@gmail.com> wrote:
> 
> 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 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-rebasing

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/d60e8c1e429e8cff7d84cce7f5c4271026399669

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/e9e8db50fbc7023cf7714825640c7a9caaeca4ec

and I’ve also restored the alphabetical order in a few other places through

  
https://gitlab.common-lisp.net/epipping/asdf/commit/8a27eb6ba7aecdc6122ba35f4d2d8483263c1e3d

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/61608a271ca1b6e66649d7e856675dcedd2fa9e3

> 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/6fcd69cc57fb76f4e2503f794e2cd67c4b7d36c6

>> 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.

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/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.

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/308faabe755d718f7fb8f2723d3615a64d50c44fadds
>>  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/5b5d38fb9253505a5219d8cdebe8eb5a7c5bb1e5
https://gitlab.common-lisp.net/epipping/asdf/commit/ac0e77fc01717dd7fa7cc413dfd4b68c11360855

> 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/d60e8c1e429e8cff7d84cce7f5c4271026399669

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/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

Thanks, that’s a better name indeed. That’s

  
https://gitlab.common-lisp.net/epipping/asdf/commit/d288f5557fe73f2d5383ea17dac0a4b3edb64aec

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

Reply via email to