Re: [RFC] building postgres with meson - v13

Hi,

On 2022-09-26 15:01:56 +0200, Peter Eisentraut wrote:
> Here is some review of the remaining ones (might not match exactly what you
> attached, I was working off your branch):

Thanks, and makes sense.

> 9f789350a7a7 meson: ci: wip: move compilerwarnings task to meson
>
> This sounds reasonable to me in principle, but I haven't followed the
> cirrus stuff too closely, and it doesn't say why it's "wip". Perhaps
> others could take a closer look.

It's mostly WIP because it doesn't yet convert all the checks, specifically
headerscheck/cpluspluscheck isn't converted yet.

> ccf20a68f874 meson: ci: Add additional CI coverage
>
> IIUC, this is just for testing your branch, not meant for master?

Yes. I think we might want to add openbsd / netbsd at some point, but that'll
be a separate thread. Until then it just catches a bunch of mistakes more
easily.

> 02d84c21b227 meson: prereq: win: remove date from version number in
> win32ver.rc
>
> do it

The newest version has evolved a bit, changing Project.pm as well.

> 5c42b3e7812e meson: WIP: Add some of the windows resource files
>
> What is the thinking on this now? What does this change over the
> current state?

The newest commit has a lot more rc files added and has this summary:

meson: Add windows resource files

The generated resource files aren't exactly the same ones as the old
buildsystems generate. Previously "InternalName" and "OriginalFileName" were
mostly wrong / not set (despite being required), but that was hard to fix in
at least the make build. Additionally, the meson build falls back to a
"auto-generated" description when not set, and doesn't set it in a few cases -
unlikely that anybody looks at these descriptions in detail.

The only thing missing rc files is the various ecpg libraries. The issue is
that we shouldn't add resource file to static libraries, so we need to split
the definitions. I'll go and do that next.

> 9bc60bccfd10 meson: Add support for relative rpaths, fixing tests on MacOS
> w/ SIP
>
> I suggest a separate thread and/or CF entry for this. There have been
> various attempts to deal with SIP before, with varying results. This
> is not part of the meson transition as such.

I think I might need to split this one more time. We don't add all the rpaths
we add with autoconf before this commit, even not on macOS, which is not
great... Nor do we have a --disable-rpath equivalent yet - I suspect we'll
need that.

https://postgr.es/m/20220922223729.GA721620%40nathanxps13

> 9f5be26c1215 meson: Add docs for building with meson
>
> I do like the overall layout of this.
>
> The "Supported Platforms" section should be moved back to near the end
> of the chapter. I don't see a reason to move it forward, at least
> none that is related to the meson issue.
>
> The changes to the "Getting the Source" section are also not
> appropriate for this patch.

We don't really support building from a tarball with meson yet (you'd need to
confiure, maintainer-clean, configure meson), so it does make some sense...

> 9c00d355d0e9 meson: Add PGXS compatibility
>
> This looks like a reasonable direction to me. How complete is it? It
> says it works for some extensions but not others. How do we define
> the target line here?

Yea, those are good questions.

> How complete is it?

It's a bit hard to know. I think the most important stuff is there. But
there's no clear "API" around pgxs. E.g. we don't (yet?) have an exactly
equivalent definition of 'host', because that's very config.guess specific.

There's lots of shortcuts - e.g. with meson we don't need an equivalent to
PGAC_CHECK_STRIP, so we need to make up something for Makefile.global.

Noah suggested using $(error something), but that only works if $variable is
only used in recursively expanded variables - the errors end up confusing.

> It says it works for some extensions but not others.

I think that's slightly outdated - IIRC it was about pgbouncer, but after a
fix the remaining failure is shared between autoconf and meson builds.

> 3fd5e13dcad3 meson: Add postgresql-extension.pc for building extension
> libraries
>
> Separate thread for this as well. This is good and important, but we
> must also add it to the make build.

Makes sense.

> eb40f6e53104 meson: Add support for building with precompiled headers
>
> Any reason not to enable this by default? The benefits on non-Windows
> appear to be less dramatic, but they are not zero. Might be better to
> enable it consistently so that for example any breakage is easier
> caught.

There's no real reason not to - the wins are small on linux, so introducing
PCH didn't seem necessary. I'm also not sure how well pch works across random
compiler versions - it's so crucial on windows that it seems like a more well
worn path there.

linux, gcc 12:

b_pch=false:
real 0m16.233s
user 6m40.375s
sys 0m48.953s

b_pch=true:
real 0m15.983s
user 6m20.357s
sys 0m49.967s

freebsd VM, clang:

b_pch=false:

real 0m23.035s
user 3m11.241s
sys 0m31.171s

b_pch=true:

real 0m21.643s
user 2m57.143s
sys 0m30.246s

Somewhat confirming my suspicions from above, gcc11 ICEs on freebsd with PCH,
and gcc12 fails with an unhelpful:
<command-line>: sorry, unimplemented: PCH allocation failure

> 377bfdea6042 meson: Add xmllint/xsltproc wrapper script to handle
> dependencies automatically
>
> Is this part of the initial transition, required for correctness, or
> is it an optional optimization? Could use more explanation. Maybe
> move to separate thread also?

It's required for correctness - in master we don't rebuild the docs when a
file changes. meson and ninja don't support wildcards (for good reasons - it
makes scanning for changes much more expensive). By using "compiler" generated
dependencies this is solved in a reliably and notationally cheap way. So I
don't think it makes sense to split this one off into a separate thread?

Greetings,

Andres Freund