[libvirt] [PATCH] virt-wireshark.m4: Defer $(prefix) substitution
The problem with evaluating $(prefix) in configure phase is that autoconf does a lot of magic with this variable. Firstly, if the --prefix argument is not set for the configure script, the variable will have value "NONE". Wait, wat? Secondly, even autoconf scripts then have special cases for when variable is "NONE". It's even worse - whenever they need to construct new path based on prefix, they save its current value, substitute it with proper value (like /usr/local/), do what they need and then restore the original value. Le sigh. This mad behaviour is, however, suppressed if there's --prefix on the configure command line. Now, the problem is that we tried to construct path for wireshark plugin in the m4 file. Based on the knowledge described above, we shouldn't do it and just construct the full path in Makefile later where prefix has proper value. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-wireshark.m4 | 2 +- tools/Makefile.am | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index f383e2b..e1e4a59 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -38,7 +38,7 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ if test "x$ws_prefix" = "x" ; then ws_prefix="/usr"; fi - plugindir="${prefix}${plugindir#$ws_prefix}" + plugindir="${plugindir#$ws_prefix}" fi 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]) diff --git a/tools/Makefile.am b/tools/Makefile.am index e7e42c3..08e1680 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -398,7 +398,7 @@ EXTRA_DIST += \ if WITH_WIRESHARK_DISSECTOR -ws_plugindir = $(plugindir) +ws_plugindir = $(prefix)$(plugindir) ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la wireshark_src_libvirt_la_CPPFLAGS = \ -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS) -- 2.8.4
On Thu, 2016-10-20 at 15:54 +0800, Michal Privoznik wrote:
The problem with evaluating $(prefix) in configure phase is that autoconf does a lot of magic with this variable. Firstly, if the --prefix argument is not set for the configure script, the variable will have value "NONE". Wait, wat? Secondly, even autoconf scripts then have special cases for when variable is "NONE". It's even worse - whenever they need to construct new path based on prefix, they save its current value, substitute it with proper value (like /usr/local/), do what they need and then restore the original value. Le sigh. This mad behaviour is, however, suppressed if there's --prefix on the configure command line. Now, the problem is that we tried to construct path for wireshark plugin in the m4 file. Based on the knowledge described above, we shouldn't do it and just construct the full path in Makefile later where prefix has proper value.
I don't think we need to go into so much detail, we can just point to the autoconf manual[1] which tells us *not* to use these variables outside of Makefiles so that users still have a chance to override $(prefix) at 'make' or 'make install' time.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-wireshark.m4 | 2 +- tools/Makefile.am | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index f383e2b..e1e4a59 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -38,7 +38,7 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ if test "x$ws_prefix" = "x" ; then ws_prefix="/usr"; fi - plugindir="${prefix}${plugindir#$ws_prefix}" + plugindir="${plugindir#$ws_prefix}"
This got me wondering: is what we're doing even correct? If the plugindir for wireshark is eg. /usr/lib64/wireshark/plugins, should we still install under an arbitrary $(prefix)? Will wireshark pick up the plugin then?
fi 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]) diff --git a/tools/Makefile.am b/tools/Makefile.am index e7e42c3..08e1680 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -398,7 +398,7 @@ EXTRA_DIST += \ if WITH_WIRESHARK_DISSECTOR -ws_plugindir = $(plugindir) +ws_plugindir = $(prefix)$(plugindir) ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la wireshark_src_libvirt_la_CPPFLAGS = \ -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS)
ACK [1] https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Install... -- Andrea Bolognani / Red Hat / Virtualization
On 20.10.2016 22:33, Andrea Bolognani wrote:
On Thu, 2016-10-20 at 15:54 +0800, Michal Privoznik wrote:
The problem with evaluating $(prefix) in configure phase is that autoconf does a lot of magic with this variable. Firstly, if the --prefix argument is not set for the configure script, the variable will have value "NONE". Wait, wat? Secondly, even autoconf scripts then have special cases for when variable is "NONE". It's even worse - whenever they need to construct new path based on prefix, they save its current value, substitute it with proper value (like /usr/local/), do what they need and then restore the original value. Le sigh. This mad behaviour is, however, suppressed if there's --prefix on the configure command line. Now, the problem is that we tried to construct path for wireshark plugin in the m4 file. Based on the knowledge described above, we shouldn't do it and just construct the full path in Makefile later where prefix has proper value.
I don't think we need to go into so much detail, we can just point to the autoconf manual[1] which tells us *not* to use these variables outside of Makefiles so that users still have a chance to override $(prefix) at 'make' or 'make install' time.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-wireshark.m4 | 2 +- tools/Makefile.am | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index f383e2b..e1e4a59 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -38,7 +38,7 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ if test "x$ws_prefix" = "x" ; then ws_prefix="/usr"; fi - plugindir="${prefix}${plugindir#$ws_prefix}" + plugindir="${plugindir#$ws_prefix}"
This got me wondering: is what we're doing even correct?
If the plugindir for wireshark is eg. /usr/lib64/wireshark/plugins, should we still install under an arbitrary $(prefix)? Will wireshark pick up the plugin then?
Well, depends how you installed wireshark. If you provided the same prefix during its installation, the plugin is loaded. If you didn't, then it isn't loaded. But that's what you want, isn't it?
fi 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]) diff --git a/tools/Makefile.am b/tools/Makefile.am index e7e42c3..08e1680 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -398,7 +398,7 @@ EXTRA_DIST += \
if WITH_WIRESHARK_DISSECTOR
-ws_plugindir = $(plugindir) +ws_plugindir = $(prefix)$(plugindir) ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la wireshark_src_libvirt_la_CPPFLAGS = \ -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS)
ACK
Pushed thanks. Michal
On Fri, 2016-10-21 at 10:47 +0800, Michal Privoznik wrote:
If the plugindir for wireshark is eg. /usr/lib64/wireshark/plugins, should we still install under an arbitrary $(prefix)? Will wireshark pick up the plugin then? Well, depends how you installed wireshark. If you provided the same prefix during its installation, the plugin is loaded. If you didn't, then it isn't loaded. But that's what you want, isn't it?
Okay, you convinced me :) Thanks for looking into this issue by the way! -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Oct 20, 2016 at 03:54:13PM +0800, Michal Privoznik wrote:
The problem with evaluating $(prefix) in configure phase is that autoconf does a lot of magic with this variable. Firstly, if the --prefix argument is not set for the configure script, the variable will have value "NONE". Wait, wat? Secondly, even autoconf scripts then have special cases for when variable is "NONE". It's even worse - whenever they need to construct new path based on prefix, they save its current value, substitute it with proper value (like /usr/local/), do what they need and then restore the original value. Le sigh. This mad behaviour is, however, suppressed if there's --prefix on the configure command line. Now, the problem is that we tried to construct path for wireshark plugin in the m4 file. Based on the knowledge described above, we shouldn't do it and just construct the full path in Makefile later where prefix has proper value.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-wireshark.m4 | 2 +- tools/Makefile.am | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index f383e2b..e1e4a59 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -38,7 +38,7 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ if test "x$ws_prefix" = "x" ; then ws_prefix="/usr"; fi - plugindir="${prefix}${plugindir#$ws_prefix}" + plugindir="${plugindir#$ws_prefix}" fi 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]) diff --git a/tools/Makefile.am b/tools/Makefile.am index e7e42c3..08e1680 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -398,7 +398,7 @@ EXTRA_DIST += \
if WITH_WIRESHARK_DISSECTOR
-ws_plugindir = $(plugindir) +ws_plugindir = $(prefix)$(plugindir) ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la wireshark_src_libvirt_la_CPPFLAGS = \ -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS)
It looks like this patch has broken the RPM build https://ci.centos.org/view/libvirt-project/job/libvirt-master-rpm/82/systems... Notice at the make install phase: /usr/bin/mkdir -p '/home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0-1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12' /bin/sh ../libtool --mode=install /usr/bin/install -c wireshark/src/libvirt.la '/home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0-1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12' libtool: install: /usr/bin/install -c wireshark/src/.libs/libvirt.so /home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0-1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12/libvirt.so libtool: install: /usr/bin/install -c wireshark/src/.libs/libvirt.lai /home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0-1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12/libvirt.la libtool: warning: remember to run 'libtool --finish /usr/usr/lib64/wireshark/plugins/1.12.12' It is getting "/usr/usr" in the path which is very wrong. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On 10/24/2016 10:52 AM, Daniel P. Berrange wrote:
On Thu, Oct 20, 2016 at 03:54:13PM +0800, Michal Privoznik wrote:
The problem with evaluating $(prefix) in configure phase is that autoconf does a lot of magic with this variable. Firstly, if the --prefix argument is not set for the configure script, the variable will have value "NONE". Wait, wat? Secondly, even autoconf scripts then have special cases for when variable is "NONE". It's even worse - whenever they need to construct new path based on prefix, they save its current value, substitute it with proper value (like /usr/local/), do what they need and then restore the original value. Le sigh. This mad behaviour is, however, suppressed if there's --prefix on the configure command line. Now, the problem is that we tried to construct path for wireshark plugin in the m4 file. Based on the knowledge described above, we shouldn't do it and just construct the full path in Makefile later where prefix has proper value.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-wireshark.m4 | 2 +- tools/Makefile.am | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/m4/virt-wireshark.m4 b/m4/virt-wireshark.m4 index f383e2b..e1e4a59 100644 --- a/m4/virt-wireshark.m4 +++ b/m4/virt-wireshark.m4 @@ -38,7 +38,7 @@ AC_DEFUN([LIBVIRT_CHECK_WIRESHARK],[ if test "x$ws_prefix" = "x" ; then ws_prefix="/usr"; fi - plugindir="${prefix}${plugindir#$ws_prefix}" + plugindir="${plugindir#$ws_prefix}" fi 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]) diff --git a/tools/Makefile.am b/tools/Makefile.am index e7e42c3..08e1680 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -398,7 +398,7 @@ EXTRA_DIST += \
if WITH_WIRESHARK_DISSECTOR
-ws_plugindir = $(plugindir) +ws_plugindir = $(prefix)$(plugindir) ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la wireshark_src_libvirt_la_CPPFLAGS = \ -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS)
It looks like this patch has broken the RPM build
https://ci.centos.org/view/libvirt-project/job/libvirt-master-rpm/82/systems...
Notice at the make install phase:
/usr/bin/mkdir -p '/home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0-1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12' /bin/sh ../libtool --mode=install /usr/bin/install -c wireshark/src/libvirt.la '/home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0-1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12' libtool: install: /usr/bin/install -c wireshark/src/.libs/libvirt.so /home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0-1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12/libvirt.so libtool: install: /usr/bin/install -c wireshark/src/.libs/libvirt.lai /home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0-1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12/libvirt.la libtool: warning: remember to run 'libtool --finish /usr/usr/lib64/wireshark/plugins/1.12.12'
It is getting "/usr/usr" in the path which is very wrong.
Regards, Daniel
I can confirm that the rpm build is broken with wireshark versions < 2. The package config of these versions do not provide plugindir and the code change in this patch is only working correctly when it is provided and otherwise ends up with the scenario Daniel outlined above. I think that line http://libvirt.org/git/?p=libvirt.git;a=blob;f=m4/virt-wireshark.m4;h=e1e4a5... needs fixing but I have no good idea how to make the adjustment compatible with the changes in the else branch. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski 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 Wed, 2016-10-26 at 08:46 +0200, Boris Fiuczynski wrote:
if WITH_WIRESHARK_DISSECTOR -ws_plugindir = $(plugindir) +ws_plugindir = $(prefix)$(plugindir) ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la wireshark_src_libvirt_la_CPPFLAGS = \ -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS) It looks like this patch has broken the RPM build https://ci.centos.org/view/libvirt-project/job/libvirt-master-rpm/82/systems... Notice at the make install phase: /usr/bin/mkdir -p '/home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0-1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12' /bin/sh ../libtool --mode=install /usr/bin/install -c wireshark/src/libvirt.la '/home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0- 1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12' libtool: install: /usr/bin/install -c wireshark/src/.libs/libvirt.so /home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0- 1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12/libvirt.so libtool: install: /usr/bin/install -c wireshark/src/.libs/libvirt.lai /home/jenkins/rpmbuild/BUILDROOT/libvirt-2.4.0- 1.fc23.x86_64/usr/usr/lib64/wireshark/plugins/1.12.12/libvirt.la libtool: warning: remember to run 'libtool --finish /usr/usr/lib64/wireshark/plugins/1.12.12' It is getting "/usr/usr" in the path which is very wrong. I can confirm that the rpm build is broken with wireshark versions < 2. The package config of these versions do not provide plugindir and the code change in this patch is only working correctly when it is provided and otherwise ends up with the scenario Daniel outlined above. I think that line http://libvirt.org/git/?p=libvirt.git;a=blob;f=m4/virt-wireshark.m4;h=e1e4a5... needs fixing but I have no good idea how to make the adjustment compatible with the changes in the else branch.
I have a partial fix for this https://www.redhat.com/archives/libvir-list/2016-October/msg01169.html Doesn't handle all cases yet but should be good enough to make the CI job green again. Feel free to give it a go :) -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Boris Fiuczynski -
Daniel P. Berrange -
Michal Privoznik