-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
Conversation
4f81244
to
a494118
Compare
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. |
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 |
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 |
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 |
So, in order to just add new functionality without disturbing the current one that it's in place. I'll add the file It works by taking the value of the terminal variable export LISPFEATURES="(:ip-management)" And then, compile lem as usual make ncurses |
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 👍 |
As for feature/functionality, I think this is ready to be merged (ping @cxxxr ) |
Sorry, this is not my cup of tea.
|
Makes totally sense to have those concerns I hope to dissipate the doubts with these clarifications:
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
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
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 👍 |
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 |
I'll continue to develop this branch while I wait for Sasaki-san feedback 👍 |
Can you expound on what "not able to print colours" means? I see coloured output in the video... |
Does this issue only happen on *nix systems (presumably, including WSL)? |
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 |
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. |
I see -- thank you for that information. |
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 |
It's not the only value that |
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 As for
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.. |
As you can see, the
A separate library, similar to
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. |
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
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.
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. |
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. |
Eliding the prior portion, as the discussion of those points are not proving fruitful.
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… |
Read the code of 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 |
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:
I'm open to other solutions of course, what do you think @cxxxr ? |
Hello and sorry for hijacking this PR comment section, but since there's some useful talk here... I'm maybe interested in improving
|
I'll move this functionality as external extension for Lem https://github.com/lem-project-archive/ip-run |
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).