[libvirt] [PATCH 0/6] readline: Drop obsolete code, add pkg-config support
This series drops a bunch of compatibility code that's no longer necessary to support our target platforms and introduces support for getting readline's CFLAGS and LIBS via pkg-config, which in turn makes it possible to automatically enable the feature on FreeBSD. There are a couple of caveats: * while most of the existing kludges are dropped, a new one had to be introduced :( The upsides are that the new kludge is small and straightforward, and that there's an expiration date on it already, since a patch making it unnecessary has already been provided for upstream readline; * autodetection on macOS still doesn't work because of a downstream packaging issue, but that's not any worse than the current situation anyway. Andrea Bolognani (6): tools: vsh: Drop obsolete readline compatibility code m4: readline: Extract code setting -D_FUNCTION_DEF m4: readline: Stop looking for rl_completion_quote_character() m4: readline: Drop extra_LIBS machinery m4: readline: Use pkg-config m4: readline: Add gross kludge for include path m4/virt-readline.m4 | 68 ++++++++++++++++++++++----------------------- tools/vsh.c | 17 ------------ 2 files changed, 33 insertions(+), 52 deletions(-) -- 2.20.1
This code is needed to use readline older than 4.1, but all our target platforms ship with at least 6.0 these days so we can safely get rid of it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tools/vsh.c | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 65b96f87d5..f2486498c9 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2945,32 +2945,15 @@ vshReadlineInit(vshControl *ctl) /* Opaque data for autocomplete callbacks. */ autoCompleteOpaque = ctl; - /* Allow conditional parsing of the ~/.inputrc file. - * Work around ancient readline 4.1 (hello Mac OS X), - * which declared it as 'char *' instead of 'const char *'. - */ -# if defined(RL_READLINE_VERSION) && RL_READLINE_VERSION > 0x0402 rl_readline_name = ctl->name; -# else - rl_readline_name = (char *) ctl->name; -# endif /* Tell the completer that we want a crack first. */ rl_attempted_completion_function = vshReadlineCompletion; -# if defined(RL_READLINE_VERSION) && RL_READLINE_VERSION > 0x0402 rl_basic_word_break_characters = break_characters; -# else - rl_basic_word_break_characters = (char *) break_characters; -# endif -# if defined(RL_READLINE_VERSION) && RL_READLINE_VERSION > 0x0402 rl_completer_quote_characters = quote_characters; rl_char_is_quoted_p = vshReadlineCharIsQuoted; -# else - rl_completer_quote_characters = (char *) quote_characters; - rl_char_is_quoted_p = (Function *) vshReadlineCharIsQuoted; -# endif if (virAsprintf(&histsize_env, "%s_HISTSIZE", ctl->env_prefix) < 0) goto cleanup; -- 2.20.1
The current code is a bit awkward, and we're going to need to share it later anyway. We can drop the call to AC_SUBST() while we're at it, since LIBVIRT_CHECK_LIB() already marks READLINE_CFLAGS for substitution. The new code goes to some extra length to avoid setting -D_FUNCTION_DEF twice: this is mostly for cosmetic reasons, and it's necessary because LIBVIRT_CHECK_READLINE() is called twice: once on its own, and then once more as part of LIBVIRT_CHECK_BASH_COMPLETION(). Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- m4/virt-readline.m4 | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/m4/virt-readline.m4 b/m4/virt-readline.m4 index 34b3ff3c4a..ffaeacef75 100644 --- a/m4/virt-readline.m4 +++ b/m4/virt-readline.m4 @@ -58,10 +58,18 @@ AC_DEFUN([LIBVIRT_CHECK_READLINE],[ # function, to ensure we aren't being confused by caching. LIBS=$lv_saved_libs AC_CHECK_LIB([readline], [rl_initialize], - [READLINE_CFLAGS="-D_FUNCTION_DEF $READLINE_CFLAGS" - AC_SUBST(READLINE_CFLAGS)], + [], [READLINE_LIBS="$READLINE_LIBS $extra_LIBS"]) LIBS=$lv_saved_libs + + # We need this to avoid compilation issues with modern compilers. + # See 9ea3424a178 for a more detailed explanation + if test "$with_readline" = "yes" ; then + case "$READLINE_CFLAGS" in + *-D_FUNCTION_DEF*) ;; + *) READLINE_CFLAGS="-D_FUNCTION_DEF $READLINE_CFLAGS" ;; + esac + fi ]) AC_DEFUN([LIBVIRT_RESULT_READLINE],[ -- 2.20.1
The function was introduced in readline 5.0, and all our target platforms ship with at least 6.0 these days. The check is also used as part of the extra_LIBS machinery, but we're going to drop that too next anyway. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- m4/virt-readline.m4 | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/m4/virt-readline.m4 b/m4/virt-readline.m4 index ffaeacef75..09d5d820ea 100644 --- a/m4/virt-readline.m4 +++ b/m4/virt-readline.m4 @@ -38,19 +38,6 @@ AC_DEFUN([LIBVIRT_CHECK_READLINE],[ LIBS="$lv_saved_libs $extra_LIBS" fi - AC_CHECK_DECLS([rl_completion_quote_character], - [], [], - [[#include <stdio.h> - #include <readline/readline.h>]]) - - if test "$ac_cv_have_decl_rl_completion_quote_character" = "no" ; then - if test "$with_readline" = "yes" ; then - AC_MSG_ERROR([readline is missing rl_completion_quote_character]) - else - with_readline=no; - fi - fi - # The normal library check... LIBVIRT_CHECK_LIB([READLINE], [readline], [readline], [readline/readline.h]) -- 2.20.1
The first implementation of this logic was introduced with commit 2ec759fc58f all the way back in 2007; looking at the build logs from our CI environment, however, it's apparent that none of the platforms we currently target are actually using it, so we can assume whatever issue it was working around has been fixed at some point in the last 12 years. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- m4/virt-readline.m4 | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/m4/virt-readline.m4 b/m4/virt-readline.m4 index 09d5d820ea..a2788cb4aa 100644 --- a/m4/virt-readline.m4 +++ b/m4/virt-readline.m4 @@ -22,33 +22,10 @@ AC_DEFUN([LIBVIRT_ARG_READLINE],[ ]) AC_DEFUN([LIBVIRT_CHECK_READLINE],[ - extra_LIBS= - lv_saved_libs=$LIBS - if test "x$with_readline" != xno; then - # Linking with -lreadline may require some termcap-related code, e.g., - # from one of the following libraries. Add it to LIBS before using - # canned library checks; then verify later if it was needed. - LIBS= - AC_SEARCH_LIBS([tgetent], [ncurses curses termcap termlib]) - case $LIBS in - no*) ;; # handle "no" and "none required" - *) # anything else is a -lLIBRARY - extra_LIBS=$LIBS ;; - esac - LIBS="$lv_saved_libs $extra_LIBS" - fi # The normal library check... LIBVIRT_CHECK_LIB([READLINE], [readline], [readline], [readline/readline.h]) - # Touch things up to avoid $extra_LIBS, if possible. Test a second - # function, to ensure we aren't being confused by caching. - LIBS=$lv_saved_libs - AC_CHECK_LIB([readline], [rl_initialize], - [], - [READLINE_LIBS="$READLINE_LIBS $extra_LIBS"]) - LIBS=$lv_saved_libs - # We need this to avoid compilation issues with modern compilers. # See 9ea3424a178 for a more detailed explanation if test "$with_readline" = "yes" ; then -- 2.20.1
With the 7.0 release, readline has finally started shipping pkg-config support in the form of a readline.pc file. Unfortunately, most downstreams have yet to catch up with this change: among Linux distributions in particular, Fedora Rawhide seems to be the only one installing it at the moment. Non-Linux operating systems have been faring much better in this regard: both FreeBSD (through ports) and macOS (through homebrew) include pkg-config support in their readline package. This is great news for us, since those are the platforms where pkg-config is more useful on account of them installing headers and libraries outside of the respective default search paths. Our implementation checks whether readline is registered as a pkg-config package, and if so obtains CFLAGS and LIBS using the tool; if not, we just keep using the existing logic. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- m4/virt-readline.m4 | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/m4/virt-readline.m4 b/m4/virt-readline.m4 index a2788cb4aa..6fde47d2c8 100644 --- a/m4/virt-readline.m4 +++ b/m4/virt-readline.m4 @@ -23,8 +23,17 @@ AC_DEFUN([LIBVIRT_ARG_READLINE],[ AC_DEFUN([LIBVIRT_CHECK_READLINE],[ - # The normal library check... - LIBVIRT_CHECK_LIB([READLINE], [readline], [readline], [readline/readline.h]) + # We have to check for readline.pc's presence beforehand because for + # the longest time the library didn't ship a .pc file at all + PKG_CHECK_EXISTS([readline], [use_pkgconfig=1], [use_pkgconfig=0]) + + if test $use_pkgconfig = 1; then + # readline 7.0 is the first version which includes pkg-config support + LIBVIRT_CHECK_PKG([READLINE], [readline], [7.0]) + else + # The normal library check... + LIBVIRT_CHECK_LIB([READLINE], [readline], [readline], [readline/readline.h]) + fi # We need this to avoid compilation issues with modern compilers. # See 9ea3424a178 for a more detailed explanation -- 2.20.1
Unfortunately the data reported by pkg-config is not completely accurate, so until the issue has been fixed in readline we need to work around it in libvirt. The good news is that we only need the fix to land in FreeBSD ports and macOS homebrew before we can drop the kludge, so we're talking months rather than years. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- m4/virt-readline.m4 | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/m4/virt-readline.m4 b/m4/virt-readline.m4 index 6fde47d2c8..0e83b026e0 100644 --- a/m4/virt-readline.m4 +++ b/m4/virt-readline.m4 @@ -43,6 +43,23 @@ AC_DEFUN([LIBVIRT_CHECK_READLINE],[ *) READLINE_CFLAGS="-D_FUNCTION_DEF $READLINE_CFLAGS" ;; esac fi + + # Gross kludge for readline include path obtained through pkg-config. + # + # As of 8.0, upstream readline.pc has -I${includedir}/readline among + # its Cflags, which is clearly wrong. This does not affect Linux + # because ${includedir} is already part of the default include path, + # but on other platforms that's not the case and the result is that + # <readline/readline.h> can't be located, causing the build to fail. + # A patch solving this issue has already been posted upstream, so once + # the fix has landed in FreeBSD ports and macOS homebrew we can safely + # drop the kludge and rely on pkg-config alone on those platforms. + # + # [1] http://lists.gnu.org/archive/html/bug-readline/2019-04/msg00007.html + case "$READLINE_CFLAGS" in + *include/readline*) READLINE_CFLAGS=$(echo $READLINE_CFLAGS | sed s,include/readline,include,g) ;; + *) ;; + esac ]) AC_DEFUN([LIBVIRT_RESULT_READLINE],[ -- 2.20.1
On Tue, 2019-04-09 at 15:10 +0200, Andrea Bolognani wrote: [...]
* autodetection on macOS still doesn't work because of a downstream packaging issue, but that's not any worse than the current situation anyway.
Actually this breaks compilation on macOS, because now when the pkg-config file is not present we'll pick up libedit from the base system, which shadows readline, and fail because it doesn't provide all the features we're expecting :( I'll post a v2 that introduces no regressions in a bit. -- Andrea Bolognani / Red Hat / Virtualization
participants (1)
-
Andrea Bolognani