[libvirt] [PATCH 0/4] Make uninstall clean again
It's been a while since 'make uninstall' cleaned up everything that 'make install' created. I've noticed this while trying to figure out why some build test on *BSD is failing (fix for that is in the first patch btw). Michal Privoznik (4): nss: Try harder to uninstall examples: Try harder to uninstall nwfilter docs: Uninstall libvirt logo too docs: Don't leave any documentation behind docs/Makefile.am | 7 +++++++ examples/Makefile.am | 2 +- tools/Makefile.am | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) -- 2.7.3
On BSD we are creating this symlink to libnss_libvirt.so called nss_libvirt.so. That's just the way it is on BSD. However, when uninstalling, we try to remove libnss_libvirt.so instead of the symlink. Moreover, if file we are trying to remove does not exist we error out instead of ignoring the error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/Makefile.am b/tools/Makefile.am index 560a9a5..c474d75 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -427,7 +427,7 @@ install-exec-hook: $(LN_S) libnss_libvirt.so.$(NSS_SO_VER) nss_libvirt.so.$(NSS_SO_VER) uninstall-local: - rm $(DESTDIR)$(libdir)/libnss_libvirt.so.$(NSS_SO_VER) + -rm -f $(DESTDIR)$(libdir)/nss_libvirt.so.$(NSS_SO_VER) else ! WITH_BSD_NSS LIBVIRT_NSS_SYMBOL_FILE = \ $(srcdir)/nss/libvirt_nss.syms -- 2.7.3
On 04/19/2016 09:50 AM, Michal Privoznik wrote:
On BSD we are creating this symlink to libnss_libvirt.so called nss_libvirt.so. That's just the way it is on BSD. However, when uninstalling, we try to remove libnss_libvirt.so instead of the symlink. Moreover, if file we are trying to remove does not exist we error out instead of ignoring the error.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/Makefile.am b/tools/Makefile.am index 560a9a5..c474d75 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -427,7 +427,7 @@ install-exec-hook: $(LN_S) libnss_libvirt.so.$(NSS_SO_VER) nss_libvirt.so.$(NSS_SO_VER)
uninstall-local: - rm $(DESTDIR)$(libdir)/libnss_libvirt.so.$(NSS_SO_VER) + -rm -f $(DESTDIR)$(libdir)/nss_libvirt.so.$(NSS_SO_VER)
^ Looks like a classic cut-n-paste error ACK w/ the adjustment John
else ! WITH_BSD_NSS LIBVIRT_NSS_SYMBOL_FILE = \ $(srcdir)/nss/libvirt_nss.syms
We have this code in our Makefile that tries to remove /etc/libvirt/nwfilter if directory is left empty after all our example nwfilters were uninstalled. However, the check for that is missing quotation marks thus rendering the test useless: test -z allow-arp.xml allow-dhcp-server.xml .. qemu-announce-self.xml || \ rmdir "/some/path/libvirt.git/_install/etc/libvirt/nwfilter" /bin/sh: line 0: test: too many arguments Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/Makefile.am b/examples/Makefile.am index 46465f9..e1c37f0 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -90,5 +90,5 @@ uninstall-local:: for f in $(FILTERS); do \ rm -f "$(NWFILTER_DIR)/`basename $$f`"; \ done - -test -z $(shell ls $(NWFILTER_DIR)) || rmdir $(NWFILTER_DIR) + -test -z "$(shell ls $(NWFILTER_DIR))" || rmdir $(NWFILTER_DIR) endif WITH_NWFILTER -- 2.7.3
On 04/19/2016 09:50 AM, Michal Privoznik wrote:
We have this code in our Makefile that tries to remove /etc/libvirt/nwfilter if directory is left empty after all our example nwfilters were uninstalled. However, the check for that is missing quotation marks thus rendering the test useless:
test -z allow-arp.xml allow-dhcp-server.xml .. qemu-announce-self.xml || \ rmdir "/some/path/libvirt.git/_install/etc/libvirt/nwfilter" /bin/sh: line 0: test: too many arguments
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK John
While we could leave it behind as an indelible sign that libvirt has been running on host, other users might not be that fond of it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/Makefile.am | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/Makefile.am b/docs/Makefile.am index f4ce9ce..c80c37b 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -368,3 +368,4 @@ uninstall-local: for f in $(devhelphtml) $(devhelppng) $(devhelpcss); do \ rm -f $(DESTDIR)$(DEVHELP_DIR)/$$(basename $$f); \ done + rm -f $(DESTDIR)$(pkgdatadir)/libvirtLogo.png -- 2.7.3
On 04/19/2016 09:50 AM, Michal Privoznik wrote:
While we could leave it behind as an indelible sign that libvirt has been running on host, other users might not be that fond of it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/Makefile.am | 1 + 1 file changed, 1 insertion(+)
ACK John
Our uninstall script is not exact counterpart of install one. Therefore we are leaving couple of files behind. This should not happen. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/Makefile.am | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/Makefile.am b/docs/Makefile.am index c80c37b..9524c94 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -363,8 +363,14 @@ install-data-local: $(INSTALL_DATA) $(srcdir)/libvirtLogo.png $(DESTDIR)$(pkgdatadir) uninstall-local: + for f in $(css) $(dot_html) $(gif) $(png); do \ + rm -f $(DESTDIR)$(HTML_DIR)/$$f; \ + done for h in $(apihtml); do rm -f $(DESTDIR)$(HTML_DIR)/$$h; done for p in $(apipng); do rm -f $(DESTDIR)$(HTML_DIR)/$$p; done + for f in $(internals_html); do \ + rm -f $(DESTDIR)$(HTML_DIR)/$$f; \ + done for f in $(devhelphtml) $(devhelppng) $(devhelpcss); do \ rm -f $(DESTDIR)$(DEVHELP_DIR)/$$(basename $$f); \ done -- 2.7.3
On 04/19/2016 09:50 AM, Michal Privoznik wrote:
Our uninstall script is not exact counterpart of install one. Therefore we are leaving couple of files behind. This should not happen.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/Makefile.am | 6 ++++++ 1 file changed, 6 insertions(+)
At 'install-data-local:', there's a : $(mkinstalldirs) $(DESTDIR)$(HTML_DIR) why not just the far more all encompassing: rm -rf $(DESTDIR)$(HTML_DIR) and rm -rf $(DESTDIR)$(DEVHELP_DIR) Rather than picking each part we install to uninstall? and missing something in the future or even now. Do the 'html' or 'internals' directories gets removed? And then of course the toplevel directory which we created. IOW: There's no corollary for the: $(mkinstalldirs) $(DESTDIR)$(HTML_DIR) $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/html $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/internals $(mkinstalldirs) $(DESTDIR)$(DEVHELP_DIR) John
diff --git a/docs/Makefile.am b/docs/Makefile.am index c80c37b..9524c94 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -363,8 +363,14 @@ install-data-local: $(INSTALL_DATA) $(srcdir)/libvirtLogo.png $(DESTDIR)$(pkgdatadir)
uninstall-local: + for f in $(css) $(dot_html) $(gif) $(png); do \ + rm -f $(DESTDIR)$(HTML_DIR)/$$f; \ + done for h in $(apihtml); do rm -f $(DESTDIR)$(HTML_DIR)/$$h; done for p in $(apipng); do rm -f $(DESTDIR)$(HTML_DIR)/$$p; done + for f in $(internals_html); do \ + rm -f $(DESTDIR)$(HTML_DIR)/$$f; \ + done for f in $(devhelphtml) $(devhelppng) $(devhelpcss); do \ rm -f $(DESTDIR)$(DEVHELP_DIR)/$$(basename $$f); \ done
On 19.04.2016 16:38, John Ferlan wrote:
On 04/19/2016 09:50 AM, Michal Privoznik wrote:
Our uninstall script is not exact counterpart of install one. Therefore we are leaving couple of files behind. This should not happen.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/Makefile.am | 6 ++++++ 1 file changed, 6 insertions(+)
At 'install-data-local:', there's a :
$(mkinstalldirs) $(DESTDIR)$(HTML_DIR)
why not just the far more all encompassing:
rm -rf $(DESTDIR)$(HTML_DIR)
and
rm -rf $(DESTDIR)$(DEVHELP_DIR)
Rather than picking each part we install to uninstall? and missing something in the future or even now. Do the 'html' or 'internals' directories gets removed? And then of course the toplevel directory which we created.
IOW: There's no corollary for the:
$(mkinstalldirs) $(DESTDIR)$(HTML_DIR) $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/html $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/internals $(mkinstalldirs) $(DESTDIR)$(DEVHELP_DIR)
Yeah. That's the other way of doing that. It's just that if users put anything in $(DESTDIR)$(HTML_DIR) it will be removed by uninstall. But I can propose v2 if you want. Michal
On 04/19/2016 10:48 AM, Michal Privoznik wrote:
On 19.04.2016 16:38, John Ferlan wrote:
On 04/19/2016 09:50 AM, Michal Privoznik wrote:
Our uninstall script is not exact counterpart of install one. Therefore we are leaving couple of files behind. This should not happen.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/Makefile.am | 6 ++++++ 1 file changed, 6 insertions(+)
At 'install-data-local:', there's a :
$(mkinstalldirs) $(DESTDIR)$(HTML_DIR)
why not just the far more all encompassing:
rm -rf $(DESTDIR)$(HTML_DIR)
and
rm -rf $(DESTDIR)$(DEVHELP_DIR)
Rather than picking each part we install to uninstall? and missing something in the future or even now. Do the 'html' or 'internals' directories gets removed? And then of course the toplevel directory which we created.
IOW: There's no corollary for the:
$(mkinstalldirs) $(DESTDIR)$(HTML_DIR) $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/html $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/internals $(mkinstalldirs) $(DESTDIR)$(DEVHELP_DIR)
Yeah. That's the other way of doing that. It's just that if users put anything in $(DESTDIR)$(HTML_DIR) it will be removed by uninstall. But I can propose v2 if you want.
I see there are other 'rf -rm' usages in other "clean" labels... I don't have a strong feeling either way - perhaps there's other opinionated folks that would like to chime in. If no one chimes in, then I'm OK with what's here... I also now see there's "-rm " usages - so looks like it makes my comment in 1/4 unnecessary. It just looked strange to me... John
On Tue, Apr 19, 2016 at 10:57:25AM -0400, John Ferlan wrote:
On 04/19/2016 10:48 AM, Michal Privoznik wrote:
On 19.04.2016 16:38, John Ferlan wrote:
On 04/19/2016 09:50 AM, Michal Privoznik wrote:
Our uninstall script is not exact counterpart of install one. Therefore we are leaving couple of files behind. This should not happen.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/Makefile.am | 6 ++++++ 1 file changed, 6 insertions(+)
At 'install-data-local:', there's a :
$(mkinstalldirs) $(DESTDIR)$(HTML_DIR)
why not just the far more all encompassing:
rm -rf $(DESTDIR)$(HTML_DIR)
and
rm -rf $(DESTDIR)$(DEVHELP_DIR)
Rather than picking each part we install to uninstall? and missing something in the future or even now. Do the 'html' or 'internals' directories gets removed? And then of course the toplevel directory which we created.
IOW: There's no corollary for the:
$(mkinstalldirs) $(DESTDIR)$(HTML_DIR) $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/html $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/internals $(mkinstalldirs) $(DESTDIR)$(DEVHELP_DIR)
Yeah. That's the other way of doing that. It's just that if users put anything in $(DESTDIR)$(HTML_DIR) it will be removed by uninstall. But I can propose v2 if you want.
I see there are other 'rf -rm' usages in other "clean" labels...
I don't have a strong feeling either way - perhaps there's other opinionated folks that would like to chime in. If no one chimes in, then I'm OK with what's here...
rm -rf is fine with me and I believe with others as well, so ACK from me.
I also now see there's "-rm " usages - so looks like it makes my comment in 1/4 unnecessary. It just looked strange to me...
I think '-' means "don't error out if this fails", similarly to '@' meaning "Don't print the command being ran" IIRC.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 20.04.2016 16:37, Martin Kletzander wrote:
On Tue, Apr 19, 2016 at 10:57:25AM -0400, John Ferlan wrote:
On 04/19/2016 10:48 AM, Michal Privoznik wrote:
On 19.04.2016 16:38, John Ferlan wrote:
On 04/19/2016 09:50 AM, Michal Privoznik wrote:
Our uninstall script is not exact counterpart of install one. Therefore we are leaving couple of files behind. This should not happen.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/Makefile.am | 6 ++++++ 1 file changed, 6 insertions(+)
At 'install-data-local:', there's a :
$(mkinstalldirs) $(DESTDIR)$(HTML_DIR)
why not just the far more all encompassing:
rm -rf $(DESTDIR)$(HTML_DIR)
and
rm -rf $(DESTDIR)$(DEVHELP_DIR)
Rather than picking each part we install to uninstall? and missing something in the future or even now. Do the 'html' or 'internals' directories gets removed? And then of course the toplevel directory which we created.
IOW: There's no corollary for the:
$(mkinstalldirs) $(DESTDIR)$(HTML_DIR) $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/html $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/internals $(mkinstalldirs) $(DESTDIR)$(DEVHELP_DIR)
Yeah. That's the other way of doing that. It's just that if users put anything in $(DESTDIR)$(HTML_DIR) it will be removed by uninstall. But I can propose v2 if you want.
I see there are other 'rf -rm' usages in other "clean" labels...
I don't have a strong feeling either way - perhaps there's other opinionated folks that would like to chime in. If no one chimes in, then I'm OK with what's here...
rm -rf is fine with me and I believe with others as well, so ACK from me.
Thank you both guys. I've pushed these. Michal
participants (3)
-
John Ferlan -
Martin Kletzander -
Michal Privoznik