Bug 59807

Created attachment 73670 [details] [review]
replace xmlto by xsltproc for cmake build system

Created attachment 73671 [details] [review]
support platform related docbook parts for cmake

(In reply to comment #0)
> docbook provides conditional text which could be used to hide platform
> unrelated stuff - see http://www.sagehill.net/docbookxsl/Profiling.html for
> more informations. 

I don't want this to happen. I think our documentation for a particular component should document it for every platform: if a particular option or whatever doesn't exist or make sense on a platform, the documentation should just say "This option is only supported on Unix platforms" or something.

I make D-Bus releases on Linux. I'm sure you don't want the HTML documentation I upload to dbus.freedesktop.org during each release to leave out any Windows-specific bits, just because I happened to be using Linux to do the build?

> Recently docbpook generation process uses xmlto on linux (autotools, cmake)
> and kde meinproc4 on windows (because of the lack of xmlto on windows[1])

Here is my order of preference:

* use xmlto on every platform (unfortunately not achievable, because it's
  not available on Windows)

* try to use xmlto and/or meinproc, in one order or the other; if we
  don't have either, don't build documentation

* (lowest preference) invent our own clone of xmlto

I think it's OK for the troff man pages to not be built when using meinproc, if xmlto is "higher priority" than meinproc: we only really support meinproc for the benefit of native Windows builds, and Windows users have a HTML viewer but no man page viewer.

Comment on attachment 73670 [details] [review]
replace xmlto by xsltproc for cmake build system

Review of attachment 73670 [details] [review]:
-----------------------------------------------------------------

I think using xmlto and meinproc is better than reinventing them, even if it means the build system needs to include both.

In one of your cross-compiling patches, you had a patch band changing this (pseudocode):

    if (have meinproc)
        use meinproc
    endif

    if (have xmlto)
        use xmlto
    endif

to this:

    if (have xmlto)
        use xmlto
    else
        use meinproc
    endif

Could you please do a patch containing only that? I think that's the right change: the previous code wasn't right if you have both xmlto *and* meinproc.

::: cmake/doc/CMakeLists.txt
@@ +4,5 @@
>  find_package(Doxygen)
>  
> +add_custom_target(doc ALL)
> +
> +if (DOXYGEN_EXECUTABLE)

Please don't make random changes to whitespace in an unrelated patch, it makes it a mess to review. If you want to change the whitespace convention in the cmake build system, do that by having patches that *only* change the whitespace.

::: cmake/modules/FindDocBookXSL.cmake
@@ +1,1 @@
> +# Try to find DocBook XSL stylesheet

This looks like the sort of file that's going to attract portability problems. I would prefer our interface to the Docbook XSL to be "run xmlto with its documented command-line options", "run meinproc with its documented command-line options".

(In reply to comment #4)

> In one of your cross-compiling patches, you had a patch band changing this
> (pseudocode):
> 
>     if (have meinproc)
>         use meinproc
>     endif
> 
>     if (have xmlto)
>         use xmlto
>     endif
> 
> to this:
> 
>     if (have xmlto)
>         use xmlto
>     else
>         use meinproc
>     endif
> 
> Could you please do a patch containing only that? I think that's the right
> change: the previous code wasn't right if you have both xmlto *and* meinproc.

This has been fixed in bug 59733

(In reply to comment #5)
> This has been fixed in bug 59733

OK, great. I think the rest is WONTFIX, as I said in Comment #2. Patches to make the documentation more comprehensive would be great, but I'd prefer it to say "On Unix... On Windows..." rather than only including one or the other in the build.

One exception is that dbus-launch seems to have different usage and command-line syntax on Unix and Windows. At the moment, dbus-launch.1.xml specifically documents Unix dbus-launch, so if you want to provide a separate dbus-launch-win.1.xml that gets installed on Windows, please do.