ctdb/wscript: Avoid installing pre-built man pages which does not exist during make

[Michael Adam]

On 2015-11-10 at 15:21 +1100, Martin Schwenke wrote:
> On Mon, 9 Nov 2015 15:52:27 +0100, Michael Adam <obnox at samba.org> wrote:
> 
> > On 2015-11-09 at 18:13 +0530, Anoop C S wrote:
> 
> > > When samba is compiled from source with-cluster-support, make fails
> > > with the following warning in the absence of xsltproc:
> > > 
> > > input file 'doc/ctdb.1' could not be found ('/root/samba/ctdb')
> > > 
> > > This is because in ctdb/wscript we try to install pre-built ctdb man
> > > pages which does not exist under ctdb/doc if XSLTPROC_MANPAGES is not
> > > set during configure as follows:
> > > 
> > > if 'XSLTPROC_MANPAGES' in bld.env and bld.env['XSLTPROC_MANPAGES']:
> > >     bld.MANPAGES('''onnode.1 ctdbd_wrapper.1 ctdbd.conf.5
> > >                  ctdb.7 ctdb-statistics.7 ctdb-tunables.7''',
> > >                  True)
> > > else:
> > >     for m in manpages:                                     
> > >         bld.SAMBA_GENERATOR(m,
> > >                          source=os.path.join("doc", m),
> > >                          target=m,
> > >                          rule='sed %s ${SRC} > ${TGT}' % (sed_cmdline))
> > >         bld.INSTALL_FILES('${MANDIR}/man%s' % m[-1], m)
> > > 
> > > On a fedora 23 machine docbook-style-xsl provides the necessary style
> > > sheets to convert xml docs to required formats.
> > > 
> > > Attached is a patch which reverts the commit 3ddd3514 which introduced
> > > this else part in ctdb/wscript. Many thanks to Michael who helped me in
> > > finding the root cause.  
> > 
> > Reviewed-by: me.
> > 
> > The reverted patch reads 'Install pre-built manpages ...'
> > But there are no pre-built manpages any more since
> > 
> > 56e1d523a538bbd2434a37415605fb6a23d39706
> > "doc: Do not keep the built version of manpages in version control"
> > (Oct 22, 2012)
> 
> There are pre-built manpages in a tarball.  I don't think a revert here
> is the right thing to do.  The original commit fixed a bug where the
> pre-built manpages for a tarball do not get installed.  It introduced a
> new bug that isn't very hard to fix.

Apparently we had not thought about the tarball
possibly containing pre-built manpages. :-)
We just wanted to fix the non-tarball build.

> Perhaps I'm being precious because it is my commit being reverted?

No offense meant. Your protest is completely justified.

> However, what about the attached patch instead?  It even handles the
> case where someone has done something strange and a subset of the
> pre-built manpages are available.  :-)

Your patch is definitely the right way to solve it.
Just one comment -- see below.

> From f9606c8ac2fc5881f14848973fd7167fc35b55cd Mon Sep 17 00:00:00 2001
> From: Martin Schwenke <martin at meltin.net>
> Date: Tue, 10 Nov 2015 10:14:56 +1100
> Subject: [PATCH] ctdb-build: Don't try to install unavailable prebuilt
>  manpages
> 
> Commit 3ddd35142ab86de431d00f93de2fb6a2b371317d tries to
> unconditionally install pre-built manpages if xsltproc is unavailable.
> However, these only exist in a tarball, not in a git repo.  That can
> make the installation fail.
> 
> If xsltproc does not exist then check for each pre-built manpage at
> configure time.  This should cover all the possible cases.
> 
> Signed-off-by: Martin Schwenke <martin at meltin.net>
> ---
>  ctdb/wscript | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/ctdb/wscript b/ctdb/wscript
> index b720748..38023d7 100755
> --- a/ctdb/wscript
> +++ b/ctdb/wscript
> @@ -227,6 +227,17 @@ def configure(conf):
>          conf.DEFINE('SAMBA_UTIL_CORE_ONLY', 1, add_to_cflags=True)
>          conf.SAMBA_CONFIG_H()
>  
> +    if 'XSLTPROC_MANPAGES' in conf.env and conf.env['XSLTPROC_MANPAGES']:
> +        conf.env.ctdb_prebuilt_manpages = None
> +    else:
> +        Logs.info("xsltproc unavailable, checking for pre-built manpages")
> +        conf.env.ctdb_prebuilt_manpages = []
> +        for m in manpages:
> +            if os.path.exists(os.path.join("doc", m)):
> +                Logs.info("  %s: yes" % (m))
> +                conf.env.ctdb_prebuilt_manpages.append(m)
> +            else:
> +                Logs.info("  %s: no" % (m))
>  
>  def build(bld):
>      if bld.env.standalone_ctdb:
> @@ -493,12 +504,12 @@ def build(bld):
>                              target=x,
>                              rule='sed %s ${SRC} > ${TGT}' % (sed_cmdline))
>  
> -    if 'XSLTPROC_MANPAGES' in bld.env and bld.env['XSLTPROC_MANPAGES']:
> +    if bld.env.ctdb_prebuilt_manpages is None:
>          bld.MANPAGES('''onnode.1 ctdbd_wrapper.1 ctdbd.conf.5
>                          ctdb.7 ctdb-statistics.7 ctdb-tunables.7''',
>                        True)
>      else:
> -        for m in manpages:
> +        for m in bld.env.ctdb_prebuilt_manpages:
>              bld.SAMBA_GENERATOR(m,
>                                  source=os.path.join("doc", m),
>                                  target=m,


I think it is too subtle and a little misleading to
have the xml-generation of the manpages conditionalized
by 'bld.env.ctdb_prebuilt_manpages is None'.

It is much more obvious to keep the line

"if 'XSLTPROC_MANPAGES' in bld.env and bld.env['XSLTPROC_MANPAGES']:"

You could then even put the loop for finding of the pre-installed
manpages into the else branch of that condition.

Otherwise this looks good to me.

Thanks - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151110/33518b85/signature.sig>