| Message ID | 20211016043948.2422-1-duncan_roe@optusnet.com.au |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [libnetfilter_log,v5] build: doc: Allow to specify whether to produce man pages, html, neither or both | expand |
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. > > Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au> > --- > 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 [...] Sorry Pablo, this is for libnetfilter_queue. I don't see it in patchwork - did you get rid of it already? Will re-send with correct Sj. Cheers ... Duncan.
Hi Pablo, On Sat, Oct 16, 2021 at 06:27:56PM +1100, Duncan Roe wrote: > 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. > > > > Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au> > > --- > > 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 > [...] > Sorry Pablo, this is for libnetfilter_queue. > I don't see it in patchwork - did you get rid of it already? > Will re-send with correct Sj. > Sorry again for the confusion but you dropped the good libnetfilter_log patch that was Tested-by: Jeremy Sowden and left the bad libnetfilter_log patch that actually applies to libnetfilter_queue. To save you the time to reinstate the dropped patch, I have re-sent it. It's in Patchwork - title is libnetfilter_log,v5,1/1] build: doc: `make` generates requested documentation Can you please post a comment why you have not applied this patch or the libnetfilter_queue corrresponding patch? Can I change anything to get you to apply them? Cheers ... Duncan.
On Sun, Oct 17, 2021 at 12:56:07PM +1100, Duncan Roe wrote: > Hi Pablo, > > On Sat, Oct 16, 2021 at 06:27:56PM +1100, Duncan Roe wrote: > > 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. > > > > > > Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au> > > > --- > > > 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 > > [...] > > Sorry Pablo, this is for libnetfilter_queue. > > I don't see it in patchwork - did you get rid of it already? > > Will re-send with correct Sj. > > > Sorry again for the confusion but you dropped the good libnetfilter_log patch > that was Tested-by: Jeremy Sowden and left the bad libnetfilter_log patch that > actually applies to libnetfilter_queue. Are you refering to this patch? https://patchwork.ozlabs.org/project/netfilter-devel/patch/20211017013951.12584-1-duncan_roe@optusnet.com.au/ This is the one that Jeremy added the Tested-by: tag, correct?
On Sun, Oct 17, 2021 at 03:41:46PM +0200, Pablo Neira Ayuso wrote: > On Sun, Oct 17, 2021 at 12:56:07PM +1100, Duncan Roe wrote: > > Hi Pablo, > > > > On Sat, Oct 16, 2021 at 06:27:56PM +1100, Duncan Roe wrote: > > > 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. > > > > > > > > Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au> > > > > --- > > > > 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 > > > [...] > > > Sorry Pablo, this is for libnetfilter_queue. > > > I don't see it in patchwork - did you get rid of it already? > > > Will re-send with correct Sj. > > > > > Sorry again for the confusion but you dropped the good libnetfilter_log patch > > that was Tested-by: Jeremy Sowden and left the bad libnetfilter_log patch that > > actually applies to libnetfilter_queue. > > Are you refering to this patch? > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20211017013951.12584-1-duncan_roe@optusnet.com.au/ > > This is the one that Jeremy added the Tested-by: tag, correct? Sorry Jeremy, this email is not for you obviosly, it is for Duncan. For some reason he is himself removed from replies.
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@optusnet.com.au> > --- > 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.
On Sun, Oct 17, 2021 at 03:41:46PM +0200, Pablo Neira Ayuso wrote: > On Sun, Oct 17, 2021 at 12:56:07PM +1100, Duncan Roe wrote: > > Hi Pablo, > > > > On Sat, Oct 16, 2021 at 06:27:56PM +1100, Duncan Roe wrote: > > > 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. > > > > > > > > Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au> > > > > --- > > > > 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 > > > [...] > > > Sorry Pablo, this is for libnetfilter_queue. > > > I don't see it in patchwork - did you get rid of it already? > > > Will re-send with correct Sj. > > > > > Sorry again for the confusion but you dropped the good libnetfilter_log patch > > that was Tested-by: Jeremy Sowden and left the bad libnetfilter_log patch that > > actually applies to libnetfilter_queue. > > Are you refering to this patch? > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20211017013951.12584-1-duncan_roe@optusnet.com.au/ > > This is the one that Jeremy added the Tested-by: tag, correct? Yes that's the one. I re-sent it
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@optusnet.com.au> > > --- > > 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.
On Mon, Oct 18, 2021 at 11:45:50AM +1100, Duncan Roe wrote: > On Sun, Oct 17, 2021 at 03:41:46PM +0200, Pablo Neira Ayuso wrote: > > On Sun, Oct 17, 2021 at 12:56:07PM +1100, Duncan Roe wrote: > > > Hi Pablo, > > > > > > On Sat, Oct 16, 2021 at 06:27:56PM +1100, Duncan Roe wrote: > > > > 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. > > > > > > > > > > Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au> > > > > > --- > > > > > 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 > > > > [...] > > > > Sorry Pablo, this is for libnetfilter_queue. > > > > I don't see it in patchwork - did you get rid of it already? > > > > Will re-send with correct Sj. > > > > > > > Sorry again for the confusion but you dropped the good libnetfilter_log patch > > > that was Tested-by: Jeremy Sowden and left the bad libnetfilter_log patch that > > > actually applies to libnetfilter_queue. > > > > Are you refering to this patch? > > > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20211017013951.12584-1-duncan_roe@optusnet.com.au/ > > > > This is the one that Jeremy added the Tested-by: tag, correct? > > Yes that's the one. I re-sent it To be unambiguous, the bad patch is https://patchwork.ozlabs.org/project/netfilter-devel/patch/20211016043948.2422-1-duncan_roe@optusnet.com.au/ If you would remove that, then patchwork will be all good. Cheers ... Duncan.
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)]) + +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)]) + AC_PROG_CC AM_PROG_CC_C_O AC_DISABLE_STATIC @@ -36,12 +52,10 @@ AC_CONFIG_FILES([Makefile src/Makefile utils/Makefile examples/Makefile doxygen/Makefile doxygen/doxygen.cfg include/linux/Makefile include/linux/netfilter/Makefile]) -AC_ARG_WITH([doxygen], [AS_HELP_STRING([--with-doxygen], - [create doxygen documentation])], - [with_doxygen="$withval"], [with_doxygen=yes]) +AS_IF([test "$enable_man_pages" = no -a "$enable_html_doc" = no], [with_doxygen=no], [with_doxygen=yes]) AS_IF([test "x$with_doxygen" != xno], [ - AC_CHECK_PROGS([DOXYGEN], [doxygen]) + AC_CHECK_PROGS([DOXYGEN], [doxygen], [""]) AC_CHECK_PROGS([DOT], [dot], [""]) AS_IF([test "x$DOT" != "x"], [AC_SUBST(HAVE_DOT, YES)], @@ -52,12 +66,18 @@ AM_CONDITIONAL([HAVE_DOXYGEN], [test -n "$DOXYGEN"]) AS_IF([test "x$DOXYGEN" = x], [ AS_IF([test "x$with_doxygen" != xno], [ dnl Only run doxygen Makefile if doxygen installed - AC_MSG_WARN([Doxygen not found - continuing without Doxygen support]) - with_doxygen=no + AC_MSG_WARN([Doxygen not found - no documentation will be built]) + enable_html_doc=no + enable_man_pages=no ]) +], [ + dnl Warn user if html docs will be missing diagrams + AS_IF([test "$enable_html_doc" = yes -a -z "$DOT"], + AC_MSG_WARN([Dot not found - install graphviz to get interactive diagrams in HTML])) ]) AC_OUTPUT echo " libnetfilter_queue configuration: - doxygen: ${with_doxygen}" +man pages: ${enable_man_pages} +html docs: ${enable_html_doc}" diff --git a/doxygen/Makefile.am b/doxygen/Makefile.am index 5a7fdd5..c6eeed7 100644 --- a/doxygen/Makefile.am +++ b/doxygen/Makefile.am @@ -14,7 +14,9 @@ doxyfile.stamp: $(doc_srcs) Makefile rm -rf html man doxygen doxygen.cfg >/dev/null +if BUILD_MAN $(abs_top_srcdir)/doxygen/build_man.sh +endif touch doxyfile.stamp @@ -24,13 +26,20 @@ all-local: doxyfile.stamp clean-local: rm -rf man html install-data-local: +if BUILD_MAN mkdir -p $(DESTDIR)$(mandir)/man3 cp --no-dereference --preserve=links,mode,timestamps man/man3/*.3\ $(DESTDIR)$(mandir)/man3/ +endif +if BUILD_HTML + mkdir -p $(DESTDIR)$(htmldir) + cp --no-dereference --preserve=links,mode,timestamps html/*\ + $(DESTDIR)$(htmldir) +endif # make distcheck needs uninstall-local uninstall-local: - rm -r $(DESTDIR)$(mandir) man html doxyfile.stamp + rm -rf $(DESTDIR)$(mandir) man html doxyfile.stamp $(DESTDIR)$(htmldir) endif EXTRA_DIST = build_man.sh diff --git a/doxygen/doxygen.cfg.in b/doxygen/doxygen.cfg.in index 99b5c90..14bd0cf 100644 --- a/doxygen/doxygen.cfg.in +++ b/doxygen/doxygen.cfg.in @@ -21,7 +21,8 @@ ALPHABETICAL_INDEX = NO SEARCHENGINE = NO GENERATE_LATEX = NO LATEX_CMD_NAME = latex -GENERATE_MAN = YES +GENERATE_MAN = @GEN_MAN@ +GENERATE_HTML = @GEN_HTML@ MAN_LINKS = YES HAVE_DOT = @HAVE_DOT@ DOT_TRANSPARENT = YES
- 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. Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au> --- 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(-)