Hi Pablo, On Sun, Oct 17, 2021 at 03:55:59PM +0200, Pablo Neira Ayuso wrote: > Hi Duncan, > > On Sat, Oct 16, 2021 at 03:39:48PM +1100, Duncan Roe wrote: > > - configure --help lists non-default documentation options. > > Looking around the web, this seemed to me to be what most projects do. > > Listed options are --enable-html-doc & --disable-man-pages. > > - --with-doxygen is removed: --disable-man-pages also disables doxygen unless > > --enable-html-doc is asserted. > > If html is requested, `make install` installs it in htmldir. > > A few comments below. > > > Signed-off-by: Duncan Roe <duncan_roe@xxxxxxxxxxxxxxx> > > --- > > v2: broken out from 0001-build-doc-Fix-man-pages.patch > > v3: no change (still part of a series) > > v4: remove --without-doxygen since -disable-man-pages does that > > v5: - update .gitignore for clean `git status` after in-tree build > > - in configure.ac: > > - ensure all variables are always set (avoid leakage from environment) > > - provide helpful warning if HTML enabled but dot not found > > .gitignore | 5 ++++- > > configure.ac | 34 +++++++++++++++++++++++++++------- > > doxygen/Makefile.am | 11 ++++++++++- > > doxygen/doxygen.cfg.in | 3 ++- > > 4 files changed, 43 insertions(+), 10 deletions(-) > > > > diff --git a/.gitignore b/.gitignore > > index 525628e..ae3e740 100644 > > --- a/.gitignore > > +++ b/.gitignore > > @@ -15,7 +15,10 @@ Makefile.in > > /libtool > > /stamp-h1 > > > > -/doxygen.cfg > > +/doxygen/doxygen.cfg > > /libnetfilter_queue.pc > > > > /examples/nf-queue > > +/doxygen/doxyfile.stamp > > +/doxygen/html/ > > +/doxygen/man/ > > diff --git a/configure.ac b/configure.ac > > index 4721eeb..83959b0 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -13,6 +13,22 @@ m4_ifdef([AM_PROG_AR], [AM_PROG_AR]) > > dnl kernel style compile messages > > m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])]) > > > > +AC_ARG_ENABLE([html-doc], > > + AS_HELP_STRING([--enable-html-doc], [Enable html documentation]), > > + [], [enable_html_doc=no]) > > +AM_CONDITIONAL([BUILD_HTML], [test "$enable_html_doc" = yes]) > > +AS_IF([test "$enable_html_doc" = yes], > > + [AC_SUBST(GEN_HTML, YES)], > > + [AC_SUBST(GEN_HTML, NO)]) > > Is this changing defaults in some way? If so, this needs to be > documented in the patch descriptions. > > + > > +AC_ARG_ENABLE([man-pages], > > + AS_HELP_STRING([--disable-man-pages], [Disable man page documentation]), > > + [], [enable_man_pages=yes]) > > +AM_CONDITIONAL([BUILD_MAN], [test "$enable_man_pages" = yes]) > > +AS_IF([test "$enable_man_pages" = yes], > > + [AC_SUBST(GEN_MAN, YES)], > > + [AC_SUBST(GEN_MAN, NO)]) > > Why do we need two new toggles for this? > > In both cases, doxygen is required to build the manpages, so the point > of the documentation toggle is to allow to build the software in the > absence of doxygen (for example, in a embedded setup). > > I'm not ambivalent this is really needed, if it might be useful but an > explaination would be good to have to explain the rationale behind > this update. > > Thanks. v6 has a re-worked commit message which addresses all your points above, or if it doesn't then please ask for clarification. I split off the .gitignore changes - they apply by themselves. Cheers ... Duncan.
