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

Add alternative inferior process management #1182

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Sasanidas
Copy link
Member

@Sasanidas Sasanidas commented Dec 9, 2023

Lem uses the library https://github.com/lem-project/async-process for managing inferior processes, which I think its really good and it accomplish his purpose. Still, I think for Linux/Unix users there maybe different alternative that can provide some value to some specific modes.

In this PR I propose to use the underlying library of https://github.com/sionescu/iolib which add an abstraction layers for input/output of different process.

With that idea in mind, I build https://github.com/Sasanidas/ip-management , which created an abstraction to interact with the child process, the API is heavily inspire by the async-process library (given that both are meant to be use for Lem), but it can also be used for other tools.

As an example, I changed the code for elixir-mode to test the change, this is the result:
async-process version:

(admin edit: this version is not able to print colours nor to handle multi-line inputs)

async_elixir.mp4

ip-management version:

ip_elixir.mp4

Given that his library only works for Unix systems and async-process still works well, I don't see it as a change one for the other but as having more options when building a run-* mode.

I think would be also interesting to only load it when the system is UNIX, as it does require the libposix library https://packages.debian.org/sid/libfixposix-dev (which is why the CI is failing).

@Sasanidas Sasanidas requested a review from cxxxr December 9, 2023 15:59
@Sasanidas Sasanidas self-assigned this Dec 11, 2023
@Sasanidas
Copy link
Member Author

I got some feedback regarding the inclusion of a new external dependency (the libposix library) and the overlap functionality of ip-management.

So, for the fist one, I think it's a problem that Lem hasn't solve yet and that is shows with the amount of issues that we have regarding problems on the installation. So I think when we make the installation process more robust and reproducible, we can include this library without that much problem.

As for the second concern, I think ip-management can be use when unix/linux is present as a more complete alternative to async-process, which will help the Lem team to focus on CL not on some obscure C problems (lem-project/async-process#20).

Another clear benefit is that we can forget about taking care of color codes (https://github.com/lem-project/lem/blob/main/extensions/lisp-mode/repl.lisp#L411), and just apply the font-color like any other major-mode.

@Sasanidas
Copy link
Member Author

Other idea that I think it maybe an interesting middle ground solution: having a flag when compiling lem to include ip-management and that flag can add a *feature* so certain internal packages can check if ip-management is present.

@Sasanidas
Copy link
Member Author

I'll try this last approach, so people can chose if they want to use that library or otherwise just use the default lem-process

@rpx99
Copy link

rpx99 commented Dec 12, 2023

Wonder if this helps with #1178

@Sasanidas
Copy link
Member Author

Wonder if this helps with #1178

Probably not, I don't think that issue is related to how the underlying process is managed, still, this issue doesn't want to replace lem-process but add another alternative to it.

@Sasanidas
Copy link
Member Author

So, in order to just add new functionality without disturbing the current one that it's in place. I'll add the file features.lisp which we can use to add keywords to *features* from the terminal.

It works by taking the value of the terminal variable $LISPFEATURES, so if someone want to use the iolib for any extension, he can just export the variable like this:

export LISPFEATURES="(:ip-management)"

And then, compile lem as usual

make ncurses

@Sasanidas
Copy link
Member Author

This would be something similar to what GNU Emacs have with functionality that is still no in the core, but developers/power users can use to modify the end executable (https://github.com/raxod502/emacs/blob/master/INSTALL#L287)

But using the CL way of doing it 👍

@Sasanidas Sasanidas changed the title Add alternative inferior process management. Add alternative inferior process management Dec 16, 2023
@Sasanidas
Copy link
Member Author

As for feature/functionality, I think this is ready to be merged (ping @cxxxr )

@cxxxr
Copy link
Member

cxxxr commented Dec 18, 2023

Sorry, this is not my cup of tea.

  • I could not see the benefit of separating it into a library called ip-management.
  • New dependencies on iolib have been added, and users will need to use apt or other means to install new libraries.
  • The features.lisp method is a bit dirty.

@Sasanidas
Copy link
Member Author

Sasanidas commented Dec 18, 2023

Makes totally sense to have those concerns I hope to dissipate the doubts with these clarifications:

I could not see the benefit of separating it into a library called ip-management.

So, the benefit of having other library is not for Lem, it's for t he CL community, so the library is not bound to the Lem project and can be used for other users. Similar to what async-process does now that is separated.

New dependencies on iolib have been added, and users will need to use apt or other means to install new libraries.

Make sense, I also don't want to introduce another dependency, that's why by default, this feature will not affect Lem, nor does it have to install the libfixposix library in the system. Only when the keyword is added to *features* will the user need to install this library. Think about it like an alternative for advance users.

The features.lisp method is a bit dirty.

I think it's a simple way to add new functionality, but I'm open to other methods more interesting to implement the same behaviors. I'm open to suggestions 👍

@Sasanidas
Copy link
Member Author

I removed the line from the CI to install the libposix library, as you can see, it's not needed for Lem to pass the compilation/lint/tests

@Sasanidas
Copy link
Member Author

I'll continue to develop this branch while I wait for Sasaki-san feedback 👍

@jonBoone
Copy link

jonBoone commented Jan 2, 2024

As an example, I changed the code for elixir-mode to test the change, this is the result: async-process version:

(admin edit: this version is not able to print colours nor to handle multi-line inputs)

Can you expound on what "not able to print colours" means? I see coloured output in the video...

@jonBoone
Copy link

jonBoone commented Jan 2, 2024

I think it's a simple way to add new functionality, but I'm open to other methods more interesting to implement the same behaviors. I'm open to suggestions 👍

Does this issue only happen on *nix systems (presumably, including WSL)?

@Sasanidas
Copy link
Member Author

Can you expound on what "not able to print colours" means? I see coloured output in the video...

What I mean is that Lem has to manually filter terminal colors to the colors of the Lem theme (https://github.com/lem-project/lem/blob/main/extensions/lisp-mode/repl.lisp#L411), also, some other color that you see in the video are because the run-elixir mode inherits from elixir-mode so it does have some syntax highlight.

@Sasanidas
Copy link
Member Author

Does this issue only happen on *nix systems (presumably, including WSL)?

The problem with this PR, is that the functionality that it's adding, doesn't work on Windows, so I didn't want to include it by default but rather as an alternative to the current one that it does work on it.

@jonBoone
Copy link

jonBoone commented Jan 2, 2024

Can you expound on what "not able to print colours" means? I see coloured output in the video...

What I mean is that Lem has to manually filter terminal colors to the colors of the Lem theme (https://github.com/lem-project/lem/blob/main/extensions/lisp-mode/repl.lisp#L411), also, some other color that you see in the video are because the run-elixir mode inherits from elixir-mode so it does have some syntax highlight.

I see -- thank you for that information.

@jonBoone
Copy link

jonBoone commented Jan 2, 2024

Does this issue only happen on *nix systems (presumably, including WSL)?

The problem with this PR, is that the functionality that it's adding, doesn't work on Windows, so I didn't want to include it by default but rather as an alternative to the current one that it does work on it.

Is there some way to provide the same functionality by adding it to async-process? That way it could be made to work on Windows as well.

@Sasanidas
Copy link
Member Author

Is there some way to provide the same functionality by adding it to async-process? That way it could be made to work on Windows as well.

I'm not sure, I don't use Windows so I wouldn't know how to make it work for it, sorry about that.

@jonBoone
Copy link

jonBoone commented Jan 3, 2024

Is there some way to provide the same functionality by adding it to async-process? That way it could be made to work on Windows as well.

I'm not sure, I don't use Windows so I wouldn't know how to make it work for it, sorry about that.

It seems like the main value ip-management is providing in this case is contained in process-get-last-output. Would it be possible to implement this functionality in async-process?

@Sasanidas
Copy link
Member Author

It seems like the main value ip-management is providing in this case is contained in process-get-last-output. Would it be possible to implement this functionality in async-process?

It's not the only value that ip-management provides, still, I wasn't able to make it work the way I think work best for Lem, so I opted for this approach instead.

@jonBoone
Copy link

jonBoone commented Jan 3, 2024

It seems like the main value ip-management is providing in this case is contained in process-get-last-output. Would it be possible to implement this functionality in async-process?

It's not the only value that ip-management provides,

Can you elaborate on what other functionality is needed? From the two issues you mentioned (colors and multiline input), I’m not seeing how any of the iolib functionality addresses the color issue. Can you help me out on that?

As for ip-management, what functionality beyond multi line input is being provided?

still, I wasn't able to make it work the way I think work best for Lem, so I opted for this approach instead.

Can you elaborate on this some more? It seems that this approach is not gaining traction, so if we could understand more of the context we might find a different approach that can gain traction..

@Sasanidas
Copy link
Member Author

Can you elaborate on what other functionality is needed? From the two issues you mentioned (colors and multiline input), I’m not seeing how any of the iolib functionality addresses the color issue. Can you help me out on that?

As you can see, the iolib take care of that al doesn't show any color code on the output of the process, it's an abstraction that Lem doesn't have to take care of.

As for ip-management, what functionality beyond multi line input is being provided?

A separate library, similar to async-process but with (I think) more clear interface and with a underlying library that take care of C, so we don't have to do things like this lem-project/async-process#20

Can you elaborate on this some more? It seems that this approach is not gaining traction, so if we could understand more of the context we might find a different approach that can gain traction..

If by "no gaining traction" is that Sasaki-san didn't reply, indeed is not gaining traction. If you want to implement another approach you are more than welcome to do it. I don't use Windows so I cannot help with that and I think (of course) this a good approach to solve this issue for *nix users.

@jonBoone
Copy link

jonBoone commented Jan 3, 2024

Can you elaborate on what other functionality is needed? From the two issues you mentioned (colors and multiline input), I’m not seeing how any of the iolib functionality addresses the color issue. Can you help me out on that?

As you can see, the iolib take care of that al doesn't show any color code on the output of the process, it's an abstraction that Lem doesn't have to take care of.

So, the design decision for lem was that all input from an inferior process would be stripped of color code characters? It surprises me that this would be the case since most other editors seem to respect the shell theme you have configured in their shell buffers. Is the reasoning for this documented anywhere?

Also, can you point me to the place in iolib that strips these control sequences out?

As for ip-management, what functionality beyond multi line input is being provided?

A separate library, similar to async-process but with (I think) more clear interface and with a underlying library that take care of C, so we don't have to do things like this lem-project/async-process#20

Does the project have a set of guiding principles for making these kind of choices? Have separate libraries to provide functionality seems like an anti-pattern. A common library with a consistent interface, backed by different implementations and (where necessary) platform-specific dependencies, seems like the preferable choice absent any guiding principles.

My perspective, as a potential user, would be that as much of lem as possible be implemented in CL as the first approach, resorting to non-CL when necessary for platform specific functionality or when optimization is desired. A slower implementation in CL is likely more portable across platforms (both current, retro and not-yet-extant), so lem would be more easily leveraged the more it avoids non-CL dependencies.

Can you elaborate on this some more? It seems that this approach is not gaining traction, so if we could understand more of the context we might find a different approach that can gain traction..

If by "no gaining traction" is that Sasaki-san didn't reply, indeed is not gaining traction. If you want to implement another approach you are more than welcome to do it. I don't use Windows so I cannot help with that and I think (of course) this a good approach to solve this issue for *nix users.

I'm not trying to pressure you to implement anything - nor would I expect you to have the resources to test such an implementation on your own. As a collaborative community, however, we might well be able to perform such testing together and the more platforms our functionality is supported across, the broader the appeal of lem in general.

@Sasanidas
Copy link
Member Author

I agree to some of the points of (#1182 (comment)), I also think that a native CL implementation is more desirable, but I also think that that's not possible as C is the "glue language" of this modern world. So either us or a 3rd party library would need C at some point.

My approach of using iolib (with itself uses https://github.com/sionescu/libfixposix , which is a well establish library) delegate that abstraction to this well maintain library. Lem already uses a lot of 3rd party libraries when it makes sense and I don't think it's an anti-pattern (https://github.com/lem-project/lem/blob/main/lem.asd#L19).

To wrap it up, I think this solution it's not a full replacement of the current one, because of Windows users, but it does greatly improve the functionality when using *nix.

@jonBoone
Copy link

jonBoone commented Jan 3, 2024

Eliding the prior portion, as the discussion of those points are not proving fruitful.

To wrap it up, I think this solution it's not a full replacement of the current one, because of Windows users, but it does greatly improve the functionality when using *nix.

I’m still not understanding how this improves functionality for *nix other than a) the dubious choice of meddling with the returned output stream from an inferior process to strip control sequences and b) accept multi line responses from the inferior process.

neither of these seem to require C interface nor do they seem specific to *nix as a collection of platforms…

Absent further information, I guess I’m out of things to ask…

@Sasanidas
Copy link
Member Author

neither of these seem to require C interface nor do they seem specific to *nix as a collection of platforms…

Read the code of async-process or the requirements for iolib, both have C on the backend to deal with different process.

I'm sorry if I'm not able to convince you, but I think I explained good enough what the library does, the purpose of these changes and not only that, but I provide the code to test the improved functionality. Suggestion are great, but without code, it doesn't solve any problem (sorry if it sounds rude, it's not the intention of the phrase).

@jonBoone
Copy link

jonBoone commented Jan 3, 2024

neither of these seem to require C interface nor do they seem specific to *nix as a collection of platforms…

Read the code of async-process or the requirements for iolib, both have C on the backend to deal with different process.

I'm sorry if I'm not able to convince you, but I think I explained good enough what the library does, the purpose of these changes and not only that, but I provide the code to test the improved functionality. Suggestion are great, but without code, it doesn't solve any problem (sorry if it sounds rude, it's not the intention of the phrase).

I'm happy to try your code, but I can't quite figure out how to force iolib to find the lfp library components in the /opt/homebrew/ tree....

@Sasanidas
Copy link
Member Author

I'm not interesting in improving the current async-process C implementation, I tried at the beginning and wasn't that happy working with the C code base (also I cannot test it on Window platform so my changes would not be that good).

The way this is going, I propose these possible solutions:

  • Integrate the full library on Lem, making it the default way for unix systems and using the async-process version for non-unix ones.

  • Using this separation, but with a different system to have it optionally compiled (as you don't like the features approach).

  • You are not interested in these improvements done this way, I'll close the issue and implement an external package for the people that are interested.

I'm open to other solutions of course, what do you think @cxxxr ?

@q3cpma
Copy link

q3cpma commented Apr 25, 2024

Hello and sorry for hijacking this PR comment section, but since there's some useful talk here...

I'm maybe interested in improving async-process, but I have some questions:

  • Why do we need to create a tty in the first place? Why can't we just posix_spawn or uiop:launch-program the process? EDIT: yeah, didn't see that this must work with interactive REPLs. Still, there should be a simpler code path for non-interactive cases.
  • We want to strip all ECMA-48 from the output in all cases, right? Such a filter should be optional and specified on a per-command basis, since it may be costly on throughput. Also, most tools wouldn't output any such sequences if run without the aforementioned tty or with a terminal not advertising any capabilities (e.g. TERM=dumb).
  • Using https://learn.microsoft.com/en-us/windows/win32/procthread/job-objects on Windows to avoid lingering processes if lem crashes might be a good idea.
  • I don't see any throughput benchmark/comparison in this PR, but I think it's very important to compare such a thing, especially since the process functionality is the basis for the pretty verbose LSP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants