need xmllint on borka

[Peter Eisentraut]

I have been working on making the DocBook XML output valid. The first
part was bb4eefe7bf518e42c73797ea37b033a5d8a8e70a, I now have the rest
ready, but I'll spare you the mostly mechanical 200kB patch for now. In
addition, I'd like to add the attached patch with an xmllint call to
make sure things stay valid.

But we don't have xmllint installed on borka, where we build the
releases. Could someone please install it?

[Tom Lane]

Post by Peter Eisentraut
I have been working on making the DocBook XML output valid. The first
part was bb4eefe7bf518e42c73797ea37b033a5d8a8e70a, I now have the rest
ready, but I'll spare you the mostly mechanical 200kB patch for now. In
addition, I'd like to add the attached patch with an xmllint call to
make sure things stay valid.
But we don't have xmllint installed on borka, where we build the
releases. Could someone please install it?

-1. Doesn't this break "make man" for *any* hacker who doesn't have
xmllint installed?

Please change the patch so that it runs xmllint if available, rather
than turning it into a de facto requirement for anybody who works on
documentation. Or if you think you can pass a vote to require it,
I'd suggest that the patch had better include documentation additions
listing this as a required build tool.

(The subtext here is that borka is absolutely not an acceptable place
to encounter documentation build failures. By the time we're at that
stage of the release cycle, I don't really care what xmllint might
have to say; there isn't going to be time to make it happy.)

regards, tom lane

[Alvaro Herrera]

Post by Tom Lane
(The subtext here is that borka is absolutely not an acceptable place
to encounter documentation build failures. By the time we're at that
stage of the release cycle, I don't really care what xmllint might
have to say; there isn't going to be time to make it happy.)

Borka is what runs the guaibasaurus animal, so failures would show up in
buildfarm ...

If the xmllint check could be made optional, I guess we could have the
failures show up in buildfarm but avoid having them cause a problem for
"make dist" when releases are created.

[Peter Eisentraut]

Post by Tom Lane

Post by Peter Eisentraut
But we don't have xmllint installed on borka, where we build the
releases. Could someone please install it?

-1. Doesn't this break "make man" for *any* hacker who doesn't have
xmllint installed?
Please change the patch so that it runs xmllint if available, rather
than turning it into a de facto requirement for anybody who works on
documentation.

The intention is that we enforce that the documentation is correctly
formatted. Enforcing that only when the required tool is installed is
the same as not enforcing it and annoying those who happen to have the
tool installed.

Post by Tom Lane
Or if you think you can pass a vote to require it,
I'd suggest that the patch had better include documentation additions
listing this as a required build tool.

Agreed. I have committed the SGML changes that make things valid now,
but I will postpone the xmllint addition until the 9.5 branch, complete
with more documentation.

[Tom Lane]

Post by Peter Eisentraut

Post by Tom Lane
-1. Doesn't this break "make man" for *any* hacker who doesn't have
xmllint installed?

The intention is that we enforce that the documentation is correctly
formatted. Enforcing that only when the required tool is installed is
the same as not enforcing it and annoying those who happen to have the
tool installed.

Okay, but if that's the intent I suggest that the check needs to happen
somewhere more prominent than in nondefault build targets. Personally,
I run "make man" about once a release cycle, if that often; and I'm not
sure I've ever built any of the other targets modified in this patch.
If you want to enforce correctness then I think you should put the xmllint
call into the "make all" path, which would render all of the targets
touched here rather redundant.

Also, in the vein of "is this a full-scale build requirement or not",
why is the pathname of xmllint just hard-coded into the makefile and
not something that's checked for by configure?

regards, tom lane

[Peter Eisentraut]

Post by Peter Eisentraut
Agreed. I have committed the SGML changes that make things valid now,
but I will postpone the xmllint addition until the 9.5 branch, complete
with more documentation.

Per the above announcement, here is an updated patch, also with more
documentation and explanations.

It would especially be valuable if someone with a different-than-mine OS
would verify whether they can install xmllint according to the
documentation, or update the documentation if not.

[Tom Lane]

Post by Peter Eisentraut
It would especially be valuable if someone with a different-than-mine OS
would verify whether they can install xmllint according to the
documentation, or update the documentation if not.

FWIW, xmllint appears to be part of the base libxml2 package on Red Hat
(checked on RHEL6 and Fedora rawhide).

I'm not sure I'd phrase it like this:

+ This library and the <command>xmllint</command> tool it contains are
+ used for processing XML. Many developers will already

The library surely does not contain the tool; more like vice versa.
Perhaps "provides" would be a better verb.

regards, tom lane

[Fabien COELHO]

Hello Peter,

Post by Peter Eisentraut

Post by Peter Eisentraut
Agreed. I have committed the SGML changes that make things valid now,
but I will postpone the xmllint addition until the 9.5 branch, complete
with more documentation.

Per the above announcement, here is an updated patch, also with more
documentation and explanations.
It would especially be valuable if someone with a different-than-mine OS
would verify whether they can install xmllint according to the
documentation, or update the documentation if not.

Tested on Ubuntu trusty.

Patched applies with some minor warning on current head, and works, that
is "xmllint" is run for some of the targets (epub, man).

However, it seems that it is not run for target "html", nor for pdf/ps
targets. I'm wondering whether it is an oversight or if there is a reason
for that. Maybe the intention is that an explicit "htmlhelp" is expected
to do the checking, but then why force it for man and epub? It seems to me
that it is not consistent.


Also, a general comment, which is independent of this patch: I found the
documentation build especially not resilient, with a lack of clear error
messages when something is broken. Basically, if configure does not found
something for the doc (openjade, osx, xmllint, ...) it does not complain.
That is fine with me, people would not always want to build the doc anyway
as it is available online. However, the Makefile in doc/src/sgml overrides
the not found commands (ifndef JADE JADE=..., etc), and proceed to
unhelpful and unclear errors later on. ISTM that it may be more helful to
do:

ifndef JADE
#error "no jade found on your system, cannot generate the documention"
endif

Rather than overriding with "JADE=jade" if jade was not there when
configuring.

This xmllint addition is done in the same spirit. I would suggest at the
minimum to check that xmllint is available before running it, or to ignore
the call if not available, something like:

type -p $(XMLLINT) || { echo error no $(XMLLINT)... ; exit 1 ; }
$(XMLLINT) ...

or

-type -p $(XMLLINT) && $(XMLLINT) ...

And I would prefer that a straightforward error is generated when
commands or styles are not found, in general.

Also, a detail, the Makefile style is not homogeneous:

ifndef XSLTPROC
XSLTPROC = xsltproc
endif

DBTOEPUB ?= dbtoepub

Why not XSLTPROC ?= xsltproc and the like?

[Fabien COELHO]

Post by Fabien COELHO
Also, a general comment, which is independent of this patch: I found the
documentation build especially not resilient, with a lack of clear error
messages when something is broken. Basically, if configure does not found
something for the doc (openjade, osx, xmllint, ...) it does not complain.
That is fine with me, people would not always want to build the doc anyway as
it is available online. However, the Makefile in doc/src/sgml overrides the
not found commands (ifndef JADE JADE=..., etc), and proceed to unhelpful and

To be more constructive:

Maybe all commands could have a check counterpart added to the
dependencies, so as to fail gracefully only if needed, something like:

.check_XXX:
if type -p $(XXX) > /dev/null ; then touch $@ ; else \
echo "command $(XXX) not found"; exit 1 ; \
fi

foo: .check_XXX
$(XXX) ...

I'm not sure how to check for the docbook style availability though.

[Peter Eisentraut]

Post by Fabien COELHO
However, it seems that it is not run for target "html", nor for pdf/ps
targets. I'm wondering whether it is an oversight or if there is a
reason for that. Maybe the intention is that an explicit "htmlhelp" is
expected to do the checking, but then why force it for man and epub? It
seems to me that it is not consistent.

It is only run when the build is via XML/XSLT, not via SGML/DSSSL
(because the SGML/DSSSL build already checks the validity anyway).

[Tom Lane]

Post by Peter Eisentraut

Post by Fabien COELHO
However, it seems that it is not run for target "html", nor for pdf/ps
targets. I'm wondering whether it is an oversight or if there is a
reason for that. Maybe the intention is that an explicit "htmlhelp" is
expected to do the checking, but then why force it for man and epub? It
seems to me that it is not consistent.

It is only run when the build is via XML/XSLT, not via SGML/DSSSL
(because the SGML/DSSSL build already checks the validity anyway).

I'm confused. I thought the point of this change was mostly that the
SGML toolchain is less strict than the XML toolchain, and you wanted
to have the more-strict checks applied whenever possible.

regards, tom lane

[Peter Eisentraut]

Post by Tom Lane

Post by Peter Eisentraut
It is only run when the build is via XML/XSLT, not via SGML/DSSSL
(because the SGML/DSSSL build already checks the validity anyway).

I'm confused. I thought the point of this change was mostly that the
SGML toolchain is less strict than the XML toolchain, and you wanted
to have the more-strict checks applied whenever possible.

The SGML tool chain is less strict about what it considers valid, but
the XML toolchain doesn't check at all unless we run xmllint, it just
produces garbage when the input is invalid.

[Fabien COELHO]

Post by Peter Eisentraut

Post by Tom Lane

Post by Peter Eisentraut
It is only run when the build is via XML/XSLT, not via SGML/DSSSL
(because the SGML/DSSSL build already checks the validity anyway).

I'm confused. I thought the point of this change was mostly that the
SGML toolchain is less strict than the XML toolchain, and you wanted
to have the more-strict checks applied whenever possible.

The SGML tool chain is less strict about what it considers valid, but
the XML toolchain doesn't check at all unless we run xmllint, it just
produces garbage when the input is invalid.

This suggests that "xmllint" should always be run, because it is more
strict than the other, so it is safer if the source has passed it and is
validated, whatever the tool chain used afterwards?

[Peter Eisentraut]

Post by Fabien COELHO
Also, a general comment, which is independent of this patch: I found the
documentation build especially not resilient, with a lack of clear error
messages when something is broken. Basically, if configure does not
found something for the doc (openjade, osx, xmllint, ...) it does not
complain. That is fine with me, people would not always want to build
the doc anyway as it is available online. However, the Makefile in
doc/src/sgml overrides the not found commands (ifndef JADE JADE=...,
etc), and proceed to unhelpful and unclear errors later on. ISTM that it
ifndef JADE
#error "no jade found on your system, cannot generate the documention"
endif

We could use $(missing) for that, which is already used for bison, flex,
and perl.

[Tom Lane]

Post by Peter Eisentraut
We could use $(missing) for that, which is already used for bison, flex,
and perl.

+1 ... I'm surprised it's not like that already. Fabien's quite right to
complain that the Makefile has no business inserting defaults for things
configure couldn't find.

regards, tom lane

[Fabien COELHO]

Post by Peter Eisentraut

Post by Fabien COELHO
ifndef JADE
#error "no jade found on your system, cannot generate the documention"
endif

We could use $(missing) for that, which is already used for bison, flex,
and perl.

I'm fine with "$(missing)" or whatever else that provides a clear message.
Oops, not "#error", but "$(error ...)", I was mixing cpp & make above...

However the example in the doc Makefile for "collageindex.pl" is on the
heavy side, as it suggests that every use of such commands should be put
in ifdef/else/endif. ISTM that a dependence-based solution would be
simpler.

[Peter Eisentraut]

Post by Fabien COELHO
I'm fine with "$(missing)" or whatever else that provides a clear message.

I've committed the $(missing) use separately, and rebased this patch on
top of that.

[Fabien COELHO]

Hello Peter,

Post by Peter Eisentraut
I've committed the $(missing) use separately,

That was simple and is a definite improvement.

Tiny detail: the new DBTOEPUB macro definition in "src/Makefile.global.in"
lacks another tab to be nicely aligned with the other definitions.

Post by Peter Eisentraut
and rebased this patch on top of that.

Applied and tested, everything looks fine.

The only remaining question is whether the xmllint check should always be
called. You stated that it was stricter than sgml processing, so I would
think it worth to always call it, but this is really a marginal
preference. I think it is okay if some slaves in the build farm do build
the various targets.

[Peter Eisentraut]

Post by Fabien COELHO

Post by Peter Eisentraut
and rebased this patch on top of that.

Applied and tested, everything looks fine.
The only remaining question is whether the xmllint check should always
be called. You stated that it was stricter than sgml processing, so I
would think it worth to always call it, but this is really a marginal
preference. I think it is okay if some slaves in the build farm do build
the various targets.

Committed.

[Alvaro Herrera]

Post by Peter Eisentraut
I have been working on making the DocBook XML output valid. The first
part was bb4eefe7bf518e42c73797ea37b033a5d8a8e70a, I now have the rest
ready, but I'll spare you the mostly mechanical 200kB patch for now. In
addition, I'd like to add the attached patch with an xmllint call to
make sure things stay valid.
But we don't have xmllint installed on borka, where we build the
releases. Could someone please install it?

xmllint installed on borka.