Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lem with other CL implementations #686

Merged
merged 39 commits into from
Jun 23, 2023

Conversation

Sasanidas
Copy link
Member

@Sasanidas Sasanidas commented Jun 5, 2023

So, this is the first try of making Lem compile/work with other implementations, I manage to make it compile the ncurses front-end with abcl,:
abcl

(It should of course also work with SBCL)

Compilation progress (make the :lem system compile)

  • ABCL
  • CCL
  • ECL
  • Clasp

Most tests green

  • ABCL
  • CCL
  • ECL
  • Clasp

Front-end ncurses progress (run the system)

  • ABCL
  • CCL
  • ECL
  • Clasp

"Usable state" (it can be used like sbcl)

  • ABCL
  • CCL
  • ECL
  • Clasp

- Modify the mailbox implementation
- Add +sbcl macro to a lot of places
- It compiles with ABCL v1.9.0
@Sasanidas Sasanidas changed the title First try Lem with other CL implementations Jun 5, 2023
@Sasanidas
Copy link
Member Author

I had better luck with CCL, which works quite better, except for some errors here and there, it's usable 👍

@Sasanidas
Copy link
Member Author

So, this is the first try of making Lem compile/work with other implementations, I manage to make it compile the ncurses front-end with abcl,: abcl

Buut sadly, when launching (lem:lem), it just shows the tmp buffer and doesn't respond to any command: abcl_lem

The good news, is that it also is working with sbcl, but of course more testing is needed +1

Same thing with ECL, it compiles but it doesn't register any input

@Sasanidas
Copy link
Member Author

Sasanidas commented Jun 5, 2023

So, loading the ncurses front-end, I realize that ABCL and ECL have the same problem, no response from the user input.

I debug the *editor-event-queue* and seems like the events are indeed been receive (it shouldn't be a problem in the change of timer because SBCL-Lem does work)

Maybe a library that I missed ❓

@cxxxr
Copy link
Member

cxxxr commented Jun 5, 2023

That's wonderful, thank you!

ECL should have worked before, maybe there is a problem around threads
I'll dig a little deeper tonight (at JST) and see what I can find.

@cxxxr
Copy link
Member

cxxxr commented Jun 5, 2023

ECL is failing a lot of tests, so it might not work without going through that one.
Of course, I'll leave the policy up to you.

@Sasanidas
Copy link
Member Author

Yeah, I had some problems with ECL, specially the annoying ASDF version error (not related to Lem), so I think I'm going to focus on ABCL, I think it's the one with more potential uses (open the Java ecosystem)

- Modify timer and the tests to reflect the changes
@Sasanidas
Copy link
Member Author

I did a couple of things in the last commit c047386 :

  • I adapt the library mailbox from SBCL to a CL ANSI version, all the tests seems to pass but still there is an ignore errors that I'm not sure how to fix (I'll add a TODO in it)
  • Modify again timer to reflect these changes
  • Also I add all the tests from the mailbox SBCL to this ANSI version to the rove framework, and all green for now.

I still didn't manage to make it work with ABCL tho, but I think I'm in the right direction here 👍

@Sasanidas
Copy link
Member Author

Maybe the mailbox library can be separated in a separate repository?
I couldn't found anything similar online (there is mailbox, but it's quite limited compare to this version), and it can be used for other people/projects

@cxxxr
Copy link
Member

cxxxr commented Jun 6, 2023

Maybe the mailbox library can be separated in a separate repository?

It's might be good.

@Sasanidas
Copy link
Member Author

I added some documentation more detail on how the progress in the tests is going (using this branch as reference)
lem-project/lem-project.github.io@8728344

@Sasanidas
Copy link
Member Author

Sasanidas commented Jun 10, 2023

OK, I think I found THE problem:

Selection_001

So, the thing is when calling collecting-subclasses, in signal condition in signal handler, it breaks because the subclass cannot be coerce into a simple condition.

So, why it works in SBCL? Because is seems like it's doing some wrapping around the classes, let's see the call to the function (this functions returns a list, in this case, I'm inspecting the first element):

(collect-subclasses 'lem-core::before-executing-command :include-itself nil)

SBCL:
Selection_002

Clasp:
Selection_003

In ABCL happens the same as in Clasp.

So, in the first case, it can be signal without any problem, but in the second it cannot be coerce. I guess the odd (or clever) behaviour is in SBCL.

Not sure what to do here, so calling for some help (@cxxxr)

@cxxxr
Copy link
Member

cxxxr commented Jun 10, 2023

Hmm, this seems to be a deep-seated problem.
The signal-holder mechanism is unnecessarily complicated and may be better replaced by a hook.

@cxxxr
Copy link
Member

cxxxr commented Jun 11, 2023

@Sasanidas
#716
I think I've merged the PR and this problem has been resolved, what do you think?

@Sasanidas
Copy link
Member Author

@Sasanidas #716 I think I've merged the PR and this problem has been resolved, what do you think?

Oh! seems very interesting, I'll merge the branch and try it 👍

@Sasanidas
Copy link
Member Author

Even tho ECL works really well (maybe the best aside from SBCL) I wasn't been able to dump the image to an executable.

Running https://ecl.common-lisp.dev/static/manual/System-building.html#Compiling-with-ASDF does creates an executable, but it the stack crashes when I try to run it.

@Sasanidas
Copy link
Member Author

Using (#756) to document different implementations, some of the results are now in https://lem-project.github.io/lem-page/development/implementations-details/

@Sasanidas
Copy link
Member Author

OK, seems like the CCL display bug was because in the micros backend, the function-name implementation was returning a symbol instead of a string, to ensure that this also works with other implementations, I format the function name to a string 👍

@Sasanidas
Copy link
Member Author

At this point, CCL works as well as SBCL (except for lsp, but that's because of the sb-concurrency library usage, which I think it's a problem for other issue 👍 )

@Sasanidas
Copy link
Member Author

Sasanidas commented Jun 22, 2023

I think what is left in this issue is to test both ECL and Clasp, upload the results and create a few specific issues for implementation (here we can also use github labels), let me know what do you think.

@cxxxr
Copy link
Member

cxxxr commented Jun 22, 2023

Yes, it might be a good idea to merge at a reasonable point for now, and separate the pull requests.

@Sasanidas
Copy link
Member Author

Yes, it might be a good idea to merge at a reasonable point for now, and separate the pull requests.

Great! I'll ping you when the test are ready 👍

@Sasanidas
Copy link
Member Author

Sasanidas commented Jun 22, 2023

OK, so I have to tweak a little bit the script to make it work with Clasp, but I think this issue is ready to merge 👍 (ping @cxxxr )

@Sasanidas Sasanidas marked this pull request as ready for review June 22, 2023 14:53
Copy link
Member

@cxxxr cxxxr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very attractive change!
I am very thankful.

I just need a few more corrections 🙏

src/system.lisp Outdated
@@ -5,7 +5,7 @@
(defun lem-relative-pathname (pathname)
(when *deployed*
(truename (merge-pathnames pathname
(uiop:pathname-directory-pathname (first sb-ext:*posix-argv*))))))
(uiop:pathname-directory-pathname "/home")))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this environment-dependent?

Comment on lines +10 to +13
#+sbcl
(with-output-to-string (stream)
(uiop:run-program "git rev-parse --short HEAD"
:output stream)))))))
:output stream))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the run-program portable?

(start-timer-thread timer ms repeat-p)
timer)

(defmethod stop-timer ((timer timer))
(stop-timer-thread timer))

(defun start-timer-thread (timer ms repeat-p)
(let ((stop-mailbox (sb-concurrency:make-mailbox))
(let ((stop-mailbox (lem-mailbox::make-mailbox))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you export symbols for mailboxes?

@Sasanidas
Copy link
Member Author

Let me know if there's anything more to change 👍 (@cxxxr )

@cxxxr
Copy link
Member

cxxxr commented Jun 23, 2023

Thank you very much.
I think it's very good.
I appreciate your efforts over the past month.

@cxxxr cxxxr merged commit dc568f3 into lem-project:main Jun 23, 2023
1 check passed
@cxxxr cxxxr mentioned this pull request Jul 11, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants