Consider libreadline/libedit only in interactive mode #1207
Conversation
There's not much point in running the entire terminal mode session on top of libreadline or libedit, unless it is clear the session is really running interactively on a terminal. Thus, only use the libreadline/libedit version if stdin points to a terminal. Otherwise, resort to the simple version without readline support. That ensures everything works well when running it within a pipeline.
|
This is great. Tested okay under macOS Ventura 13.0.1 (Apple Mac Mini M1 2020) with an Arduino Uno clone using |
|
I also tested under Windows 11 with an Arduino Uno clone using MSYS2 mingw64. There is no regression here. Both line editing and keepalive feature are working with |
|
That's a clever fix! Works great on my mac. |
|
I have done the Wiki updates as I think this PR can be merged. Please take a look if my update is good or not. Thanks. |
|
Edge cases could be for pipeline input that is slow (think a process that prepares the input for avrdude does a lot of sleeping) when terminal mode talks to a bootloader that might time out, but I actually don't know whether |
|
I agree this is an edge case, but I think at some point we have to draw a line. If someone is talking to a bootloader that employs timeouts, it's their responsibility to be fast enough to make the bootloader not time out. |
Updated. Yes it will be good to add the clean and install targets. Linux and FreeBSD building instruction Wiki page have also been updated to use |
Always cool. I personally copy the following Makefile into the avrdude directory after cloning and then work with that I guess that's Uni*x specific but keeps me for having to type long cmake lines all the time during development. |
|
| edge case
I predict above test will
| If someone is talking to a bootloader that employs timeouts, it's their responsibility to be fast enough Correct, but it is actually AVRDUDE that talks to the bootloader! | at some point we have to draw a line Correct.
|
Indeed. It works until It has even the same behavior for MSVC build without Readline. This PR: git main: MSVC build. |
|
@stefanrueger and @dl8dtl This PR fails the test under Linux (GNU Readline), and behavior is the same as Windows MSYS2 (with GNU Readline). git main works fine. |
What you say is correct about git main (piping is not working under macOS with libedit). With this PR, they all behave the same in this aspect -- all will fail the tests. So this PR brings equality between libedit and GNU Readline under macOS, and brings eaulity to Windows GNU Readline build as well. For the paticular tests, even Windows MSVC build without GNU Readline behaves the same.
So I guess you are still fine with this PR, right? Or do you want to treat Linux/FreeBSD a bit differently and unconditionally use GNU Readline? I am okay either way. BTW, I am OS neutral. :) I like cross-platform projects and my engagements with the open source world are almost all cross-platform projects. |
This PR brings equality of libedit and GNU Readline under macOS, both behave the same and both will fail the tests. Without this PR, you are right. |
|
Ah, of course! This test works without the PR on Linux but is bound to fail under Linux with this PR. My bad. So, there is a regression here. The PR is actually only needed for Mac when compiled with libedit. So, I suggest to replace Then
|
|
| equality between libedit and GNU Readline under macOS What we want is justice though :) https://www.pnwu.edu/files/2021/05/EEJ.jpeg |
Haha, nice picture. I am okay with your proposal and indeed there is a regression here under Linux. |
|
I tried to use the same Linux code path for MinGW but then terminal mode does not work at all with GNU Readline. On the other hand, cygwin uses the same code path as Linux (without any changes) and it works fine out of box. I have updated Wiki for Cygwin to add GNU Readline dependancy. |
Keep in mind that libedit originates from NetBSD. We just don't test this system very much. |
|
So since this is not an OS issue per se (someone could install GNU libreadline on MacOS as well as on NetBSD), I'd prefer to not drag in any OS dependency, but depend it on a proper detection whether the readline facility is GNU readline or not. |
I have not figured how to build avrdude under NetBSD yet. I can test avrdude under NetBSD once I figure it out. As of now the binary is not pointing to the right library. I am not familiar with pkgsrc myself. |
That will be the best. Unfortunately we need some CMake magic in order to do that. Hopefully @mariusgreuel can figure that out when he got the time. |
I have figured out how to build avrdude under NetBSD properly. We just need to replace But I have not figured how to get my Arduino Uno USB to Serial port to appear under NetBSD VM yet. Ref:
|
|
In any case, official avrdude 6.4 under NetBSD uses GNU Readline. So indeed @stefanrueger is correct that only macOS is using libedit by default for the purpose of avrdude. https://github.com/NetBSD/pkgsrc/blob/trunk/cross/avrdude/Makefile
|
|
| since this is not an OS issue per se The issue is that our make-cmake-build-configure-amake-boostrap-idontknowwhat detects MacOS's |
|
Well, since libedit offers a libreadline compat layer, it's not quite surprising that the readline test also triggers. I'm not sure what the respective tests could perform at configure time, but one possible distinction is the variable |
|
From the following test results, it seems this PR is actually good for the Readline-win32 which can be used with MSVC. The |
|
BTW, I found a port libedit for Windows called WinEditline. Unfortunately it does not support the |
|
Latest version of libedit (20221030) seems to perform better under Linux than the older version in Ubuntu 20.04. Ubuntu 20.04 old version (20191231). |
I tried your patch under Linux with GNU Readline and it works fine. (As mentioned before, it does not work with libedit under Linux but we would not really cate for libedit under Linux). BTW, I feel there is a missing return for |
|
I actully figured out a rather hacky way to use CMake to differentiate libedit vs GNU Readline, as macOS detects the symbolic link So I think @dl8dtl's idea to detect at runtime is much better. |
|
@stefanrueger |
|
For you patch to work under macOS with GNU Readline, apparently we need to change CMake scripts. |
|
@dl8dtl git main (default with GNU Readline) -- everything works fine
|
Doesn't surprise me – I've been testing their library under my FreeBSD. |
Great. So we can say the BSDs are similar in this aspect as macOS if using libedit. Even libedit latest version performs similar under Ununtu 20.04 Linux. Only the old libedit versio from Ubutnu 20.04 is a bit strange. |
|
Same for OpenBSD.
|
GNU libreadline does not seem to have any issues when running on a pipe as input, thus use it all the time. EditLine (NetBSD, MacOS) has issues with that, so only use it when running interactively on a terminal.
@dl8dtl Have you pushed a runtime check in this PR? Doesn't look like it. Let me suggest replacing This runs the callback readline input (what you call interactive) when it's a real terminal or when AVRDUDE was seemingly compiled using GNU Using So I prefer that additional runtime test. In a slight complication that additional test might not improve things under Windows compiled with Do you want to check and change, @dl8dtl, or should I push the change onto your PR? |
|
@mcuee thanks for brilliant testing here and sharing your insights as to the matrix of OS/library variants |
|
@dl8dtl Should that not be the other way round? When it's GNU libreadline, strstr() will not find the string, and return NULL, in which case to run term_interactive (even when not a terminal). |
|
... also i just checked the spellings mentioned in this conversation; three are mentioned: |
|
Indeed you are right, the latest version does not work under Linux with GNU Readline. And your modification works fine. It can cope with long sleep when building with GNU Readline. And timeout will happen when building with libedit. I will test macOS later. |
|
The modification is also okay for macOS. BTW, I have also tested with Windows, it will still time out as mentioned by @stefanrueger, but at least no regression. |
Probably. It was too late last night for fixing it, I only noticed it didn't work correctly, but then went to bed. -) |
I think this was partially my fault, partially typing sloppiness. Just another idea: all editline wrappers I find use numeric version 0x0402. Maybe GNU readline was about the same level featurewise with that old version, and we could just compare |
Yes that should be the correct one based on what I have seen.
I think this will work. I looked at the history here, only the initial version (2.11-20080614-6 does not have that rl_readline_version. All other versions after that is okay. |
|
Indeed the version check works fine under Linux. |
Besides of fixing a logico, this allows for a really straightforward decision. All EditLine versions so far use 0x0402 as their version number.
|
OK, I updated the PR. |
There's not much point in running the entire terminal mode session on top of libreadline or libedit, unless it is clear the session is really running interactively on a terminal.
Thus, only use the libreadline/libedit version if stdin points to a terminal. Otherwise, resort to the simple version without readline support. That ensures everything works well when running it within a pipeline.