GNOME Bugzilla – Bug 759823
build: install nm-settings-ifcfg-rh.5 man page conditionally
Last modified: 2016-01-23 15:52:22 UTC
Created attachment 317833 [details] [review] [PATCH] build: install nm-settings-ifcfg-rh.5 man page conditionally Only install nm-settings-ifcfg-rh.5 man page if the ifcfg-rh configuration plugin has been enabled. E.g. Debian doesn't enalble the ifcfg plugin, so the man page shouldn't be installed either.
maybe I miss something, but docbook_autogenerated_man_pages =»··\ »···nm-settings.5 docbook_autogenerated_man_pages =»··\ »···nm-settings-keyfile.5 should the second "=" be "=+" ? Doesn't this skip "nm-settings.5"? Otherwise lgtm
(In reply to Thomas Haller from comment #1) > maybe I miss something, but > > > docbook_autogenerated_man_pages =»··\ > »···nm-settings.5 > > docbook_autogenerated_man_pages =»··\ > »···nm-settings-keyfile.5 > > > > should the second "=" be "=+" ? Doesn't this skip "nm-settings.5"? > The second "=" is "+="? But maybe I first have an unconditional assignment docbook_autogenerated_man_pages = nm-settings-keyfile.5 and then later on if CONFIG_PLUGIN_IFCFG_RH docbook_autogenerated_man_pages += \ nm-settings-ifcfg-rh.5 endif
(In reply to Michael Biebl from comment #2) > (In reply to Thomas Haller from comment #1) > > should the second "=" be "=+" ? Doesn't this skip "nm-settings.5"? > > > > The second "=" is "+="? But maybe I don't see that. The current outcome of the patch is: docbook_autogenerated_man_pages = \ nm-settings.5 docbook_autogenerated_man_pages = \ nm-settings-keyfile.5 if CONFIG_PLUGIN_IFCFG_RH docbook_autogenerated_man_pages += \ nm-settings-ifcfg-rh.5 endif it should be: docbook_autogenerated_man_pages = \ nm-settings.5 \ nm-settings-keyfile.5 if CONFIG_PLUGIN_IFCFG_RH docbook_autogenerated_man_pages += \ nm-settings-ifcfg-rh.5 endif Anyway, one downside of this patch is that it also removes nm-settings-ifcfg-rh.5 from EXTRA_DIST, which I think it should not because you want to include the manual page in the source-tarball. Hence it also breaks INSTALL_PREGEN_MANPAGES configure.ac-# check for pregenerated manpages to be installed configure.ac-install_pregen_manpages=no configure.ac-if test "$enable_gtk_doc" != "yes" \ configure.ac- -a -f man/NetworkManager.conf.5 \ configure.ac- -a -f man/nm-settings.5 \ configure.ac- -a -f man/nm-settings-keyfile.5 \ configure.ac- -a -f man/nm-settings-ifcfg-rh.5 \ configure.ac- -a -f man/nmcli-examples.5 \ configure.ac- -a -f man/NetworkManager.8; then configure.ac- install_pregen_manpages=yes configure.ac-fi configure.ac:AM_CONDITIONAL(INSTALL_PREGEN_MANPAGES, test "x${install_pregen_manpages}" = "xyes") Probably we still want to generate nm-settings-ifcfg-rh, except we don't want to install it. Dunno how to fix that properly.
(In reply to Thomas Haller from comment #3) > (In reply to Michael Biebl from comment #2) > > (In reply to Thomas Haller from comment #1) > > > > should the second "=" be "=+" ? Doesn't this skip "nm-settings.5"? > > > > > > > The second "=" is "+="? But maybe > > > > I don't see that. The current outcome of the patch is: > > > docbook_autogenerated_man_pages = \ > nm-settings.5 > > docbook_autogenerated_man_pages = \ > nm-settings-keyfile.5 Hm, indeed. Sorry for that. I don't actually know why I split that up for no reason. That should have been docbook_autogenerated_man_pages = \ nm-settings.5 > if CONFIG_PLUGIN_IFCFG_RH > docbook_autogenerated_man_pages += \ > nm-settings-ifcfg-rh.5 > endif > > > it should be: > > docbook_autogenerated_man_pages = \ > nm-settings.5 \ > nm-settings-keyfile.5 > > if CONFIG_PLUGIN_IFCFG_RH > docbook_autogenerated_man_pages += \ > nm-settings-ifcfg-rh.5 > endif > > > > > > > Anyway, one downside of this patch is that it also removes > nm-settings-ifcfg-rh.5 from EXTRA_DIST, which I think it should not because > you want to include the manual page in the source-tarball. > Hence it also breaks INSTALL_PREGEN_MANPAGES > > configure.ac-# check for pregenerated manpages to be installed > configure.ac-install_pregen_manpages=no > configure.ac-if test "$enable_gtk_doc" != "yes" \ > configure.ac- -a -f man/NetworkManager.conf.5 \ > configure.ac- -a -f man/nm-settings.5 \ > configure.ac- -a -f man/nm-settings-keyfile.5 \ > configure.ac- -a -f man/nm-settings-ifcfg-rh.5 \ > configure.ac- -a -f man/nmcli-examples.5 \ > configure.ac- -a -f man/NetworkManager.8; then > configure.ac- install_pregen_manpages=yes > configure.ac-fi > configure.ac:AM_CONDITIONAL(INSTALL_PREGEN_MANPAGES, test > "x${install_pregen_manpages}" = "xyes") > > > > > Probably we still want to generate nm-settings-ifcfg-rh, except we don't > want to install it. Dunno how to fix that properly.
grr, something odd happened. Wanted to say, this should have been: docbook_autogenerated_man_pages =↦ \ ↦ nm-settings.5↦ ↦ ↦ \ ↦ nm-settings-keyfile.5 if CONFIG_PLUGIN_IFCFG_RH docbook_autogenerated_man_pages +=↦ \ ↦ nm-settings-ifcfg-rh.5 endif As for the dist tarball, this is a valid point. But since you have --enable-ifcfg-rh in DISTCHECK_CONFIGURE_FLAGS in Makefile.am, is this an issue in practice?
Created attachment 318229 [details] [review] [PATCH] build: install nm-settings-ifcfg-rh.5 man page conditionally
(In reply to Michael Biebl from comment #5) > As for the dist tarball, this is a valid point. > > But since you have > --enable-ifcfg-rh > in DISTCHECK_CONFIGURE_FLAGS in Makefile.am, is this an issue in practice? ./autogen.sh --enable-gtk-doc make make dist does not contain nm-settings-ifcfg-rh.5. I think that is bad. It's already annoying that before doing `make dist` you must explicitly configure with --enable-gtk-doc and explicitly `make`. If I'd know how to fix that, I'd do it. Does anybody know how to achieve that nm-settings-ifcfg-rh.5 doesn't get installed (but build and dist'ed)? noinst_man_MANS didn't work for me.
It's going to be a hack. But something like this should do the trick: EXTRA_MANPAGES= if CONFIG_PLUGIN_IFCFG_RH docbook_autogenerated_man_pages += \ nm-settings-ifcfg-rh.5 else EXTRA_MANPAGES+=nm-settings-ifcfg-rh.5 nm-settings-ifcfg-rh.5: nm-settings-ifcfg-rh.xml $(AM_V_GEN) xsltproc $(XSLTPROC_MAN_FLAGS) $< all-am: Makefile $(MANS) nm-settings-ifcfg-rh.5 endif and then add EXTRA_MANPAGES to EXTRA_DIST and CLEANFILES. This works because you can override Automake targets. So if Automake ever changes 'all-am' we're hosed, but it works for now.
What about the following patch, looks less hackish to me: diff --git a/man/Makefile.am b/man/Makefile.am index 85b6f3a..be33eeb 100644 --- a/man/Makefile.am +++ b/man/Makefile.am @@ -78,8 +78,13 @@ docbook_generated_man_pages = \ docbook_autogenerated_man_pages = \ nm-settings.5 \ - nm-settings-keyfile.5 \ - nm-settings-ifcfg-rh.5 + nm-settings-keyfile.5 + +if CONFIG_PLUGIN_IFCFG_RH +docbook_autogenerated_man_pages += nm-settings-ifcfg-rh.5 +else +EXTRA_DIST += nm-settings-ifcfg-rh.5 +endif EXTRA_DIST += \ nm-settings.xml \
merged patch from comment 9 to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=b72cdb75d8a313adeeb09dc695ad3955cf2f0759 it tests correctly for me: ./autogen.sh --prefix=/tmp/NM --enable-gtk-doc --disable-ifcfg-rh make make dist then the source-tarball contains "man/nm-settings-ifcfg-rh.5". but nm-settings-ifcfg-rh.5 is not installed.