Bug 59805 - Get rid of troff doc sources
Summary: Get rid of troff doc sources
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Ralf Habacker
QA Contact:
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-01-24 13:23 UTC by Ralf Habacker
Modified: 2013-04-19 11:40 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Moved docbook source used by cmake to doc subdir (161.23 KB, patch)
2013-02-05 00:44 UTC, Ralf Habacker
Details | Splinter Review
Moved docbook source used by cmake to doc subdir (107.87 KB, patch)
2013-02-05 00:56 UTC, Ralf Habacker
Details | Splinter Review
Moved docbook source used by cmake to doc subdir (98.83 KB, patch)
2013-02-05 01:10 UTC, Ralf Habacker
Details | Splinter Review
Moved docbook source used by cmake to doc subdir (fixed install target) (99.78 KB, patch)
2013-02-05 02:18 UTC, Ralf Habacker
Details | Splinter Review
Updated man docbook xml sources from man page source using doclifter. (58.28 KB, patch)
2013-02-05 02:19 UTC, Ralf Habacker
Details | Splinter Review
Generate Man pages from docbook sources for cmake buildsystem (2.99 KB, patch)
2013-02-05 02:21 UTC, Ralf Habacker
Details | Splinter Review
Fixed tweaks when generating man pages from docbook sources. (2.97 KB, patch)
2013-02-05 06:36 UTC, Ralf Habacker
Details | Splinter Review
Generate man pages from xml docbook sources for cmake buildsystem. (3.03 KB, patch)
2013-02-12 15:57 UTC, Simon McVittie
Details | Splinter Review
Fill in a manual and source for all man pages (3.28 KB, patch)
2013-02-12 15:57 UTC, Simon McVittie
Details | Splinter Review
Fill in a manual and source for all man pages (v2) (3.29 KB, patch)
2013-02-12 16:04 UTC, Simon McVittie
Details | Splinter Review
Use Docbook XML as the source for all man pages (53.79 KB, patch)
2013-02-12 16:04 UTC, Simon McVittie
Details | Splinter Review
Remove doclifter "signature" from Docbook man pages' source (3.83 KB, patch)
2013-02-14 13:44 UTC, Simon McVittie
Details | Splinter Review
Eliminate unwanted whitespace from the man pages' XML source (11.89 KB, patch)
2013-02-14 13:44 UTC, Simon McVittie
Details | Splinter Review
Fixed different docbook versions (2.31 KB, patch)
2013-02-22 15:34 UTC, Ralf Habacker
Details | Splinter Review
Unify docbook dtd version to 4.4 (2.30 KB, patch)
2013-02-22 23:41 UTC, Ralf Habacker
Details | Splinter Review

Description Ralf Habacker 2013-01-24 13:23:46 UTC
The dbus doc generation process currently uses two different doc format sources: docbook and troff+man.

Because of the fact that man pages could be easily generated from docbook too, migrating troff sources to docbook reduces the number of required build dependencies and eases doc maintaining. 

The cmake build system running on windows already uses docbook only sources and generates html man pages. 

The task here is to convert current troff sources to docbook with the help of doclifter and to adapt the cmake and autotools based buildsystem to use docbook sources for generating man pages on linux.
Comment 1 Simon McVittie 2013-01-24 17:00:45 UTC
AIUI, the dbus/doc Bugzilla component was a now-abandoned effort to define an embedded documentation format for D-Bus introspection data, similar to the one used in Telepathy. Documentation of dbus.git is dbus/core; reassigning there so I get bugmail.
Comment 2 Ralf Habacker 2013-01-26 12:07:22 UTC
(In reply to comment #1)
> AIUI, the dbus/doc Bugzilla component was a now-abandoned effort to define
> an embedded documentation format for D-Bus introspection data, similar to
> the one used in Telepathy. Documentation of dbus.git is dbus/core;
> reassigning there so I get bugmail.

Looks like that the component definition at https://bugs.freedesktop.org/describecomponents.cgi?product=dbus is somehow misleading
...
doc   Rob Taylor 	
      Documentation format and generation tools 
...
Comment 3 Simon McVittie 2013-01-28 11:33:25 UTC
(In reply to comment #2)
> doc   Rob Taylor 	
>       Documentation format and generation tools 

The description is vague, but I believe this subproject was meant to be: define a documentation format that can be embedded in introspection XML (like the one we use in Telepathy); write tools that generate HTML and/or Docbook from the introspection XML. It didn't happen.

Regardless, I don't get bugmail for dbus/doc, so please avoid it if you want your code to get reviewed :-)

We should probably either get this Bugzilla component removed, or repurpose it for what you thought it meant.
Comment 4 Ralf Habacker 2013-01-31 20:03:35 UTC
(In reply to comment #0)
> The task here is to convert current troff sources to docbook with the help
> of doclifter and to adapt the cmake and autotools based buildsystem to use
> docbook sources for generating man pages on linux.

Simon, how would you like to get the required patches to be set up or to be splitted or ... ?
Comment 5 Simon McVittie 2013-02-01 09:34:34 UTC
(In reply to comment #4)
> Simon, how would you like to get the required patches to be set up or to be
> splitted or ... ?

I think something like this would be easiest to review:

1) make dbus-daemon.1.in "configuration-neutral" (no @SUBSTITUTION@ tags),
   perhaps using the wording I suggested on another bug

2) move cmake/bus/dbus-daemon.xml, cmake/tools/*.xml to doc/*.xml,
   without changing their contents; adjust cmake build system to compensate

3) use doclifter to update doc/*.xml from *.1 and dbus-daemon.1.in

4) manual check that man pages and HTML generated from doc/*.xml
   look basically OK; tweak them if necessary

5) make Autotools build system treat doc/*.xml as source, and remove *.1,
   *.1.in from git

where I assume you probably want me to do (5) since it touches Autotools.
Comment 6 Ralf Habacker 2013-02-04 10:05:08 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > Simon, how would you like to get the required patches to be set up or to be
> > splitted or ... ?
> 
> I think something like this would be easiest to review:
> 
> 1) make dbus-daemon.1.in "configuration-neutral" (no @SUBSTITUTION@ tags),
>    perhaps using the wording I suggested on another bug

Could we agree to move this content related change from this bug to bug 59808, which will be proceeded after the doc process migration ?
> 
> 2) move cmake/bus/dbus-daemon.xml, cmake/tools/*.xml to doc/*.xml,
>    without changing their contents; adjust cmake build system to compensate
> 
> 3) use doclifter to update doc/*.xml from *.1 and dbus-daemon.1.in
> 
> 4) manual check that man pages and HTML generated from doc/*.xml
>    look basically OK; tweak them if necessary
> 
> 5) make Autotools build system treat doc/*.xml as source, and remove *.1,
>    *.1.in from git
> 
> where I assume you probably want me to do (5) since it touches Autotools.
That would be nice, I can do the other tasks.
Comment 7 Simon McVittie 2013-02-04 10:51:51 UTC
(In reply to comment #6)
> Could we agree to move this content related change from this bug to bug
> 59808, which will be proceeded after the doc process migration ?

At the moment, the documentation source is a generated file which contains build-specific information. I would rather not have to use AC_SUBST (and cmake's equivalent) for the XML version too... I have already provided some suggested OS-neutral wording on Bug #59808, which I would be happy with. My preferred course of action would be to apply the change from Bug #59808 to the troff source as step 1 of this bug.

If you insist on doing things in the other order, instead of bus/dbus-daemon.xml you'll need to have a dbus-daemon.xml.in (which is included in tarballs, and is the real source), and bus/dbus-daemon.xml (which is not included in tarballs, and is generated from dbus-daemon.xml either via AC_SUBST + AC_CONFIG_FILES or a makefile rule in Autotools, and via CONFIGURE_FILE in CMake). Then, Bug #59808 will just simplify that to what I suggested above...

The first way seems a lot simpler to me, and doesn't require you to understand Autotools, but, whatever; if you want to make extra work for yourself, the second way can work too.
Comment 8 Ralf Habacker 2013-02-05 00:44:53 UTC
Created attachment 74208 [details] [review]
Moved docbook source used by cmake to doc subdir
Comment 9 Ralf Habacker 2013-02-05 00:48:03 UTC
Comment on attachment 74208 [details] [review]
Moved docbook source used by cmake to doc subdir

This patch includes unwanted copies of xml files and will be updated soon.
Comment 10 Ralf Habacker 2013-02-05 00:56:13 UTC
Created attachment 74209 [details] [review]
Moved docbook source used by cmake to doc subdir
Comment 11 Ralf Habacker 2013-02-05 01:10:16 UTC
Created attachment 74210 [details] [review]
Moved docbook source used by cmake to doc subdir

fixup
Comment 12 Ralf Habacker 2013-02-05 02:18:17 UTC
Created attachment 74212 [details] [review]
Moved docbook source used by cmake to doc subdir (fixed install target)
Comment 13 Ralf Habacker 2013-02-05 02:19:11 UTC
Created attachment 74213 [details] [review]
Updated man docbook xml sources from man page source using doclifter.
Comment 14 Ralf Habacker 2013-02-05 02:21:56 UTC
Created attachment 74214 [details] [review]
Generate Man pages from docbook sources for cmake buildsystem
Comment 15 Ralf Habacker 2013-02-05 06:36:49 UTC
Created attachment 74216 [details] [review]
Fixed tweaks when generating man pages from docbook sources.
Comment 16 Ralf Habacker 2013-02-07 20:12:26 UTC
(In reply to comment #7)

> if you want to make extra work for
> yourself, the second way can work too.

appended patches uses the second way.
Comment 17 Ralf Habacker 2013-02-07 20:27:45 UTC
(In reply to comment #5)

> 5) make Autotools build system treat doc/*.xml as source, and remove *.1,
>    *.1.in from git
> 
> where I assume you probably want me to do (5) since it touches Autotools.
(In reply to comment #5)
> (In reply to comment #4)
> > Simon, how would you like to get the required patches to be set up or to be
> > splitted or ... ?
> 
> I think something like this would be easiest to review:
> 
> 1) make dbus-daemon.1.in "configuration-neutral" (no @SUBSTITUTION@ tags),
>    perhaps using the wording I suggested on another bug
> 
> 2) move cmake/bus/dbus-daemon.xml, cmake/tools/*.xml to doc/*.xml,
>    without changing their contents; adjust cmake build system to compensate
> 
> 3) use doclifter to update doc/*.xml from *.1 and dbus-daemon.1.in
> 
> 4) manual check that man pages and HTML generated from doc/*.xml
>    look basically OK; tweak them if necessary

1 .. 4 are done with the appended patches. Do you have any time to review them ? 

> 5) make Autotools build system treat doc/*.xml as source, and remove *.1,
>    *.1.in from git
> 
> where I assume you probably want me to do (5) since it touches Autotools.

Do you have gotten any time in this area ? May be i can assist you by giving a try ?
Comment 18 Simon McVittie 2013-02-12 15:13:29 UTC
Comment on attachment 74212 [details] [review]
Moved docbook source used by cmake to doc subdir (fixed install target)

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

Looks OK if someone (probably me) adds the moved files to the Automake build system.
Comment 19 Simon McVittie 2013-02-12 15:15:32 UTC
Comment on attachment 74213 [details] [review]
Updated man docbook xml sources from man page source using doclifter.

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

Looks OK, we can clean up any non-ideal rendering afterwards.
Comment 20 Simon McVittie 2013-02-12 15:17:20 UTC
Comment on attachment 74214 [details] [review]
Generate Man pages from docbook sources for cmake buildsystem

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

::: cmake/doc/CMakeLists.txt
@@ +70,4 @@
>  			WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>  		)
>  	  endif ()
> +	 # add_dependencies(xmldoc ${_outname})

Why is this line commented out? Either it's necessary, in which case it shouldn't be commented, or it isn't, in which case please delete it.
Comment 21 Simon McVittie 2013-02-12 15:17:52 UTC
Comment on attachment 74216 [details] [review]
Fixed tweaks when generating man pages from docbook sources.

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

I suppose so.
Comment 22 Simon McVittie 2013-02-12 15:57:00 UTC
Created attachment 74691 [details] [review]
Generate man pages from xml docbook sources for cmake  buildsystem.

From: Ralf Habacker <ralf.habacker@freenet.de>
Date: Tue, 5 Feb 2013 03:10:59 +0100
Subject: [PATCH 3/5] Generate man pages from xml docbook sources for cmake
 buildsystem.

[removed commented line -smcv]
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=59805
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

---

Slightly modified version of Attachment #74214 [details]. I'd be OK with applying this one if you are.
Comment 23 Simon McVittie 2013-02-12 15:57:48 UTC
Created attachment 74693 [details] [review]
Fill in a manual and source for all man pages

I only filled in a version for dbus-daemon, whose XML is already
generated by configure: it doesn't seem worth making all the other
man pages into .in files just to get a version number substituted.

---

This shuts up xmlto, and I think it's a better solution for that than Attachment #74216 [details].
Comment 24 Simon McVittie 2013-02-12 16:04:07 UTC
Created attachment 74695 [details] [review]
Fill in a manual and source for all man pages (v2)

I only filled in a version for dbus-daemon, whose XML is already
generated by configure: it doesn't seem worth making all the other
man pages into .in files just to get a version number substituted.

---

v2: use @DBUS_VERSION@ rather than @VERSION@, which hopefully works for CMake too?
Comment 25 Simon McVittie 2013-02-12 16:04:45 UTC
Created attachment 74696 [details] [review]
Use Docbook XML as the source for all man pages

This means we no longer need man2html, which is nice.
Comment 26 Ralf Habacker 2013-02-13 10:24:23 UTC
(In reply to comment #25)
> Created attachment 74696 [details] [review] [review]
> Use Docbook XML as the source for all man pages

compiled with autotools on opensuse without any problem, looks good. 

> 
> This means we no longer need man2html, which is nice.

yeah
Comment 27 Ralf Habacker 2013-02-13 10:51:59 UTC
(In reply to comment #24)
> Created attachment 74695 [details] [review] [review]
> Fill in a manual and source for all man pages (v2)
> 
> I only filled in a version for dbus-daemon, whose XML is already
> generated by configure: it doesn't seem worth making all the other
> man pages into .in files just to get a version number substituted.
> 
> ---
> 
> v2: use @DBUS_VERSION@ rather than @VERSION@, which hopefully works for
> CMake too?

yes
Comment 28 Ralf Habacker 2013-02-13 10:56:12 UTC
(In reply to comment #23)
> Created attachment 74693 [details] [review] [review]
> Fill in a manual and source for all man pages
> 
> I only filled in a version for dbus-daemon, whose XML is already
> generated by configure: 

which prints the folloing man page bottom line
D-Bus 1.6.255             02/13/2013         DBUS-DAEMON(1)

you can see for which dbus version this man page is indented for 

> it doesn't seem worth making all the other
> man pages into .in files just to get a version number substituted.
> 
> 
I think the other man pages would benefit also from having a version. 


> 
> This shuts up xmlto, and I think it's a better solution for that than
> Attachment #74216 [details].

looks good except that the patch file have a parse error. 

I would correct this and commit all patches with a related reviewed-by line if you like.
Comment 29 Simon McVittie 2013-02-13 12:47:02 UTC
(In reply to comment #27)
> > v2: use @DBUS_VERSION@ rather than @VERSION@, which hopefully works for
> > CMake too?
> 
> yes

I'm assuming that's a positive review... so the version in http://cgit.freedesktop.org/~smcv/dbus/log/?h=xmlto now has your Reviewed-By.

(In reply to comment #28)
> > it doesn't seem worth making all the other
> > man pages into .in files just to get a version number substituted.
>
> I think the other man pages would benefit also from having a version. 

Fair enough. Appended a patch to my branch, I'll attach it here in a moment.

> looks good except that the patch file have a parse error. 

Really? It applies successfully for me via either `git am` or `patch -p1`, and looks fine in Splinter too.

(e.g. `curl "https://bugs.freedesktop.org/attachment.cgi?id=74696" | git am` works)

http://cgit.freedesktop.org/~smcv/dbus/log/?h=xmlto and ssh://people.freedesktop.org/~smcv/dbus might be easier for you to review when there are this many file moves going on?
Comment 30 Ralf Habacker 2013-02-14 07:05:29 UTC
(In reply to comment #29)
> (In reply to comment #27)
> > > v2: use @DBUS_VERSION@ rather than @VERSION@, which hopefully works for
> > > CMake too?
> > 
> > yes
> 
> I'm assuming that's a positive review... so the version in
> http://cgit.freedesktop.org/~smcv/dbus/log/?h=xmlto now has your Reviewed-By.
> 
> (In reply to comment #28)
> > > it doesn't seem worth making all the other
> > > man pages into .in files just to get a version number substituted.
> >
> > I think the other man pages would benefit also from having a version. 
> 
> Fair enough. Appended a patch to my branch, I'll attach it here in a moment.
> 
> > looks good except that the patch file have a parse error. 
> 
> Really? It applies successfully for me via either `git am` or `patch -p1`,
> and looks fine in Splinter too.

got it. After redownloading and diffing i recognized an editing failure on my side. I added the Reviewed-by line by editing the patch files. 

> 
> (e.g. `curl "https://bugs.freedesktop.org/attachment.cgi?id=74696" | git am`
> works)
> 
> http://cgit.freedesktop.org/~smcv/dbus/log/?h=xmlto and
> ssh://people.freedesktop.org/~smcv/dbus might be easier for you to review
> when there are this many file moves going on?

I will take a look.
Comment 31 Ralf Habacker 2013-02-14 07:15:09 UTC
Comment on attachment 74696 [details] [review]
Use Docbook XML as the source for all man pages

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

I took a further look on the autotools patch: Here my comments

::: doc/Makefile.am
@@ +71,5 @@
>  	dbus-specification.html			\
>  	dbus-test-plan.html			\
> +	dbus-tutorial.html			\
> +	$(MAN_HTML_FILES)			\
> +	$(NULL)

why is here $(NULL) required ? I saw this already at the top of this file
Comment 32 Ralf Habacker 2013-02-14 07:26:15 UTC
(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #27)
> > > > v2: use @DBUS_VERSION@ rather than @VERSION@, which hopefully works for
> > > > CMake too?
> > > 
> > > yes
> > 
> > I'm assuming that's a positive review... so the version in
> > http://cgit.freedesktop.org/~smcv/dbus/log/?h=xmlto now has your Reviewed-By.
> > 
> > (In reply to comment #28)
> > > > it doesn't seem worth making all the other
> > > > man pages into .in files just to get a version number substituted.
> > >
> > > I think the other man pages would benefit also from having a version. 
> > 
> > Fair enough. Appended a patch to my branch, I'll attach it here in a moment.
> > 
> > > looks good except that the patch file have a parse error. 
> > 
> > Really? It applies successfully for me via either `git am` or `patch -p1`,
> > and looks fine in Splinter too.
> 
> got it. After redownloading and diffing i recognized an editing failure on
> my side. I added the Reviewed-by line by editing the patch files. 
> 
> > 
> > (e.g. `curl "https://bugs.freedesktop.org/attachment.cgi?id=74696" | git am`
> > works)
> > 
> > http://cgit.freedesktop.org/~smcv/dbus/log/?h=xmlto and
> > ssh://people.freedesktop.org/~smcv/dbus might be easier for you to review
> > when there are this many file moves going on?
> 
> I will take a look.

The first 4 pages are already reviewed. 

If the mentioned ${null} is required i cannot see any problem with http://cgit.freedesktop.org/~smcv/dbus/commit/?h=xmlto&id=1a6cce4f1903cf83e4dd7d6d7684ad7a54e743f0  as far as i understand the autotools stuff. 
so review+

http://cgit.freedesktop.org/~smcv/dbus/commit/?h=xmlto&id=4d72dcab8d108a9626e02db1b9c936c2197a94db contains unrelated space changes, which should be removed. With that review+
Comment 33 Ralf Habacker 2013-02-14 07:40:02 UTC
(In reply to comment #32)
> The first 4 pages are already reviewed. 
s/pages/patches/

sorry
Comment 34 Ralf Habacker 2013-02-14 07:55:56 UTC
(In reply to comment #28)
> D-Bus 1.6.255             02/13/2013         DBUS-DAEMON(1)
        ^^^^^^^ 
BTW: Should we not be 1.7.0 on git master ?
Comment 35 Simon McVittie 2013-02-14 12:58:53 UTC
(In reply to comment #32)
> If the mentioned ${null} is required i cannot see any problem with
> http://cgit.freedesktop.org/~smcv/dbus/commit/
> ?h=xmlto&id=1a6cce4f1903cf83e4dd7d6d7684ad7a54e743f0  as far as i understand
> the autotools stuff. 
> so review+

The point of using $(NULL) (whose value should be empty) is for the last item in a list not to be a special case: if you say

foo_SOURCES = \
    1.c \
    2.c \
    3.c

then when you come to add 4.c, it's too easy to forget to add the (newly required) trailing backslash to 3.c, leading to a syntax error; or when you do remember to add it, the diff has to touch twice as many lines, making conflicts more likely. If you use

foo_SOURCES = \
    1.c \
    2.c \
    3.c \
    $(NULL)

then every line that specifies a source file "works the same", and the last source file isn't special.

> http://cgit.freedesktop.org/~smcv/dbus/commit/
> ?h=xmlto&id=4d72dcab8d108a9626e02db1b9c936c2197a94db contains unrelated
> space changes, which should be removed. With that review+

The commit hook that's automatically set up by autogen.sh wouldn't let me commit the file move without them. I can split the whitespace changes into a separate patch if you'd prefer...

(I think it's worth getting rid of the trailing whitespace in those files now, since the file moves and format change mean we've touched every line anyway.)
Comment 36 Simon McVittie 2013-02-14 13:00:20 UTC
(In reply to comment #34)
> BTW: Should we not be 1.7.0 on git master ?

Not until we actually release it. I'm using 1.6.255 to mean "1.7.0 minus epsilon" - I originally had it set to 1.6.999, which is closer to the convention we use in Telepathy, but bits of libdbus assume that the micro version fits in a byte :-(
Comment 37 Simon McVittie 2013-02-14 13:43:09 UTC
(In reply to comment #32)
> http://cgit.freedesktop.org/~smcv/dbus/commit/
> ?h=xmlto&id=4d72dcab8d108a9626e02db1b9c936c2197a94db contains unrelated
> space changes, which should be removed. With that review+

You're right that they should have been a separate commit, I was being lazy. I also separated out the addition of @DBUS_VERSION@ into a separate commit, and pushed the format change and the @DBUS_VERSION@ (but not the whitespace).
Comment 38 Simon McVittie 2013-02-14 13:44:07 UTC
Created attachment 74812 [details] [review]
Remove doclifter "signature" from Docbook man pages'  source

This no longer serves any purpose, and might mislead contributors
into thinking that this XML is not the source for the man pages.
(The man(7)-formatted man pages used to be the canonical source for
the XML, but now it's the other way round.)

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=59805
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Comment 39 Simon McVittie 2013-02-14 13:44:30 UTC
Created attachment 74813 [details] [review]
Eliminate unwanted whitespace from the man pages' XML  source

As demanded by the git commit hook set up by autogen.sh, this eliminates
trailing whitespace on each line, and blank lines at EOF. We might as
well do this now, since every line in these files has changed anyway.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=59805
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Comment 40 Ralf Habacker 2013-02-15 07:11:09 UTC
Comment on attachment 74812 [details] [review]
Remove doclifter "signature" from Docbook man pages'  source

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

looks good r+
Comment 41 Ralf Habacker 2013-02-15 07:22:23 UTC
Comment on attachment 74813 [details] [review]
Eliminate unwanted whitespace from the man pages' XML  source

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

looks good, r+
Comment 42 Simon McVittie 2013-02-18 14:16:40 UTC
Fixed in git for 1.7.0
Comment 43 Ralf Habacker 2013-02-22 15:34:50 UTC
Created attachment 75325 [details] [review]
Fixed different docbook versions
Comment 44 Simon McVittie 2013-02-22 16:48:26 UTC
Comment on attachment 75325 [details] [review]
Fixed different docbook versions

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

It's spelled "Unified" (but I prefer commit messages in present tense, so I'd say "Unify" - I think of the commit message as something that can follow "if you merge this patch it will...")

The commit message says 4.2, but the patch says 4.4. Which do you want?

What practical effect does this have? Is there any particular reason to prefer one DTD version over another?
Comment 45 Ralf Habacker 2013-02-22 23:24:00 UTC
(In reply to comment #44)
> Comment on attachment 75325 [details] [review] [review]
> Fixed different docbook versions
> 
> Review of attachment 75325 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> It's spelled "Unified" (but I prefer commit messages in present tense, so
> I'd say "Unify" - I think of the commit message as something that can follow
> "if you merge this patch it will...")
okay 
> 
> The commit message says 4.2, but the patch says 4.4. Which do you want?
sorry, good that this difference has been raised. I took 4.4 because this version has been taken by doclifter, but we may take also 4.1.2. I do not have a dedicated preference yet.  
> 
> What practical effect does this have? Is there any particular reason to
> prefer one DTD version over another?

xmllint/xsltproc, which is called by xmlto, will fetch the related docbook dtd for every generated document from the internet, when it is not installed locally. If you have one version installed the other will be fetched from the internet, which mean you cannot build the doc without an active internet connection. 

Especially on windows fetching the dtd from the internet results into a noticable slow down of the doc generating process (required time for one document is about 5-10 seconds)
Comment 46 Ralf Habacker 2013-02-22 23:36:34 UTC
(In reply to comment #45)
> (In reply to comment #44)
> > Comment on attachment 75325 [details] [review] [review] [review]
> > Fixed different docbook versions
> > 
> > Review of attachment 75325 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > It's spelled "Unified" (but I prefer commit messages in present tense, so
> > I'd say "Unify" - I think of the commit message as something that can follow
> > "if you merge this patch it will...")
> okay 
> > 
> > The commit message says 4.2, but the patch says 4.4. Which do you want?
> sorry, good that this difference has been raised. I took 4.4 because this
> version has been taken by doclifter, but we may take also 4.1.2. I do not
> have a dedicated preference yet.  

http://www.docbook.org/specs/cd-docbook-docbook-4.4.html says: 

The Version 4.4 release is a maintenance release. It introduces no backwards-incompatible changes, same for 4.3 and 4.2.

Using 4.4 for all documents seems to be possible without any problem.
Comment 47 Ralf Habacker 2013-02-22 23:41:16 UTC
Created attachment 75384 [details] [review]
Unify docbook dtd version to 4.4
Comment 48 Simon McVittie 2013-04-19 11:40:36 UTC
(In reply to comment #47)
> Unify docbook dtd version to 4.4

Looks fine, applying it. Will be fixed in git for 1.7.2.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.