Page MenuHomeFreeBSD

Update libedit to 2019-09-10
ClosedPublic

Authored by bapt on Tue, Sep 10, 2:40 PM.

Details

Summary

Our custom patches are not necessary anymore
let's switch to plain upstream libedit

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bapt created this revision.Tue, Sep 10, 2:40 PM
bcr accepted this revision as: manpages.Tue, Sep 10, 7:04 PM
bcr added a subscriber: bcr.

Manpages is OK with it, since it is a file deletion and no content change as far as I can see.

bapt added a reviewer: naito.yuichiro_gmail.com.

I am puzzeled about bumping shlib. As far as I can tell, none of the changes are used in base and I readded a compatibility shim for _el_fn_sh_complete

@bapt

I think bumping shlib is not needed.
It seems that '_el_fn_sh_complete' is the only function that is referenced by base.
It has been already redefined in contrib/libedit to keep compatibility.

I have tried this patch,
building "gnu/usr.bin/gdb" and "usr.sbin/ntp/libntp" was failed.
Because "readline/readline.h" has moved from "lib/libedit/edit/readline/readline.h" to "contrib/libedit/readline/readline.h".

I have tried to fix CLFAGS path from "-I${SRCTOP}/lib/libedit/edit" to
"-I${SRCTOP}/contrib/libedit -I${SRCTOP}/lib/libedit".

It solves gdb build but doesn't libntp build.
Because "usr.sbin/ntp/config.h" conflicts with "contrib/libedit/config.h".
For the compiler options, "contrib/libedit/config.h" was read earlier.

If "lib/libedit/readline/readline.h" is as same as "contrib/libedit/readline/readline.h", I can omit "-I${SRCTOP}/contrib/libedit" that solves the conflict.

IMHO, symlink from "contrib/libedit/readline/readline.h" to "lib/libedit/readline/readline.h" works in my source tree.

bapt added a comment.Thu, Sep 12, 3:51 PM

Both gdb and ntp errors are bugs in the build system we do use. which I have addressed in rS352248 and rS352249.

@bapt

Thanks for the patches, I could make buildworld.
I can confirm this libedit works with /bin/sh and /usr/sbin/sftp under UTF-8 and eucJP locales.
I also confirmed previous /bin/sh (before applying this patch) can work with this libedit.
It looks good to me.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Sep 13, 6:50 AM
This revision was automatically updated to reflect the committed changes.
pfg added a comment.Fri, Sep 13, 4:47 PM

I was busy in the hospital (visiting not as patient) and didn't get the chance to approve it ... Huge thanks for doing this!

This breaks the build for aarch64:

--- terminal.o ---
/usr/src/contrib/libedit/terminal.c:569:41: error: comparison of integers of different signs: 'wint_t' (aka 'int') and 'wchar_t' (aka 'unsigned int') [-Werror,-Wsign-compare]
                                            el->el_cursor.v][where & 0370] !=
                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/usr/src/contrib/libedit/terminal.c:659:28: error: comparison of integers of different signs: 'wint_t' (aka 'int') and 'wchar_t' (aka 'unsigned int') [-Werror,-Wsign-compare]
                                            [el->el_cursor.h] == MB_FILL_CHAR)
                                            ~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~
2 errors generated.
*** [terminal.o] Error code 1

Sorry for the compile error.

It seems that MB_FILL_CHAR is always used for el_vdisplay.
The type of el_vdisplay is wint_t**,
so I think MB_FILL_CHAR should be defined as wint_t.

Following patch changes MB_FILL_CHAR type from wchar_t to wint_t.
With this patch, buildworld TARGET=arm64 has passed in my source tree.

I will talk to christos@netbsd.org and ask him to apply this patch.

Index: contrib/libedit/chartype.h
===================================================================
--- contrib/libedit/chartype.h	(revision 352312)
+++ contrib/libedit/chartype.h	(working copy)
@@ -87,7 +87,7 @@
 /* The terminal is thought of in terms of X columns by Y lines. In the cases
  * where a wide character takes up more than one column, the adjacent
  * occupied column entries will contain this faux character. */
-#define MB_FILL_CHAR ((wchar_t)-1)
+#define MB_FILL_CHAR ((wint_t)-1)

 /* Visual width of character c, taking into account ^? , \0177 and \U+nnnnn
  * style visual expansions. */
Index: contrib/libedit/terminal.c
===================================================================
--- contrib/libedit/terminal.c	(revision 352312)
+++ contrib/libedit/terminal.c	(working copy)
@@ -1224,7 +1224,7 @@
 {
 	char buf[MB_LEN_MAX +1];
 	ssize_t i;
-	if (c == (wint_t)MB_FILL_CHAR)
+	if (c == MB_FILL_CHAR)
 		return 0;
 	if (c & EL_LITERAL)
 		return fputs(literal_get(el, c), el->el_outfile);

Quoted Text