-
Notifications
You must be signed in to change notification settings - Fork 139
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
Revamp terminal output (progress bar, callback and stdout/stderr) #1132
Conversation
Now
|
Indeed this fixed the issue mentioned. |
Is it true that only "parms" command suffers this issue? I was expecting changes across programmers based on your comments here. |
Yes, it is |
Using this PR to put code in to periodically call a programmer function so bootloader WDTs can be reset. Note that the bootloader programmers still need to provide their "WDT reset" code, but the terminal side is finished. Made use of @mariusgreuel's Windows port for readline callback functions (see Issue #1028 and his repo https://github.com/mariusgreuel/termreadline). This works under Linux and should work under Windows and MacOS, too, as long as you HAVE_LIBREADLINE. Would be great if you could test in all other OS whether terminal still works with this PR, both interactively, and with input redirection from file and pipes. Right now there is a readline incompatibility for MacOS:
Wrong library? |
As long as it's only related to terminal mode operation, I think it's fine to use stdout. |
That's exactly how this PR behaves. The terminal operations are printed on stdout (and can be used in a pipeline); errors, warnings etc are on stderr as always. |
…) call in Windows
OK, now that the readline() calbacks compile for MacOS, Windows and Un*xes, this PR needs just checking whether the terminal still works in either OS. Please try with your favourite programmer and part:
You can recognise it's the readline library if arrow up/down gets your back and forth in the command line history. Known bug (reported by @mariusgreuel) that under Windows redirected input needs to end with If all other things work, I'd merge at this point (and get windows EOF reading from pipe/file fixed another time). The big benefit of this PR is that the programmer can do things concurrently whilst the user types (requires the readline() callbacks). This allows us to write code to keep bootloaders from time out. |
Strange, even though github action has no issues building under MSYS2 mingw64, I have issues with missing reference to Edit to add: even after I fully updated the MSYS2 installation, the issue remains. Strange.
|
I am using Windows terminal and actually I do not see much differences between this one and git_main. I can already use arrow up/down with git main. As for EOF, indeed But under Windows, even without libreadline, we can use And there is a anomaly under Windows with Reference: git main.
PR 1132 with libreadline (dynamic linking so that it will work).
|
I am thinking that Edit: this is not correct.
|
I figured out what was the problem. It is not It is always very tricky to use static linking for mingw gcc but that is the default for avrdude Windows. Ref:
Static linking will fail.
|
Looking at the CMake artifacts, github action does not use libreadline at all. My MSYS2 mingw64 system has libreadline installed and that is the reason for failing. I have no issues to build with VS2019 as it does not use libreadline either. From the CMake artifacts for mingw64.
|
I tried on another Windows machine with libreadline installed for MSYS2 mingw64, and it has the same issue. I can not remove it right now as it is needed by many other packages. That is the issue with pacman (similar thing for Arch Linux) where the library and development package are within the same package, unlike Debian/Ubuntu or Fedora.
|
All in all it is not a real issue for this PR. It is just that readline is still a problem for Windows (and MSYS2 mingw64 libreadline or libtermcap dynamic linking is broken). We can probably fix that with the discussion here. I can still easily work around this issue by using dynamic linking for libreadline. |
@mcuee Yes, the gitlab runners work for MacOS; looking at the build report evidence they found libreadline, use it and it compiles. Would be good if standard MacOS installation would work as well. Looking at your MacOS installation problems, it seems to be the same as msys2/MINGW-packages#8105 Maybe there is a solution hidden in this discussion? @mariusgreuel has made readline work in Windows, and I used his code from https://github.com/mariusgreuel/termreadline
This may be down to |
@stefanrueger CTRL-D does not work either with libedit. github action binary (x86_64) version has the same issue as it also uses libedit. BTW, no issues with git main, history using arrow keys works fine. CTRL-D works fine as well.
|
@mariusgreuel If I force link readline under macOS, it can be found using pkg-config.
|
@stefanrueger Edit to add: We still need to fix avrdude side to add the termcap library. Right now I need to manually add it after the failure. That may require the help from @mariusgreuel again on the CMake side (find out readline and maybe termcap). Not so sure if this helps or not.
This will produce the static linking version of avrude.exe. I can also use
|
The result libreadline based avrdude works fine. CTRL-D works as well.
It also works much better for my official Arduino Nano Every with buggy jtag2updi firmware in terms of progress bar, compared to git main. You can see the commits are working as expected.
Take note due to the padding size, it will still print out the progress bar, but then it will diable the progress bar because of the error. In order to show the real results, I will not modify any outputs -- so you need to scroll to the right to see the command.
|
In the end, I think it is a good pull request and we need to fix the issue with macOS. We will need CMake experts like @mariusgreuel to help out in this case. Windows is actually not too bad (MSVC build works, MSYS2 build works if people use minimum MSYS2 installation just for avrdude build, like github action build). Both will not use readline though. We can always wait for @mariusgreuel to help on the MSVC side. The MSYS2 issue with libreadline looks like a MSYS2 packaging issue of either readline or termcap. |
I figured out why macOS is using libedit in the end. It automatically pull in the SDK library
And apparently that libreadline.tbd is actually libedit.
Manually replacing the SDK readline with homebrew readline ( /opt/homebrew/lib/libreadline.dylib) and it will be okay. The libreadline version works well and CTRL-D works fine as EOF or
|
From the following it seems simple to force macOS to use libreadline and not libedit, unfortunately it does not work out of the box. I tried the following build options and none of them worked.
|
@stefanrueger and @mariusgreuel I have finally figured out a simple way to fix for macOS. There is no need to change CMake files. This works fine.
|
@stefanrueger |
From my tests: Looks like only the github runners for linux-x86_64 and macos-x86_64 Linux actually define HAVE_LIBREADLINE. The others
do not set HAVE_LIBREADLINE for the CI tests. In particular, @mariusgreuel's |
I believe the following will for sure not have libreadline since the library is not installed by github action
MSYS2 mingw anyway has problems with libreadline build as of now due to the packaging issues with termcap. Need to check why the following does not build with readline.
|
@stefanrueger and @mariusgreuel
@stefanrueger and @mariusgreuel |
You can try to add libreadline-dev in the github actions. It should work for Linux build now.
|
@stefanrueger Edit to add: this does not work. github action will fail. Need to study further. |
I tested in my avrdude fork and created a pull request #1146. for the three Linux build. Please review. |
Pull request #1148 created for MSYS2 ming32/mingw64. Interestingly it works in my avrdude fork test. Maybe my MSYS2 installation needs a revamp. |
Finally figured out the way for macOS, I have created pull request #1149 to add real readline (and not libedit) for macOS. |
In the end it is not my MSYS2 installation problem. Looking at the failed build log (https://github.com/avrdudes/avrdude/actions/runs/3308684165/jobs/5461248027), the issue is still the MSYS2 readline/termcap packaging issues mentioned here. On my local machine I can fix the issue by rebuilding termcap as per the tips from MSYS2 github issue. I'll see what I can do to correct this issue using github action. |
I believe this works but I may have messed up the git thingy. I have to create a new PR. |
For those who want to test readline and static linking mingw32 Windows version of avrdude git main, you can try the binary mentioned here. I did some simple test and it works well. |
Supposed to fix Issue #1130