Re: meson documentation build open issues

Hi,

On 2023-11-14 21:16:13 +0100, Peter Eisentraut wrote:
> Some comments on your patches:
>
> v2-0001-meson-Change-default-of-selinux-feature-option-to.patch
>
> Ok
>
> v2-0002-docs-Document-with-selinux-Dselinux-options-centr.patch
>
> Ok, but "selinux" should be "SELinux" when referring to the product.

Will apply with that fix.

> v2-0003-meson-docs-Add-doc-html-man-targets.patch
>
> We have make targets "html" and "man", so I suggest we make the meson
> targets the same.

Hm, ok.

> v2-0004-meson-Add-world-target.patch
>
> AFAICT, this world target doesn't include the man target. (Again, this
> would all work better if we added "man" to "docs".)

I agree with that sentiment - I only moved to the current arrangement after
Tom argued forcefully against building both.

The situation in the make world is weird:
"make docs" in the toplevel builds both, because it's defined as

docs:
$(MAKE) -C doc all

Buf if you "make -C doc/src/sgml" (or are in doc/src/sgml), we only build
html, as the default target is explicitly just html:

# Make "html" the default target, since that is what most people tend
# to want to use.
html:

There's no real way of making the recursive-make and non-recursive ninja
coherent. There's no equivalent to default target in a sudirectory with ninja
(or non-recursive make).

> v2-0005-docs-meson-Add-documentation-for-important-build-.patch
>
> It's nice to document this, but it's weird that we only document the meson
> targets, not the make targets.

I think it'd have been good if we had documented the important targets with
make. But I don't think documenting them as a prerequisite to documenting the
meson targets makes much sense.

> v2-0006-meson-Add-help-target-build-docs-from-a-common-so.patch
>
> Here also, making this consistent and uniform with make would be useful.

What precisely are you referring to here? Also adding a help target? Or just
consistency between what the "docs" target does?

> v2-0007-meson-Add-Dpkglibdir-option.patch
>
> Normally, the pkgFOOdir variables are just FOOdir plus package name. I
> don't feel comfortable allowing those to be separately set. We don't allow
> that with configure; this just arose from a Debian patch.

Right - but Debian's desire seems quite sensible. The need to have multiple
postgres versions installed in parallel is quite widespread.

> The description "location to dynamically loadable modules" is too narrow.
> Consider for example, another proposed patch, where we are doing some
> preprocessing on postgres.bki at build time. Since that makes postgres.bki
> platform dependent, it should really be moved from share (datadir in
> configure parlance) to pkglibdir.

I think I cannot be faulted for documenting the current use of the directory
:).

Separately, I'm not really convinced that moving some build time values into
postgres.bki is useful, but that's a matter for a different thread.

> So then we have things in there that are not loadable modules. I don't know
> how that affects Debian packaging, but this patch might not be the right
> one.

I'm not really seeing why that'd affect pkglibdir being adjustable, besides
needing to tweak the description of pkglibdir?

> I suggest we leave this patch for a separate discussion.

Fair enough.

Thanks for the review,

Andres Freund