<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN"> <HTML> <HEAD> <META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8"> <META NAME="GENERATOR" CONTENT="GtkHTML/3.26.0"> </HEAD> <BODY> On Sun, 2010-01-03 at 18:16 +0100, Matthieu Herrb wrote: <BLOCKQUOTE TYPE=CITE> <PRE> Hi and best wishes for 2010, There are duplicated checks for xmlto in more and more X.Org packages. I've 2 problems with that: - the 1st one is just code duplication - the 2nd one is that the code currently used doesn't allow to ignore an installed xmlto package. I want this because the base OpenBSD system doen't have xmlto (and other systems also don't have it) yet I want to have consistant X builds, wether or not xmlto is installed as a 3rd party package. </PRE> </BLOCKQUOTE> I have noticed the use of the logic: "if the tool exist, then build it, otherwise ignore it". I had on my long-term todo list to review those behaviours. There is an on-going effort to move the specs away from xorg-docs into their respective modules. This will add the number of modules building documentation. Before reviewing th details of patches, I'd like to review the desired behaviours. Adding a macro to util-macros is exposing it to 250 modules. It must have the potential of being useful for many of those. Let' see how this plays out. The module, say xfs, decides if it wants to build doc or not. The default (auto) is to build the doc if the tool is present. If xmlto is not found a warning is issued and the makefile completes successfully. If the builder specifies with-xmlto=no, a warning is issued and XMLTO is unset. Is the warning necessary? The builder asked not to build. Is it necessary (or even wise)  to unset the variable set by AC_PATH_PROG? The makefile completes successfully. If the builder specified -with=yes,  and the tool is not found, should it not issue an error and stop? You specifically asked to build it. For a developer, it is not important, but for an automated build machine, it needs to stop when something goes wrong, however it happens. I wanted to spell out the functions the macro provide to see if they are applicable to other modules as well. I agree with the problem you raised and I am trying to apply your solution to more than a few modules. One suggestion: would it be useful to factor out the check for xmlto in a macro (XORG_CHECK_XMLTO)? This would allow other modules to make their own decision about what to do when xmlto is not available. Look at XORG_CHECK_DOCBOOK. There are other tools used to build docs, so I can foresee other macros based on this one. Some comments on the patch itself - The macro should be minimum version 1.5 - The comment is confusing: XORG_CHECK_XMLTO or XORH_WITH_XMLTO? - The comment should explain how to/why use the macro Somewhat unrelated: I installed OpenBSD in a virtualBox. I haven't hunt down the toolchain yet. It's be nice to list the tools that are or are not available in this wiki: <A HREF="http://wiki.x.org/wiki/RequiredPackages.">http://wiki.x.org/wiki/RequiredPackages.</A> I did it for Ubuntu and OpenSolaris. <BLOCKQUOTE TYPE=CITE> <PRE> So I'm proposing to add a generic test to xorg-macros.m4 and replace the 3 lines that are found in app/xfs, lib/libXcomposite, lib/libXfont, lib/libXi and lib/libXtst by XORG_WITH_XMLTO: like this: --- a/configure.ac +++ b/configure.ac @@ -46,9 +46,7 @@ AC_CANONICAL_HOST XORG_DEFAULT_OPTIONS # xmlto is used to convert doc/design.xml from DocBook to PDF/HTML -AC_ARG_VAR([XMLTO], [Path to xmlto command]) -AC_PATH_PROG([XMLTO], [xmlto]) -AM_CONDITIONAL([HAVE_XMLTO], [test "x$XMLTO" != "x"]) +XORG_WITH_XMLTO AC_CHECK_HEADERS([stdint.h]) >From 05a9995d9679a72948d56c3bbae7b3ed04374c57 Mon Sep 17 00:00:00 2001 >From: Matthieu Herrb <<A HREF="mailto:matthieu.herrb@laas.fr">matthieu.herrb@laas.fr</A>> Date: Sun, 3 Jan 2010 18:04:25 +0100 Subject: [utils/macros] Add XORG_WITH_XMLTO to factorize xmlto tests. This also allow to configure with --without-xmlto to ignore a 3rd party xmlto tool on systems that normally don't have it, in order to have reproducable builds. Signed-off-by: Matthieu Herrb <<A HREF="mailto:matthieu.herrb@laas.fr">matthieu.herrb@laas.fr</A>> --- xorg-macros.m4.in | 26 ++++++++++++++++++++++++++ 1 files changed, 26 insertions(+), 0 deletions(-) diff --git a/xorg-macros.m4.in b/xorg-macros.m4.in index 20d0c70..614f649 100644 --- a/xorg-macros.m4.in +++ b/xorg-macros.m4.in @@ -310,6 +310,32 @@ AC_SUBST(MAKE_PDF) AC_SUBST(MAKE_HTML) ]) # XORG_CHECK_DOCBOOK +# XORG_CHECK_XMLTO +# ---------------- +# Minimum version: 1.4.2 +# +# xmlto checks +AC_DEFUN([XORG_WITH_XMLTO],[ +AC_ARG_VAR([XMLTO], [Path to xmlto command]) +AC_ARG_WITH(xmlto, + AS_HELP_STRING([--with-xmlto], + [Use xmlto to regenerate documentation (default: auto)]), + [use_xmlto=$withval], [use_xmlto=auto]) + +if ! test "x$use_xmlto" = x"no" ; then + AC_PATH_PROG([XMLTO], [xmlto]) + if test "x$XMLTO" = "x" -a ! -f $srcdir/man/Xcomposite.man ; then + AC_MSG_WARN([xmlto not found - cannot create man pages without it]) + fi +else + if ! test "x$XMLTO" = "x"; then + AC_MSG_WARN([ignoring XMLTO environment variable since --with-xmlto=no was specified]) + unset XMLTO + fi +fi +AM_CONDITIONAL([HAVE_XMLTO], [test "x$XMLTO" != "x"]) +]) # XORG_CHECK_XMLTO + # XORG_CHECK_MALLOC_ZERO # ---------------------- # Minimum version: 1.0.0 -- 1.6.5.3 </PRE> </BLOCKQUOTE> </BODY> </HTML>