Description
Ralf Habacker
2013-01-24 13:23:46 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. (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 ... (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. (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 ... ? (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. (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. (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. Created attachment 74208 [details] [review] Moved docbook source used by cmake to doc subdir 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. Created attachment 74209 [details] [review] Moved docbook source used by cmake to doc subdir Created attachment 74210 [details] [review] Moved docbook source used by cmake to doc subdir fixup Created attachment 74212 [details] [review] Moved docbook source used by cmake to doc subdir (fixed install target) Created attachment 74213 [details] [review] Updated man docbook xml sources from man page source using doclifter. Created attachment 74214 [details] [review] Generate Man pages from docbook sources for cmake buildsystem Created attachment 74216 [details] [review] Fixed tweaks when generating man pages from docbook sources. (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. (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 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 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 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 on attachment 74216 [details] [review] Fixed tweaks when generating man pages from docbook sources. Review of attachment 74216 [details] [review]: ----------------------------------------------------------------- I suppose so. 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. 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]. 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? 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. (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 (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 (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. (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? (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 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 (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+ (In reply to comment #32) > The first 4 pages are already reviewed. s/pages/patches/ sorry (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 ? (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.) (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 :-( (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). 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> 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 on attachment 74812 [details] [review] Remove doclifter "signature" from Docbook man pages' source Review of attachment 74812 [details] [review]: ----------------------------------------------------------------- looks good r+ Comment on attachment 74813 [details] [review] Eliminate unwanted whitespace from the man pages' XML source Review of attachment 74813 [details] [review]: ----------------------------------------------------------------- looks good, r+ Fixed in git for 1.7.0 Created attachment 75325 [details] [review] Fixed different docbook versions 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? (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) (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. Created attachment 75384 [details] [review] Unify docbook dtd version to 4.4 (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.