[libvirt] [PATCH 0/3] wireshark: Further build system fixes
This takes care of the few remaining nits. All use cases I could think of are covered; if any more issues are discovered, we'll take care of them then. Andrea Bolognani (3): wireshark: Don't redefine ws_plugindir wireshark: Try a bunch of possible prefixes wireshark: Use ${exec_prefix} for $ws_plugindir m4/virt-wireshark.m4 | 23 ++++++++++++++++------- tools/Makefile.am | 1 - 2 files changed, 16 insertions(+), 8 deletions(-) -- 2.7.4
autoconf already defines the variable for us, and prints out a warning if we try to do it a second time. So let's not :) --- tools/Makefile.am | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/Makefile.am b/tools/Makefile.am index 319abb2..100e657 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -398,7 +398,6 @@ EXTRA_DIST += \ if WITH_WIRESHARK_DISSECTOR -ws_plugindir = @ws_plugindir@ ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la wireshark_src_libvirt_la_CPPFLAGS = \ -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS) -- 2.7.4
If we can't obtain Wireshark's plugindir variable from pkg-config, we fall back to building it ourselves starting from $libdir. The problem with that is that we have zero insights on what $libdir actually looks like, so we can't simply strip $prefix and call it a day. On the other hand, we have to do *something* or $ws_plugindir will be unusable. Our solution is to try the four most likely prefixes, and use the first one that matches. It's not perfect, but should be able to cope with all but the weirdest setups. Worst case scenario, the user can pass --with-ws-plugindir to configure and explicitly provide a suitable installation path. --- m4/virt-wireshark.m4 | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index 556272a..75786de 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -35,11 +35,20 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ dnl On some systems the plugindir variable may not be stored within pkg config. dnl Fall back to older style of constructing the plugin dir path. ws_plugindir="$libdir/wireshark/plugins/$ws_modversion" - ws_prefix="$prefix" + dnl We have no idea what the contents of $libdir look like, so we'll + dnl have to play a bit of a guessing game: let's try stripping off + dnl a bunch of likels prefixed and pick the first one that matches. + dnl Even if none does, we'll still have one last shot later + for try in "$prefix" "$exec_prefix" '${prefix}' '${exec_prefix}'; do + if test "x${ws_plugindir#$try}" != "x$ws_plugindir"; then + ws_prefix="$try" + break + fi + done fi if test "x$ws_prefix" = "x" ; then - dnl If the wireshark prefix cannot be retrieved from pkg-config, - dnl /usr is our best bet + dnl If the wireshark prefix cannot be retrieved from pkg-config + dnl or otherwise guessed, /usr is our best bet ws_prefix="/usr" fi dnl Replace the wireshark prefix with our own. -- 2.7.4
On 26.10.2016 18:08, Andrea Bolognani wrote:
If we can't obtain Wireshark's plugindir variable from pkg-config, we fall back to building it ourselves starting from $libdir.
The problem with that is that we have zero insights on what $libdir actually looks like, so we can't simply strip $prefix and call it a day. On the other hand, we have to do *something* or $ws_plugindir will be unusable.
Our solution is to try the four most likely prefixes, and use the first one that matches. It's not perfect, but should be able to cope with all but the weirdest setups.
Worst case scenario, the user can pass --with-ws-plugindir to configure and explicitly provide a suitable installation path. --- m4/virt-wireshark.m4 | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index 556272a..75786de 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -35,11 +35,20 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ dnl On some systems the plugindir variable may not be stored within pkg config. dnl Fall back to older style of constructing the plugin dir path. ws_plugindir="$libdir/wireshark/plugins/$ws_modversion" - ws_prefix="$prefix" + dnl We have no idea what the contents of $libdir look like, so we'll + dnl have to play a bit of a guessing game: let's try stripping off + dnl a bunch of likels prefixed and pick the first one that matches. + dnl Even if none does, we'll still have one last shot later + for try in "$prefix" "$exec_prefix" '${prefix}' '${exec_prefix}'; do + if test "x${ws_plugindir#$try}" != "x$ws_plugindir"; then + ws_prefix="$try" + break + fi + done fi if test "x$ws_prefix" = "x" ; then - dnl If the wireshark prefix cannot be retrieved from pkg-config, - dnl /usr is our best bet + dnl If the wireshark prefix cannot be retrieved from pkg-config + dnl or otherwise guessed, /usr is our best bet ws_prefix="/usr" fi dnl Replace the wireshark prefix with our own.
I am under the impression that it will be pretty hard to build a functioning solution for a plugindir-less wireshark.pc file other than for the purpose to fix the RPM build (and don't break make distcheck). My first idea was to start off using the libdir value from wireshark, see https://www.redhat.com/archives/libvir-list/2016-October/msg01186.html (the wireshark.pc versions I've seen all have it set, as well as the prefix). This would cover the case where libdir[wireshark] != libdir[libvirt]. But both your and my approach will only be correct if the [exec_]prefix of both libvirt and wireshark match, which will not be the case for non-RPM builds for the default configuration (exec_prefix=/usr/local) or the less usual but valid /opt/$package naming scheme. Plus libdir has to be the same in both packages anyway in order to not break the RPM build (unless you want to tweak the spec file). I'd suggest to stick to your original version (and rely on the last shot as you put it): the general case is really hard to fix properly. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, 2016-10-27 at 10:04 +0200, Viktor Mihajlovski wrote:
If we can't obtain Wireshark's plugindir variable from pkg-config, we fall back to building it ourselves starting from $libdir. The problem with that is that we have zero insights on what $libdir actually looks like, so we can't simply strip $prefix and call it a day. On the other hand, we have to do *something* or $ws_plugindir will be unusable. Our solution is to try the four most likely prefixes, and use the first one that matches. It's not perfect, but should be able to cope with all but the weirdest setups. Worst case scenario, the user can pass --with-ws-plugindir to configure and explicitly provide a suitable installation path. --- m4/virt-wireshark.m4 | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index 556272a..75786de 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -35,11 +35,20 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ dnl On some systems the plugindir variable may not be stored within pkg config. dnl Fall back to older style of constructing the plugin dir path. ws_plugindir="$libdir/wireshark/plugins/$ws_modversion" - ws_prefix="$prefix" + dnl We have no idea what the contents of $libdir look like, so we'll + dnl have to play a bit of a guessing game: let's try stripping off + dnl a bunch of likels prefixed and pick the first one that matches. + dnl Even if none does, we'll still have one last shot later + for try in "$prefix" "$exec_prefix" '${prefix}' '${exec_prefix}'; do + if test "x${ws_plugindir#$try}" != "x$ws_plugindir"; then + ws_prefix="$try" + break + fi + done fi if test "x$ws_prefix" = "x" ; then - dnl If the wireshark prefix cannot be retrieved from pkg-config, - dnl /usr is our best bet + dnl If the wireshark prefix cannot be retrieved from pkg-config + dnl or otherwise guessed, /usr is our best bet ws_prefix="/usr" fi dnl Replace the wireshark prefix with our own. I am under the impression that it will be pretty hard to build a functioning solution for a plugindir-less wireshark.pc file other than for the purpose to fix the RPM build (and don't break make distcheck). My first idea was to start off using the libdir value from wireshark, see https://www.redhat.com/archives/libvir-list/2016-October/msg01186.html (the wireshark.pc versions I've seen all have it set, as well as the
On 26.10.2016 18:08, Andrea Bolognani wrote: prefix). This would cover the case where libdir[wireshark] != libdir[libvirt]. But both your and my approach will only be correct if the [exec_]prefix of both libvirt and wireshark match, which will not be the case for non-RPM builds for the default configuration (exec_prefix=/usr/local) or the less usual but valid /opt/$package naming scheme. Plus libdir has to be the same in both packages anyway in order to not break the RPM build (unless you want to tweak the spec file). I'd suggest to stick to your original version (and rely on the last shot as you put it): the general case is really hard to fix properly.
Of course the only way for the plugin to be loaded by Wireshark is to install it in the directory Wireshark will load plugins for :) But, as far as libvirt is concerned, it's also completely okay to install everything, including the plugin, under eg. /usr/local regardless of the fact that Wireshark will look for plugins in a sub-directory of /usr/lib. It would actually be a bug if libvirt was compiled with prefix=/usr/local and installed *anything* outside of /usr/local, unless the user provides a very specific override. We can't stick to the previous version because one very common use case is broken with it: passing only prefix to configure and relying on the automatically derived values for everything else. I've come up with a hybrid approach that incorporates some of your suggested changes with the ones that we've come up with independently, and AFAICT handles all reasonable use cases correctly. I'll post it shortly. Hope you don't mind my sticking with my own commits, it's just that the fact that I'm changing a single thing per commit makes review easier than lumping three changes together :) -- Andrea Bolognani / Red Hat / Virtualization
Plus libdir has to be the same in both packages anyway in order to not break the RPM build (unless you want to tweak the spec file). I'd suggest to stick to your original version (and rely on the last shot as you put it): the general case is really hard to fix properly.
Of course the only way for the plugin to be loaded by Wireshark is to install it in the directory Wireshark will load plugins for :) Sure :-)
But, as far as libvirt is concerned, it's also completely okay to install everything, including the plugin, under eg. /usr/local regardless of the fact that Wireshark will look for plugins in a sub-directory of /usr/lib. It would actually be a bug if libvirt was compiled with prefix=/usr/local and installed *anything* outside of /usr/local, unless the user provides a very specific override. Couldn't agree more.
We can't stick to the previous version because one very common use case is broken with it: passing only prefix to configure and relying on the automatically derived values for everything else. I meant your previous, already committed patch, which already had the "sentinel prefix" prepended.
I've come up with a hybrid approach that incorporates some of your suggested changes with the ones that we've come up with independently, and AFAICT handles all reasonable use cases correctly. I'll post it shortly. My email server is slow as molasses. But I've reviewed the V2 series on
On 27.10.2016 16:13, Andrea Bolognani wrote: [...] the list archive(!) and it looks fine to me.
Hope you don't mind my sticking with my own commits, it's just that the fact that I'm changing a single thing per commit makes review easier than lumping three changes together :)
Thanks for considering the comments.
-- Andrea Bolognani / Red Hat / Virtualization
-- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
${exec_prefix} and ${prefix} point to the same directory in most setups, but when that's not the case the former should be used for architecture-dependent data such as shared objects, which makes it the best fit for our Wireshark dissector. While at it, change from $(var) to ${var}: they are absolutely identicaly as far as make's concerned, but autoconf itself seems to prefer the latter form so we might as well follow suit. --- m4/virt-wireshark.m4 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index 75786de..7a817f6 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -52,10 +52,10 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ ws_prefix="/usr" fi dnl Replace the wireshark prefix with our own. - dnl Note that $(prefix) is kept verbatim at this point in time, and will - dnl only be expanded later, when make is called: this makes it possible - dnl to override the prefix at compilation or installation time - ws_plugindir='$(prefix)'"${ws_plugindir#$ws_prefix}" + dnl Note that ${exec_prefix} is kept verbatim at this point in time, and + dnl will only be expanded later, when make is called: this makes it + dnl possible to override such prefix at compilation or installation time + ws_plugindir='${exec_prefix}'"${ws_plugindir#$ws_prefix}" elif test "x$with_ws_plugindir" = "xno" || test "x$with_ws_plugindir" = "xyes"; then AC_MSG_ERROR([ws-plugindir must be used only with valid path]) else -- 2.7.4
On 26.10.2016 18:08, Andrea Bolognani wrote:
${exec_prefix} and ${prefix} point to the same directory in most setups, but when that's not the case the former should be used for architecture-dependent data such as shared objects, which makes it the best fit for our Wireshark dissector.
While at it, change from $(var) to ${var}: they are absolutely identicaly as far as make's concerned, but autoconf itself seems to prefer the latter form so we might as well follow suit. --- m4/virt-wireshark.m4 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index 75786de..7a817f6 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -52,10 +52,10 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ ws_prefix="/usr" fi dnl Replace the wireshark prefix with our own. - dnl Note that $(prefix) is kept verbatim at this point in time, and will - dnl only be expanded later, when make is called: this makes it possible - dnl to override the prefix at compilation or installation time - ws_plugindir='$(prefix)'"${ws_plugindir#$ws_prefix}" + dnl Note that ${exec_prefix} is kept verbatim at this point in time, and + dnl will only be expanded later, when make is called: this makes it + dnl possible to override such prefix at compilation or installation time + ws_plugindir='${exec_prefix}'"${ws_plugindir#$ws_prefix}" elif test "x$with_ws_plugindir" = "xno" || test "x$with_ws_plugindir" = "xyes"; then AC_MSG_ERROR([ws-plugindir must be used only with valid path]) else
Purely cosmetic, but s/ws_prefix/ws_exec_prefix/ in the entire file could avoid future confusions. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (2)
-
Andrea Bolognani -
Viktor Mihajlovski