[libvirt] [PATCH 0/3] Couple of build fixes for readline stuff
After I've merged bash completion patches, couple of build problems emerged. Michal Privoznik (3): vsh: Mark cmdComplete arguments as unused tools: Always compile {virsh,virt-admin}-completer.c m4: Check for rl_completion_quote_character m4/virt-readline.m4 | 13 +++++++++++++ tools/Makefile.am | 20 ++------------------ tools/vsh.c | 3 ++- 3 files changed, 17 insertions(+), 19 deletions(-) -- 2.13.6
When building without readline, this function does nothing but return false. Without touching any of its arguments. Therefore, we have to mark them as unused even though they might be used when building with readline support. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/vsh.c b/tools/vsh.c index 4426c08d6..88561ef61 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3500,7 +3500,8 @@ const vshCmdInfo info_complete[] = { }; bool -cmdComplete(vshControl *ctl, const vshCmd *cmd) +cmdComplete(vshControl *ctl ATTRIBUTE_UNUSED, + const vshCmd *cmd ATTRIBUTE_UNUSED) { bool ret = false; #ifdef WITH_READLINE -- 2.13.6
On Sun, Jan 14, 2018 at 02:46:43PM +0100, Michal Privoznik wrote:
When building without readline, this function does nothing but return false. Without touching any of its arguments. Therefore, we have to mark them as unused even though they might be used when building with readline support.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 4426c08d6..88561ef61 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3500,7 +3500,8 @@ const vshCmdInfo info_complete[] = { };
bool -cmdComplete(vshControl *ctl, const vshCmd *cmd) +cmdComplete(vshControl *ctl ATTRIBUTE_UNUSED, + const vshCmd *cmd ATTRIBUTE_UNUSED) { bool ret = false; #ifdef WITH_READLINE
Hmm, I have no problem with this, although, I kinda find the usage of ATTRIBUTE_UNUSED a bit obfuscated here, especially if compiled with readline, since one might not see the connection at first glance - I think we might want to go down the usual road and have a separate definition of the function in the distinct preprocessor branches, I know, more lines, but IMHO it enhances the readability. Just my 2 cents. Reviewed-by: Erik Skultety <eskultet@redhat.com> Erik
The functions defined in these sources are referenced all over the place, however, compiler only when building with readline. Thus when building without it linker gets sad as it can't find them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/Makefile.am | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/tools/Makefile.am b/tools/Makefile.am index 48125f516..e9597cdb4 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -216,6 +216,7 @@ virt_login_shell_CFLAGS = \ virsh_SOURCES = \ virsh.c virsh.h \ + virsh-completer.c virsh-completer.h \ virsh-console.c virsh-console.h \ virsh-domain.c virsh-domain.h \ virsh-domain-monitor.c virsh-domain-monitor.h \ @@ -231,15 +232,6 @@ virsh_SOURCES = \ virsh-volume.c virsh-volume.h \ $(NULL) -VIRSH_COMPLETER = \ - virsh-completer.c virsh-completer.h - -if WITH_READLINE -virsh_SOURCES += $(VIRSH_COMPLETER) -else ! WITH_READLINE -EXTRA_DIST += $(VIRSH_COMPLETER) -endif ! WITH_READLINE - virsh_LDFLAGS = \ $(AM_LDFLAGS) \ $(PIE_LDFLAGS) \ @@ -256,17 +248,9 @@ virsh_CFLAGS = \ virt_admin_SOURCES = \ virt-admin.c virt-admin.h \ + virt-admin-completer.c virt-admin-completer.h \ $(NULL) -VIRT_ADMIN_COMPLETER = \ - virt-admin-completer.c virt-admin-completer.h - -if WITH_READLINE -virt_admin_SOURCES += $(VIRT_ADMIN_COMPLETER) -else ! WITH_READLINE -EXTRA_DIST += $(VIRT_ADMIN_COMPLETER) -endif ! WITH_READLINE - virt_admin_LDFLAGS = \ $(AM_LDFLAGS) \ $(COVERAGE_LDFLAGS) \ -- 2.13.6
On Sun, Jan 14, 2018 at 02:46:44PM +0100, Michal Privoznik wrote:
The functions defined in these sources are referenced all over the place, however, compiler only when building with readline. Thus when building without it linker gets sad as it can't find them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Apparently we can't assume that people run readline recent enough to have rl_completion_quote_character (added in readline-5.0 released in 2011). However, we can't compile without it. So if not present, disable readline. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Frankly, I hate this patch. How far into the past do we want to go when introducing something new? 10 years? 15? I've only written this patch because travis is unhappy without it (I'm looking at you Mac OS/X). m4/virt-readline.m4 | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/m4/virt-readline.m4 b/m4/virt-readline.m4 index 9fba5148a..34b3ff3c4 100644 --- a/m4/virt-readline.m4 +++ b/m4/virt-readline.m4 @@ -38,6 +38,19 @@ 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.13.6
On Sun, 2018-01-14 at 14:46 +0100, Michal Privoznik wrote:
Apparently we can't assume that people run readline recent enough to have rl_completion_quote_character (added in readline-5.0 released in 2011). However, we can't compile without it. So if not present, disable readline.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Frankly, I hate this patch. How far into the past do we want to go when introducing something new? 10 years? 15? I've only written this patch because travis is unhappy without it (I'm looking at you Mac OS/X).
The problem with macOS is that Apple is shipping very old releases of a lot of GNU software. Compare that with FreeBSD, which got rid of basically all GNU software from the base system but still makes (modern versions of) it available through ports. macOS has brew, though. I've kicked off a Travis build with this commit[1] included, let's see whether configure picks up readline installed from brew instead of the obsolete one available in the base system. If it does, then we can omit your patch and... Document the version requirement somehow? If we used pkg-config to detect readline availability, that would be easy. Alas, readline only introduced pkg-config support relatively recently, so we can't do that. [1] https://github.com/andreabolognani/libvirt/commit/f2ca5da50609b814edbbb73858... -- Andrea Bolognani / Red Hat / Virtualization
On Mon, 2018-01-15 at 10:26 +0100, Andrea Bolognani wrote:
macOS has brew, though. I've kicked off a Travis build with this commit[1] included, let's see whether configure picks up readline installed from brew instead of the obsolete one available in the base system.
Nope, it still picks up the one shipped with the OS :/
If it does, then we can omit your patch and... Document the version requirement somehow? If we used pkg-config to detect readline availability, that would be easy. Alas, readline only introduced pkg-config support relatively recently, so we can't do that.
So, one way to solve this once and for all would be to: * try looking up readline through pkg-config. If that works, then we already know we're compiling against a recent readline version and everything will work; * if readline's pkg-config file is not available, try linking against it the old way. This will succeed on oldish versions like the one shipped with CentOS but fail because of missing functions on macOS. I could try cooking up something like the above, but I can never seem to get it right the first couple of times when m4 is involved, so in the interest of time - and not having to merge this patch you hate ;) - would you mind looking into it yourself instead? -- Andrea Bolognani / Red Hat / Virtualization
On 01/15/2018 12:30 PM, Andrea Bolognani wrote:
On Mon, 2018-01-15 at 10:26 +0100, Andrea Bolognani wrote:
macOS has brew, though. I've kicked off a Travis build with this commit[1] included, let's see whether configure picks up readline installed from brew instead of the obsolete one available in the base system.
Nope, it still picks up the one shipped with the OS :/
If it does, then we can omit your patch and... Document the version requirement somehow? If we used pkg-config to detect readline availability, that would be easy. Alas, readline only introduced pkg-config support relatively recently, so we can't do that.
So, one way to solve this once and for all would be to:
* try looking up readline through pkg-config. If that works, then we already know we're compiling against a recent readline version and everything will work;
* if readline's pkg-config file is not available, try linking against it the old way. This will succeed on oldish versions like the one shipped with CentOS but fail because of missing functions on macOS.
I could try cooking up something like the above, but I can never seem to get it right the first couple of times when m4 is involved, so in the interest of time - and not having to merge this patch you hate ;) - would you mind looking into it yourself instead?
The reason I hate this patch is not because the patch itself looks ugly. It's because we have to deal with the situation in the first place and invest our time in resolving it. And what you're suggesting might sound right but we'll end up with the same situation after all. I don't know. What are your thoughts? Michal
On Mon, 2018-01-15 at 13:31 +0100, Michal Privoznik wrote:
So, one way to solve this once and for all would be to:
* try looking up readline through pkg-config. If that works, then we already know we're compiling against a recent readline version and everything will work;
* if readline's pkg-config file is not available, try linking against it the old way. This will succeed on oldish versions like the one shipped with CentOS but fail because of missing functions on macOS.
I could try cooking up something like the above, but I can never seem to get it right the first couple of times when m4 is involved, so in the interest of time - and not having to merge this patch you hate ;) - would you mind looking into it yourself instead?
The reason I hate this patch is not because the patch itself looks ugly. It's because we have to deal with the situation in the first place and invest our time in resolving it. And what you're suggesting might sound right but we'll end up with the same situation after all.
I don't think that's the case. Right now we have to work around issues in macOS all the time because we're linking against the obsolete readline version included in the base system; if we implemented what I propose above, then we could just mandate that readline 6.0 or newer is required. macOS builds would then fail unless you install a recent readline using brew, but that's entirely acceptable, and we would have obtained a reasonable baseline to work against going forward. Plus it would allow us to get rid of some nasty hacks[1] on our CI as well :) [1] https://github.com/libvirt/libvirt-jenkins-ci/blob/635a1e22806c525a11d80fc32... -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Jan 15, 2018 at 02:20:01PM +0100, Andrea Bolognani wrote:
On Mon, 2018-01-15 at 13:31 +0100, Michal Privoznik wrote:
So, one way to solve this once and for all would be to:
* try looking up readline through pkg-config. If that works, then we already know we're compiling against a recent readline version and everything will work;
* if readline's pkg-config file is not available, try linking against it the old way. This will succeed on oldish versions like the one shipped with CentOS but fail because of missing functions on macOS.
I could try cooking up something like the above, but I can never seem to get it right the first couple of times when m4 is involved, so in the interest of time - and not having to merge this patch you hate ;) - would you mind looking into it yourself instead?
The reason I hate this patch is not because the patch itself looks ugly. It's because we have to deal with the situation in the first place and invest our time in resolving it. And what you're suggesting might sound right but we'll end up with the same situation after all.
I don't think that's the case. Right now we have to work around issues in macOS all the time because we're linking against the obsolete readline version included in the base system; if we implemented what I propose above, then we could just mandate that readline 6.0 or newer is required. macOS builds would then fail unless you install a recent readline using brew, but that's entirely acceptable, and we would have obtained a reasonable baseline to work against going forward. Plus it would allow us to get rid of some nasty hacks[1] on our CI as well :)
IMHO it is entirely reasonable to require modern readline when building on macOS, and not try to support the one in bsae system. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 01/15/2018 12:30 PM, Andrea Bolognani wrote:
On Mon, 2018-01-15 at 10:26 +0100, Andrea Bolognani wrote:
macOS has brew, though. I've kicked off a Travis build with this commit[1] included, let's see whether configure picks up readline installed from brew instead of the obsolete one available in the base system.
Nope, it still picks up the one shipped with the OS :/
If it does, then we can omit your patch and... Document the version requirement somehow? If we used pkg-config to detect readline availability, that would be easy. Alas, readline only introduced pkg-config support relatively recently, so we can't do that.
So, one way to solve this once and for all would be to:
* try looking up readline through pkg-config. If that works, then we already know we're compiling against a recent readline version and everything will work;
I just found out that this will not work - even though there is readline.pc.in in the readline repo, they are lacking rule to install the .pc file. So nobody ships that. For instance, on my rawhide box: [root@fedora ~]# rpm -q readline readline-7.0-5.fc26.x86_64 [root@fedora ~]# rpm -ql readline | grep \.pc [root@fedora ~]#
* if readline's pkg-config file is not available, try linking against it the old way. This will succeed on oldish versions like the one shipped with CentOS but fail because of missing functions on macOS.
I should have commented earlier too - what good it is to switch to pkg-config if we're keeping the old way of detecting the library (with this patch included) anyway? Therefore I think we should merge this patch and switch to pkg-config later (when distros have it). Michal
On Mon, 2018-01-15 at 16:36 +0100, Michal Privoznik wrote:
So, one way to solve this once and for all would be to:
* try looking up readline through pkg-config. If that works, then we already know we're compiling against a recent readline version and everything will work;
I just found out that this will not work - even though there is readline.pc.in in the readline repo, they are lacking rule to install the .pc file. So nobody ships that. For instance, on my rawhide box:
[root@fedora ~]# rpm -q readline readline-7.0-5.fc26.x86_64 [root@fedora ~]# rpm -ql readline | grep \.pc [root@fedora ~]#
Ouch. (Note the .pc file would be in the readline-devel package, not in the runtime one. Still, I've checked on my Rawhide guest and it's not there.) At least FreeBSD ships it, though: # pkg list readline | grep pc$ /usr/local/libdata/pkgconfig/readline.pc Not sure about brew. Not having access to a macOS box is really annoying in this kind of scenario.
* if readline's pkg-config file is not available, try linking against it the old way. This will succeed on oldish versions like the one shipped with CentOS but fail because of missing functions on macOS.
I should have commented earlier too - what good it is to switch to pkg-config if we're keeping the old way of detecting the library (with this patch included) anyway?
We wouldn't need to include this patch: we could just assume a readline new enough to provide a .pc file contains all the functionality we need.
Therefore I think we should merge this patch and switch to pkg-config later (when distros have it).
Well, we're running out of time for 4.0.0 anyway, so Reviewed-by: Andrea Bolognani <abologna@redhat.com> to your patch and let's work on a better solution, assuming one is even possible, later :) -- Andrea Bolognani / Red Hat / Virtualization
On 01/15/2018 05:39 PM, Andrea Bolognani wrote:
On Mon, 2018-01-15 at 16:36 +0100, Michal Privoznik wrote:
So, one way to solve this once and for all would be to:
* try looking up readline through pkg-config. If that works, then we already know we're compiling against a recent readline version and everything will work;
I just found out that this will not work - even though there is readline.pc.in in the readline repo, they are lacking rule to install the .pc file. So nobody ships that. For instance, on my rawhide box:
[root@fedora ~]# rpm -q readline readline-7.0-5.fc26.x86_64 [root@fedora ~]# rpm -ql readline | grep \.pc [root@fedora ~]#
Ouch.
(Note the .pc file would be in the readline-devel package, not in the runtime one. Still, I've checked on my Rawhide guest and it's not there.)
Ah, right.
At least FreeBSD ships it, though:
# pkg list readline | grep pc$ /usr/local/libdata/pkgconfig/readline.pc
is this GNU readline? I've heard that FreeBSD is ditching GNU software.
Not sure about brew. Not having access to a macOS box is really annoying in this kind of scenario.
* if readline's pkg-config file is not available, try linking against it the old way. This will succeed on oldish versions like the one shipped with CentOS but fail because of missing functions on macOS.
1: ^^^
I should have commented earlier too - what good it is to switch to pkg-config if we're keeping the old way of detecting the library (with this patch included) anyway?
We wouldn't need to include this patch: we could just assume a readline new enough to provide a .pc file contains all the functionality we need.
Well, what if isn't? We would just disable readline? We can't because you're advocating for linking the old way [1]. However, that wouldn't work on systems where we require more than just -lreadline (which is current situation). Currently, the problem is that my code requires this rl_completion_quote_character variable from readline.h. It's not a function, it's a variable. And if not present, compilation fails. So what should we do if pkg-config fails?
Therefore I think we should merge this patch and switch to pkg-config later (when distros have it).
Well, we're running out of time for 4.0.0 anyway, so
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Agreed, pushed. Thanks. Michal
On Mon, 2018-01-15 at 18:51 +0100, Michal Privoznik wrote:
At least FreeBSD ships it, though:
# pkg list readline | grep pc$ /usr/local/libdata/pkgconfig/readline.pc
is this GNU readline? I've heard that FreeBSD is ditching GNU software.
FreeBSD (starting with version 11) does, indeed, not ship GNU readline in their base system, which is great because you can then simply install it from ports (or using pkgng, as seen above) and it will work without any fiddling. Now, wouldn't it be great if Apple did the same thing? :)
* if readline's pkg-config file is not available, try linking against it the old way. This will succeed on oldish versions like the one shipped with CentOS but fail because of missing functions on macOS.
1: ^^^
I should have commented earlier too - what good it is to switch to pkg-config if we're keeping the old way of detecting the library (with this patch included) anyway?
We wouldn't need to include this patch: we could just assume a readline new enough to provide a .pc file contains all the functionality we need.
Well, what if isn't? We would just disable readline? We can't because you're advocating for linking the old way [1]. However, that wouldn't work on systems where we require more than just -lreadline (which is current situation). Currently, the problem is that my code requires this rl_completion_quote_character variable from readline.h. It's not a function, it's a variable. And if not present, compilation fails. So what should we do if pkg-config fails?
My whole idea was based on the assumption that the .pc file would be shipped by the OS if it was in the readline sources, which as we've seen is unfortunately not the case, so going down that path is not feasible. Your patch is currently the only option; sorry if I made it sound like I was claiming otherwise. -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Daniel P. Berrange -
Erik Skultety -
Michal Privoznik