run xmllint during build (was Re: need xmllint on borka)

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?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> 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

Tom Lane wrote:

> (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.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Peter Eisentraut wrote:
> 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.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, 2014-05-01 at 22:51 -0400, Tom Lane wrote:
> > 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.

> 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.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On Thu, 2014-05-01 at 22:51 -0400, Tom Lane wrote:
>> -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

On 5/6/14 10:20 PM, Peter Eisentraut wrote:
> 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.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> 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

Hello Peter,

>> 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.

> 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:

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.

--
Fabien.

On 8/21/14 5:12 AM, Fabien COELHO wrote:
>
> 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).

On 8/21/14 5:12 AM, Fabien COELHO wrote:
> 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

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

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 8/21/14 5:12 AM, Fabien COELHO wrote:
>> 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 <peter_e(at)gmx(dot)net> writes:
> 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

On 8/21/14 4:00 PM, Tom Lane wrote:
>> 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.

>>> 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?

--
Fabien.

>> ISTM that it may be more helful to do:
>>
>> 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.

--
Fabien.

On 8/21/14 4:28 PM, Fabien COELHO wrote:
> 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.

Hello Peter,

> 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.

> 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.

--
Fabien.

On 9/14/14 3:34 AM, Fabien COELHO wrote:
>> 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.