Bug 264010 - bc no longer has vi-keys history editing
Summary: bc no longer has vi-keys history editing
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 13.0-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: Stefan Eßer
URL:
Keywords: needs-qa
Depends on:
Blocks: 264030
  Show dependency treegraph
 
Reported: 2022-05-15 23:33 UTC by David E. O'Brien
Modified: 2023-09-12 16:20 UTC (History)
6 users (show)

See Also:
koobs: maintainer-feedback? (se)


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David E. O'Brien freebsd_committer freebsd_triage 2022-05-15 23:33:10 UTC
Up until FreeBSD 13.0, bc linked against either libreadline or libedit.  Both of which support "vi keys" in addition to "emacs keys" for history editing.
~/.inputrc: "set editing-mode vi", ~/.editrc: "bind -v"

For those of use that have used their Shell with "vi keys" (tcsh/zsh: 'bindkey -v', sh/ksh/bash: 'set -o vi') it is now much harder to use bc for anything other than the simplest of things (such as: echo "1 + 2" | bc).

For those of us that have used Vi-keys for over 25 years, this bc is a MAJOR regression and has made it very annoying to use.
Comment 1 Stefan Eßer freebsd_committer freebsd_triage 2022-05-16 21:27:52 UTC
The author of the bc and dc built in FreeBSD-13.x by default is considering to implement vi style editing, but it will take some time to develop this functionality.

As a work-around, the bc and dc from FreebSD-12 can be used in 13 and -CURRENT by either building the "world" with WITHOUT_GH_BC defined in src.conf, or by just building and installing both usr.bin/bc and usr.bin/dc.
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2022-05-17 00:45:44 UTC
@Stefan 

- Do we have this behaviour change listed in RELNOTES? If not, might this be worth adding?

- Is there a potential resolution to this in releng/13 (for subsequent releases), or is this a 'not accepted' based on the implied proposal to revert the change, and pending feature development/support in future versions?

- What alternative resolutions are possible, if any, in leui of future functionality/features to resolve the issue as reported in releng/13? Is re-linking to libedit / libreadline possible?
Comment 3 Gavin D. Howard 2022-05-17 03:14:31 UTC
Original author of the new bc here.

I am going to add vi mode and Emacs mode to history editing, regardless of how I have to do it. I will either implement it all by hand or somehow make libedit work. I also intend to get it done before the merge window for 13.2 closes, and hopefully far before so that testing can be done well in advance.

I don't know what that implies for release engineering, but I thought I would let you all know.

Anyone who would like to help me test is welcome to email me because I've never used either mode (intentionally) for either readline or editline. In fact, I didn't know they existed until recently.

Thank you for this report, by the way; I do want my bc to not surprise anybody, and I'm sorry it has been a problem.
Comment 4 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-17 13:41:18 UTC
libedit seems to provide some kind of libreadline compatibility, but I know nothing about it, just saw a few commits over in NetBSD repo mentioning readline.
Comment 5 Gavin D. Howard 2022-05-19 05:55:15 UTC
Good news!

I have successfully integrated both editline and readline into my bc. The code is in the line_libs branch at https://git.yzena.com/gavin/bc (https://git.yzena.com/gavin/bc/src/branch/line_libs if you're lazy).

To build my bc with readline, use the `-r` argument to configure.sh. To build it with editline, use the `-e` argument. Obviously, you cannot do both at the same time.

Please do build and test because there are two problems that I need help with.

First, FreeBSD does not define SIGWINCH (needed for editline) unless __BSD_VISIBLE is non-zero, and __BSD_VISIBLE is only non-zero if not building in a C89, C99, or C11 environment. I don't know how to make that happen without causing another build error because (for some reason) C11 is still active, and it gets confused about static_assert. Can someone help me figure out how to have SIGWINCH while making the build work?

Second, editline appears to not save history, despite my code correctly calling `el_set(<el>, EL_HIST, history, <hist>);`. It might be that I just don't know how vi or Emacs mode work with editline, so I could use some help testing history in editline.
Comment 6 Stefan Eßer freebsd_committer freebsd_triage 2022-05-19 08:22:41 UTC
(In reply to Kubilay Kocak from comment #2)

> - Do we have this behaviour change listed in RELNOTES? If not, might this be worth adding?

I had not been aware aware that the bc imported from OpenBSD had been extended to use libedit, when Gavin's bc was imported into FreeBSD-12 (then -CURRENT). It has not been enabled in 12.x by default (but can be built with WITH_GH_BC in src.conf).

The release notes of FreeBSD-13.0 did mention the new bc/dc implementation and the benefits it provides, but did not mention any changes with regard to command line editing, since I had not been aware of the libedit support in the previous version.

> - Is there a potential resolution to this in releng/13 (for subsequent releases), or is this a 'not accepted' based on the implied proposal to revert the change, and pending feature development/support in future versions?

The new bc/dc has already received a number of enhancements specifically to match traditional FreeBSD behavior and expectations.

Line editing with libedit or readline (compile time option, I'll use the libedit provided in the base system) has been implemented by Gavin and will be made available in the port (lang/gh-bc) after testing (and will be imported into -CURRENT and 13-STABLE, thereafter).

> - What alternative resolutions are possible, if any, in leui of future 
functionality/features to resolve the issue as reported in releng/13? Is re-
linking to libedit / libreadline possible?

The planned update of bc in -CURRENT and 13-STABLE will link against libedit instead of the private editing and history functions in the bc sources.

Users who prefer the old bc can easily build it (make world -DWITHOUT_GH_BC), or they can install the updated port or package (lang/gh-bc) with libedit support, when it has been committed.
Comment 7 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-19 16:16:22 UTC
(In reply to Gavin D. Howard from comment #5)
To save history in libedit, you would need to do something like we did in
https://cgit.freebsd.org/src/commit/?id=d9d0812bc6e
Of course, that does more than is needed for bc, but the commit is still short enough to easily extract the important parts.

I'm looking into other issues with this patch.
Comment 8 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-19 16:54:23 UTC
(In reply to Gavin D. Howard from comment #5)
Your 04ef32e888fc0e562ba21ff0d325c7c77c2322a0 introduces use of f->f in a code path where that doesn't exist.
Comment 9 Gavin D. Howard 2022-05-19 17:29:37 UTC
@pstef, I have fixed the build issue. Sorry about that. That's what happens when I only get small snippets of time and mostly work at night.

About history in libedit, I'm not worried about saving history to a file; I just want history for the current session. Do I still need to do that for history just in the current session?
Comment 10 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-19 19:26:21 UTC
You also need some combination of:

history(h->hist, &he, H_SETSIZE, 100);
history(h->hist, &he, H_SETUNIQUE, 1);
el_set(h->el, EL_EDITOR, "emacs");
around the initialization of history and editline, and

history(h->hist, &he, H_ENTER, line);
somewhere in bc_history_line().
Comment 11 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-19 19:40:23 UTC
I don't think SIGWINCH is Posix yet and the program claims to be Posix-compatible. Dropping -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 and reverting https://git.yzena.com/gavin/bc/commit/f8b5ab0dfdd511d59425b32e7fa7afae3ecd2f1c makes bc build and work for me.

editline(3) says "Programs should be linked with -ledit -ltermcap." although not linking with termcap has immediately apparent effects.
Comment 12 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-19 19:42:10 UTC
(In reply to Piotr Pawel Stefaniak from comment #11)
I meant to write that not linking with termcap has no undesired effects in my case. Probably works by accident.
Comment 13 Gavin D. Howard 2022-05-20 06:40:57 UTC
Wow, okay, the editline documentation is bad. I thought that registering the history function with EL_HIST would be sufficient, and I have no idea why it's not.

Anyway, your fix worked, see https://git.yzena.com/gavin/bc/commit/c74c2a56d47ad , which can be tested by pulling the latest line_libs branch.

About SIGWINCH: I know it's not POSIX, but history is the one part of bc where I do not claim that POSIX is enough. That is why there is the option to disable history to build.

However, FreeBSD seems to be different from other platforms I have personally tested on. Most platforms seem to *enable* stuff when you define _POSIX_C_SOURCE. I know for a fact that glibc does. FreeBSD does the opposite. So on Linux, I have to define it (to get a few functions bc needs), and on FreeBSD, I can't.

I fixed the issue with https://git.yzena.com/gavin/bc/commit/7bbf1eec5982 , a configure test for FreeBSD, and while it's ugly, it works. I had to do the same thing for OpenBSD for different reasons (pledge() and unveil()).

About needing -ltermcap: it appears that that is another case of the editline documentation being wrong. When I use it on Linux, the build errors because it can't find termcap. When I use it on FreeBSD, it works fine, but the `ldd bin/bc` does *not* bring up libtermcap as one of the required libraries. So it appears that it is not needed?

Also, I thought that if shared library A depended on another shared library B, the executable that depends on A does not have to say it depends on B and that the compiler/linker would still pull B in automatically. Correct me if I'm wrong, though.

Anyway, everything seems to be working now. Please test and let me know if there are still problems because I also don't have a full FreeBSD system I can test on.
Comment 14 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-21 08:02:34 UTC
It works for me, thank you.

The only thing I could find is that regardless of bc.sigint_reset, SIGINT results in:
Fatal error: I/O error
    Function: (main)

Also, when quitting with EOF, a newline is not printed - but this is a long-standing bug.
Comment 15 Gavin D. Howard 2022-05-21 16:36:12 UTC
I have pushed more commits that hopefully fix both the EOF bug and the SIGINT bug. I could use some help testing.
Comment 16 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-21 16:53:31 UTC
That fixed the last issues I was aware of, I currently know none.
Comment 17 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-21 18:16:00 UTC
For my own amusement, I configured your bc to use libedit-emulated readline from our base, then made it include <edit/readline/readline.h>, compiled and linked with -ledit -- and it seems to work.

By the way, the readline version seems to lack the inclusion of string.h:
diff --git a/src/history.c b/src/history.c
index 30936c3..e18c087 100644
--- a/src/history.c
+++ b/src/history.c
@@ -275,8 +275,9 @@ end:
 #if BC_ENABLE_READLINE

 #include <assert.h>
 #include <setjmp.h>
+#include <string.h>

 #include <history.h>
 #include <vm.h>
Comment 18 Gavin D. Howard 2022-05-21 18:33:10 UTC
The missing #include is fixed now.

I'm not going to release yet, and I'm not going to do my release process yet either. We managed to complete this far before I thought would happen, and since 13.1 was just released, I think we have time to wait, in order to shake out more bugs.

I'll start the process in a month or so, but I'll keep an eye on the FreeBSD release schedule, and I'll adjust when the 13.2 schedule is released, if necessary.

However, I'm willing to do whatever else is needed to get this in a robust state and to comply with FreeBSD policies.
Comment 19 Piotr Pawel Stefaniak freebsd_committer freebsd_triage 2022-05-21 18:44:58 UTC
I'm not disagreeing, but maybe we could import this as-is into the main branch, to have it tested by a wider audience, before it lands in the stable branches and then release branches.
Comment 20 Gavin D. Howard 2022-05-21 18:54:56 UTC
I have no preference one way or the other and will go with whatever is decided since I am not a FreeBSD regular.
Comment 21 commit-hook freebsd_committer freebsd_triage 2022-06-11 11:16:34 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=78bc019d220e05abb5b12f678f9b4a847019bbcc

commit 78bc019d220e05abb5b12f678f9b4a847019bbcc
Merge: 36447829aee5 bd54318046bf
Author:     Stefan Eßer <se@FreeBSD.org>
AuthorDate: 2022-06-11 11:03:06 +0000
Commit:     Stefan Eßer <se@FreeBSD.org>
CommitDate: 2022-06-11 11:14:37 +0000

    usr.bin/bc: update to version 5.3.1

    This version adds support for command line editing and history using
    the libedit or readline libraries in addition to the line editing
    features available in previous versions.

    The version in the base system is configured to use libedit.

    This allows to choose between emacs and vi line editing commands and
    to use command overrides via a ~/.editrc file.

    Merge commit 'bd54318046bfee055b140705a5cfd4148e78da07'

    PR:             264010

    MFC after:      2 weeks

 contrib/bc/Makefile.in                |   15 +-
 contrib/bc/NEWS.md                    |   23 +
 contrib/bc/README.md                  |   34 +-
 contrib/bc/configure.sh               |   87 ++-
 contrib/bc/gen/bc_help.txt            |   26 +
 contrib/bc/gen/dc_help.txt            |   26 +
 contrib/bc/gen/strgen.c               |  308 +++++++--
 contrib/bc/gen/strgen.sh              |   31 +-
 contrib/bc/include/args.h             |    4 +-
 contrib/bc/include/bc.h               |   65 +-
 contrib/bc/include/bcl.h              |  258 ++++---
 contrib/bc/include/dc.h               |   15 +-
 contrib/bc/include/file.h             |   80 ++-
 contrib/bc/include/history.h          |  177 +++--
 contrib/bc/include/lang.h             |   95 +--
 contrib/bc/include/lex.h              |   49 +-
 contrib/bc/include/library.h          |  104 ++-
 contrib/bc/include/num.h              |  161 +++--
 contrib/bc/include/opt.h              |   24 +-
 contrib/bc/include/parse.h            |   47 +-
 contrib/bc/include/program.h          | 1065 +++++++++++++++--------------
 contrib/bc/include/rand.h             |   48 +-
 contrib/bc/include/read.h             |   12 +-
 contrib/bc/include/status.h           |  113 +++-
 contrib/bc/include/vector.h           |  106 +--
 contrib/bc/include/version.h          |    2 +-
 contrib/bc/include/vm.h               |  155 +++--
 contrib/bc/locales/en_US.msg          |    4 +-
 contrib/bc/manuals/bc/A.1             |  128 +++-
 contrib/bc/manuals/bc/A.1.md          |  141 ++--
 contrib/bc/manuals/bc/E.1             |  100 ++-
 contrib/bc/manuals/bc/E.1.md          |  121 ++--
 contrib/bc/manuals/bc/EH.1            |  100 ++-
 contrib/bc/manuals/bc/EH.1.md         |  121 ++--
 contrib/bc/manuals/bc/EHN.1           |  100 ++-
 contrib/bc/manuals/bc/EHN.1.md        |  121 ++--
 contrib/bc/manuals/bc/EN.1            |  100 ++-
 contrib/bc/manuals/bc/EN.1.md         |  121 ++--
 contrib/bc/manuals/bc/H.1             |  128 +++-
 contrib/bc/manuals/bc/H.1.md          |  141 ++--
 contrib/bc/manuals/bc/HN.1            |  128 +++-
 contrib/bc/manuals/bc/HN.1.md         |  141 ++--
 contrib/bc/manuals/bc/N.1             |  128 +++-
 contrib/bc/manuals/bc/N.1.md          |  141 ++--
 contrib/bc/manuals/bcl.3              |   16 +-
 contrib/bc/manuals/bcl.3.md           |   10 +-
 contrib/bc/manuals/build.md           |   39 +-
 contrib/bc/manuals/dc/A.1             |   77 ++-
 contrib/bc/manuals/dc/A.1.md          |   59 +-
 contrib/bc/manuals/dc/E.1             |   60 +-
 contrib/bc/manuals/dc/E.1.md          |   48 +-
 contrib/bc/manuals/dc/EH.1            |   60 +-
 contrib/bc/manuals/dc/EH.1.md         |   48 +-
 contrib/bc/manuals/dc/EHN.1           |   60 +-
 contrib/bc/manuals/dc/EHN.1.md        |   48 +-
 contrib/bc/manuals/dc/EN.1            |   60 +-
 contrib/bc/manuals/dc/EN.1.md         |   48 +-
 contrib/bc/manuals/dc/H.1             |   77 ++-
 contrib/bc/manuals/dc/H.1.md          |   59 +-
 contrib/bc/manuals/dc/HN.1            |   77 ++-
 contrib/bc/manuals/dc/HN.1.md         |   59 +-
 contrib/bc/manuals/dc/N.1             |   77 ++-
 contrib/bc/manuals/dc/N.1.md          |   59 +-
 contrib/bc/scripts/format.sh (new +x) |   49 ++
 contrib/bc/scripts/functions.sh       |   94 +++
 contrib/bc/scripts/lint.sh (new +x)   |   63 ++
 contrib/bc/src/args.c                 |  157 ++++-
 contrib/bc/src/bc.c                   |    5 +-
 contrib/bc/src/bc_lex.c               |   90 ++-
 contrib/bc/src/bc_parse.c             |  692 +++++++++++--------
 contrib/bc/src/data.c                 |  870 ++++++++++++------------
 contrib/bc/src/dc.c                   |    5 +-
 contrib/bc/src/dc_lex.c               |   70 +-
 contrib/bc/src/dc_parse.c             |   66 +-
 contrib/bc/src/file.c                 |  214 ++++--
 contrib/bc/src/history.c              |  816 +++++++++++++++-------
 contrib/bc/src/lang.c                 |  124 ++--
 contrib/bc/src/lex.c                  |  145 ++--
 contrib/bc/src/library.c              |  387 +++++++----
 contrib/bc/src/main.c                 |   11 +-
 contrib/bc/src/num.c                  | 1188 +++++++++++++++++++++------------
 contrib/bc/src/opt.c                  |  141 ++--
 contrib/bc/src/parse.c                |   85 ++-
 contrib/bc/src/program.c              | 1108 +++++++++++++++++++-----------
 contrib/bc/src/rand.c                 |  196 ++++--
 contrib/bc/src/read.c                 |   85 ++-
 contrib/bc/src/vector.c               |  248 ++++---
 contrib/bc/src/vm.c                   |  548 +++++++++------
 contrib/bc/tests/bcl.c                |   69 +-
 contrib/bc/tests/history.py           |   10 +-
 contrib/bc/tests/other.sh             |   58 +-
 contrib/bc/vs/bc.vcxproj              |   41 +-
 usr.bin/gh-bc/Makefile                |   11 +-
 93 files changed, 9129 insertions(+), 4652 deletions(-)
Comment 22 Marcin Cieślak 2022-11-30 11:06:10 UTC
Since this bug has become a kind of catch-all for gc-bc problems, here is more from me, looks like SIGWINCH crashes dc:

Filed https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=268076