refactor: comint daemon to jsonrpc

[elken]

Change the hacky comint implementation to a more stable and robust
JSONRPC implementation.

A process is managed by emacs and commands are sent/recieved
asynchronously to that process.

Currently there still exists a couple of bugs I'm struggling to nail down
(mostly because I'm unfamiliar with the hash-table/dash combination):

• On startup, sometimes it errors because it sends the two startup messages at
  once. This should easily be rectified with a listp check when parsing
• Debugging emulators on linux fails, and I'm struggling to work out exactly
  why. I suspect something isn't being set correctly maybe in one of the
  listeners
  • However , debugging regular linux works fine and seems faster and more stable

Apologies for opening two PRs for the same thing, when I first tried getting
this working every request timed out so I looked into the tcp implementation.
This supersedes that now as it's much more simple.

Obseletes #164

[ericdallo]

@elken sorry for the delay, could you fix conflicts and the CI ?

[elken]

Apologies, I haven't used flutter in ages so these changes have been forgotten.

I will make time to resolve these this week

[elken]

Apparently time makes fools of us all! 😅

I'm looking at this as we speak (will try and resolve the merge conflicts too)

[elken]

@ericdallo I'm not familiar with eask but it seems to be complaining about missing packages and autoloads now (missing packages might be https://github.com/emacs-eask/eask/issues/11)

[elken]

Well I can't replicate the CI errors...

Have tried with freshly built emacs 27 and it runs everything fine

[nidyaonur]

After seeing your last comment, I have reinstalled emacs freshly as well, and I am getting the same error. I can assist you in the process if you need someone for that. @elken

[elken]

Sure! If you figure them out do let me know, I'll try and pick it up as I have spare time

[nidyaonur]

I misread the CI part 😅. If you could instruct me how to use your branch on my local machine, I can try it out. I am not really good at these:/

[elken]

I misread the CI part 😅. If you could instruct me how to use your branch on my local machine, I can try it out. I am not really good at these:/

Well that's the thing, it works fine when I run it in isolation locally. Maybe see if act breaks

[ericdallo]

@jcs090218 any thoughts about CI failing?

[elken]

Cool this looks like it might resolve the CI errors.

This isn't ready to go in yet though, and I am starting to be annoyed enough to resolve this so I'll try and have the issues resolved this week ish

EDIT: Did a quick google of the windows snapshot failure, it's an emacs 29 function so best guess would be it's pinned to a commit that doesn't have it?

[jcs090218]

There is an issue is from Emacs windows snapshot, see https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-07/msg01656.html. We could do the following:

1. ignore it, since it's a snapshot
2. Add experimental flag, see example code in https://github.com/jcs-emacs/jcs-emacs/blob/master/.github/workflows/startup.yml#L26-L45. (this will turn the badge from red to green, but the icon in action tab will remain the same, red ❌)

[elken]

There is an issue is from Emacs windows snapshot, see https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-07/msg01656.html. We could do the following:

1. ignore it, since it's a snapshot
2. Add experimental flag, see example code in https://github.com/jcs-emacs/jcs-emacs/blob/master/.github/workflows/startup.yml#L26-L45. (this will turn the badge from red to green, but the icon in action tab will remain the same, red ❌)

I like the experimental version the most, me personally I don't consider Emacs 29 as anything as there's not even a branch cut. So I'd be fine ignoring it unless it's needed.

[jcs090218]

Oh yes, please ignore 29. I have it there for my own use. Just look at the experimental part of the code.

[elken]

Oh yes, please ignore 29. I have it there for my own use. Just look at the experimental part of the code.

I know what you were referring to, I was agreeing xD

I'll submit those in a different PR though

[ericdallo]

@elken Looks good to me!
I'll give a little bit of more testing and then we can merge it

[elken]

@elken Looks good to me! I'll give a little bit of more testing and then we can merge it

You sure? I was having issues with emulators starting up in a few cases.

It's your codebase so you are obviously much more knowledgable so if you're happy to merge it go for it, I just want to run through some usecases to make sure it works and resolves my issues proper! :D

Glad you're happy with it though, I was worried when I made it that I botched it lol

[ericdallo]

I meant the code review 😅 I still need to manually test it

[ericdallo]

@elken any updates on this? it'd be really nice to have this fixed

[elken]

@elken any updates on this? it'd be really nice to have this fixed

Posted an update on discord but for visibility; I'm leaving the company that uses flutter so my usage of it has gone way down, but I'm keen to try and pick it back up to wrap it up when I can.

EDIT: I have acquired much more free time now, so I will be looking into this this week

[Pacane]

I've tried running this branch with the latest master changes, and here's an error I get when opening a dart buffer, and also when I call lsp-dart-run in my flutter project.

LSP :: Guessed project root is ~/code/dart/test_mono_repo
LSP :: Connected to [dart_analysis_server:48364/starting].
Error processing message (wrong-type-argument stringp ("/Users/joel/.asdf/installs/flutter/3.3.4-stable/bin/flutter")).
make-process--with-editor-process-filter: Wrong type argument: stringp, ("/Users/joel/.asdf/installs/flutter/3.3.4-stable/bin/flutter")

The path of my flutter install is correct, written like this in my config.el file (actually here's the whole lsp-dart config block)

(use-package! lsp-dart
  :config
  (setq lsp-dart-sdk-dir (expand-file-name "~/.asdf/installs/flutter/3.3.4-stable/bin/cache/dart-sdk/")
        lsp-dart-flutter-sdk-dir (expand-file-name "~/.asdf/installs/flutter/3.3.4-stable/")
        lsp-dart-project-root-discovery-strategies '(closest-pubspec lsp-root)
        lsp-dart-flutter-widget-guides nil
        gc-cons-threshold (* 100 1024 1024)
        read-process-output-max (* 1024 1024))
  )

[elken]

I've tried running this branch with the latest master changes, and here's an error I get when opening a dart buffer, and also when I call lsp-dart-run in my flutter project.

LSP :: Guessed project root is ~/code/dart/test_mono_repo
LSP :: Connected to [dart_analysis_server:48364/starting].
Error processing message (wrong-type-argument stringp ("/Users/joel/.asdf/installs/flutter/3.3.4-stable/bin/flutter")).
make-process--with-editor-process-filter: Wrong type argument: stringp, ("/Users/joel/.asdf/installs/flutter/3.3.4-stable/bin/flutter")

The path of my flutter install is correct, written like this in my config.el file (actually here's the whole lsp-dart config block)

(use-package! lsp-dart
  :config
  (setq lsp-dart-sdk-dir (expand-file-name "~/.asdf/installs/flutter/3.3.4-stable/bin/cache/dart-sdk/")
        lsp-dart-flutter-sdk-dir (expand-file-name "~/.asdf/installs/flutter/3.3.4-stable/")
        lsp-dart-project-root-discovery-strategies '(closest-pubspec lsp-root)
        lsp-dart-flutter-widget-guides nil
        gc-cons-threshold (* 100 1024 1024)
        read-process-output-max (* 1024 1024))
  )

A change was recently made to adjust how the dart executable is called, I have time to work on this PR again so I will look into it in the next couple of days

[Pacane]

Just a mention though, I've tried running a dart vm main file with lsp-dart-run and it works fine so far.

[Pacane]

Any update on this? I can provide details/help if needed. I could also provide a PR if somebody directed me to something to look at!

[elken]

Thanks for this ping, I really do hate having a life sometimes 😅

This weekend I should be free so I'll try my very best to pick it up. I tried to pick this up the other day...

In terms of help, I've put the couple of issues I was getting in the initial PR message.

@ericdallo I know lsp-mode is starting to move everything over to plists, is this something I should add in here or is that something you'll look into later? Just remembered the last time I tried it when I had my macbook I was running the plist setup and kept hitting issues

[ericdallo]

@elken Feel free to do whatever it's easiest for you, I can migrate it later, there are some missing points on other places of lsp-dart that I still need to take a look
lsp-dart already has issues with lsp-use-plists, I intend to take a look at this soon

[elken]

@elken Feel free to do whatever it's easiest for you, I can migrate it later, there are some missing points on other places of lsp-dart that I still need to take a look lsp-dart already has issues with lsp-use-plists, I intend to take a look at this soon

In that case I'll leave it then. Worth being aware of though, last I spoke to yyoncho about it it seemed that most of the other clients have implemented it so I imagine the switch will be soon ™️

[ericdallo]

Yep, it's the top lsp-dart priority to me ATM after this PR , I will try to take a look in the next days

[elken]

@ericdallo I can't apologise enough for this taking so long 😅

I've fixed both the issues I mentioned in the original message, so this should be fine to look over now. I appreciate that jsonrpc is a bit of a black box, so I'm happy to provide some accompanying documentation on how things work but as far as I can tell this should be good for testing at least.

I also don't have access to a macbook now, but I might be able to wrangle some people who do; but there's no reason that this shouldn't resolve the original issue that spawned this.

I also know it's a busy night for you, so without derailing this thread good luck!

[ericdallo]

Thank you very much for this @elken!
I reviewed and manually tested and works great, I will merge this and do only a few refactorings on master like removing the message, try to remove ht dep and improve some function's complexity, anyway glad to see this working!

[elken]

Thank you very much for this @elken! I reviewed and manually tested and works great, I will merge this and do only a few refactorings on master like removing the message, try to remove ht dep and improve some function's complexity, anyway glad to see this working!

Thanks! Glad it's working smoothly. I wrote that massive process filter function in a bit of a haze then convinced myself "ah it works for now" 🤣

[ericdallo]

@elken no problem, I'm finishing the refactor on that function, it's not that better than before but it's using some lsp-mode functions for that instead.
The issue is that jsonrpc only accepts plists, and lsp-mode users could be using plists or hash-tables depending of the lsp-use-plists, not a big deal for now though, I should commit the changes soon

[elken]

The issue is that jsonrpc only accepts plists, and lsp-mode users could be using plists or hash-tables depending of the lsp-use-plists, not a big deal for now though, I should commit the changes soon

Yeah, ideally both would have to be supported. Maybe create a frozen no-plists branch when lsp-mode pushes the change? There's probably not a clean way to handle this

[ericdallo]

yeah for now I'll try to leave this complexity in a specific function or so