armedbear / abcl

Armed Bear Common Lisp <git+https://github.com/armedbear/abcl/> <--> <svn+https://abcl.org/svn> Bridge
https://abcl.org#rdfs:seeAlso<https://gitlab.common-lisp.net/abcl/abcl>
Other
288 stars 29 forks source link

Add support for Gray stream implementation of interactive-stream-p #640

Closed yitzchak closed 9 months ago

yitzchak commented 9 months ago

Also make the Java methods isInputStream, isOutputStream, isCharacterStream and isBinaryStream, isOpen call the appropriate Gray stream methods versus assuming specific inheritance.

easye commented 9 months ago

Sent from my iPad

On Nov 22, 2023, at 14:57, Tarn W. Burton @.***> wrote:

Also, do you want me to update the change log as I make PRs?

That would be welcome, but do it in separate commits if possible please, otherwise merging commits can get needlessly complicated.

easye commented 9 months ago

@yitzchak Finally found some time to test, and it seems the version of SLIME which uses GRAY-STREAMS won't run with this patch, as one gets the following stack trace.

Error (FORMAT-ERROR) during printing: #<TYPE-ERROR {5E7E2642}>
   [Condition of type TYPE-ERROR]

Restarts:
 0: [*ABORT] Return to SLIME's top level.
 1: [ABORT] Abort thread.

Backtrace:
  0: (INVOKE-DEBUGGER #<TYPE-ERROR {5E7E2642}>)
  1: org.armedbear.lisp.Lisp.error(Lisp.java:385)
  2: org.armedbear.lisp.Lisp.type_error(Lisp.java:459)
  3: org.armedbear.lisp.Lisp.checkFunction(Lisp.java:1852)
  4: (MAKE-TWO-WAY-STREAM #<SWANK/GRAY::SLIME-INPUT-STREAM {225BAD86}> #<SWANK/GRAY::SLIME-OUTPUT-STREAM {675927A9}>)

One needs an unreleased version of SLIME past v2.28 that incorporates https://github.com/slime/slime/commit/96de8b0baae985ff7589dd11c83db066d50bc566. Previously ABCL had used a special, used-for-SLIME-only Java-side implementation of streams to communicate with SLIME (see https://github.com/armedbear/abcl/blob/master/src/org/armedbear/lisp/SlimeInputStream.java). With this patch, SLIME uses GRAY-STREAMS like most other implementations.

Again, I think the problem with your patch stems from the removal of the SUBTYPEP checks. But now that I look at it, the problem might be able to be solved by changing the MAKE-TWO-WAY-STREAM implementation (cf. https://github.com/armedbear/abcl/blob/6ae3f67c81e4d984d2345778fe53002d3acc9f38/src/org/armedbear/lisp/TwoWayStream.java#L227 ff.)

easye commented 9 months ago

I think I see the better way to fix things.

Lisp.checkStream() isn't checking for subtypes of streams https://github.com/armedbear/abcl/blob/master/src/org/armedbear/lisp/Lisp.java#L1861.

Investigating a fix…

yitzchak commented 9 months ago

Ok, thank you for that.

On my side I have tested this on about a dozen systems that depend on trivial-gray-streams by loading and using those systems or by running the included tests. I haven't seen any issues yet. This isn't an exhaustive test but it included archive, babel, chunga, flexi-streams, and nibbles-streams. I also tested on the printer stack Incless/Inravina/Invistra which implements the CL printer and relies on Gray streams heavily.

easye commented 9 months ago

@yitzchak I've added patches on top of your work in https://github.com/easye/abcl/tree/gray-interactive to start to address the issue with use of SLIME, as well as being more robust in general.

I am able to use SLIME with the ABCL GRAY-STREAM patch to connect, but the repl dies on the first interactive command issued

I'm out of time at the moment to continue, so sharing where I've gotten.

yitzchak commented 9 months ago

@easye I've reproduced your issues and I think I have a fix. I'll cherry pick your commits and update the PR later today.

Also, I think slime needs some updates for ABCL. Specifically, I got an undefined symbol for abcl-introspect/sys:find-locals which I was able to fix by replacing in swank with sys:find-locals.

yitzchak commented 9 months ago

@easye I've pushed a commit that fixed the issue for me. The commit history will need to be reworked and the change log updated.

One thing I don't completely understand yet is why FIND-METHOD is needed at all. It seems like the current strategy will ignore specializations which are superclasses of the Gray stream instance and will also ignore specialization of the second argument for methods like STREAM-WRITE-SEQUENCE. Why can't we just call the generic functions directly?

easye commented 9 months ago

Why can't we just call the generic functions directly?

I don't know of a reason off hand, but I think I remember trying that early on in the fixes to GRAY-STREAMS for abcl-1.9.2, and it didn't work, so I ended up using FIND-METHOD.

I would be in favor of using the generic methods directly, as your criticism that the current of use FIND-METHOD will not work for some specializations.

Go ahead, and see if using the generic methods directly works for you?

yitzchak commented 9 months ago

Go ahead, and see if using the generic methods directly works for you?

@easye Will do.

easye commented 9 months ago

isn't an exhaustive test but it included archive, babel, chunga, flexi-streams, and nibbles-streams

This is the test matrix from my notes, which seems more-or-less the same as the systems you tested. I derived this list by looking at every Quicklisp system that references TRIVIAL-GRAY-STREAMS

| System          | tests | Pass  | Notes                                                            |
|-----------------+-------+-------+------------------------------------------------------------------|
| able            | no    |       | TCL editor seems to come up                                      |
| archive         | no    |       | abcl to read tar file contents                                   |
| babel           | yes   |       |                                                                  |
| flexi-stream    | yes   |       |                                                                  |
| bitio           | no    |       |                                                                  |
| bodge-utilities | no    |       | simple use of BODGE-UTL:MAKE-BOUNDED-INPUT-STREAM                |
| chunga          | no    |       |                                                                  |
| dexador         | yes   |       | Tests seize up with some sort of signal (SBCL also has problems) |
|                 |       |       |                                                                  |
easye commented 9 months ago

SLIME is loading fine for me with your patch to my FIND-METHOD call missing the second argument https://github.com/armedbear/abcl/pull/640/commits/2f3cd57b0128a81e2b8e738585d3a870b2e3a1f4.

Now I seem to be getting problems in using ASDF from the REPL, seeing errors like:

Error while trying to load definition for system abcl-prove from
pathname /Users/evenson/work/abcl/abcl-prove.asd:
   The value NIL is not of type #<STANDARD-CLASS STANDARD-METHOD {7F20185B}>.

[…] 

Backtrace:
  0: (INVOKE-DEBUGGER #<ASDF/FIND-SYSTEM:LOAD-SYSTEM-DEFINITION-ERROR {5441A65}>)
  1: (ERROR ASDF/FIND-SYSTEM:LOAD-SYSTEM-DEFINITION-ERROR :NAME "abcl-prove" :PATHNAME #P"/Users/evenson/work/abcl/abcl-prove.asd" :CONDITION #<SIMPLE-TYPE-ERROR {126215B1}>)
  2: (#<LOCAL-FUNCTION IN METHOD PERFORM NIL (DEFINE-OP SYSTEM) {6896CB97}> #<SIMPLE-TYPE-ERROR {126215B1}>)
  3: (SIGNAL #<SIMPLE-TYPE-ERROR {126215B1}>)
  4: (MOP:METHOD-FUNCTION NIL)
  5: (SYSTEM::%LOAD #P"/Users/evenson/work/abcl/abcl-prove.asd" T NIL T :UTF-8)
  6: (LOAD #P"/Users/evenson/work/abcl/abcl-prove.asd" :EXTERNAL-FORMAT :UTF-8)

Interestingly, loading things via QL:QUICKLOAD still seems to work.

easye commented 9 months ago

Now I seem to be getting problems in using ASDF from the REP

Fixed this with https://github.com/easye/abcl/commit/54f10232a46101236f920d65b016d6c67c3fa52d.

Gonna do a little more testing, and push these changes.

Afterwards, I will experiment with the direct use of generic functions, as "all" the error we seem to find here comes from the manual use of FIND-METHOD.

easye commented 9 months ago

Superseded by https://github.com/armedbear/abcl/pull/641.

Thanks again for the work.