Hi,
On 2022-03-09 13:37:23 +0100, Peter Eisentraut wrote:
> I looked at this today mainly to consider whether some of the prereq
> work is ready for adoption now.
Thanks!
> A lot of the work has to do with
> making various scripts write the output to other directories. I
> suspect this has something to do with how meson handles separate build
> directories and how we have so far handled files created in the
> distribution tarball. But the whole picture isn't clear to me.
A big part of it is that when building with ninja tools are invoked in the
top-level build directory, but right now a bunch of our scripts put their
output in CWD.
> More generally, I don't see a distprep target in the meson build
> files. I wonder what your plan for that is, or whether that would
> even work under meson. In [0], I argued for getting rid of the
> distprep step. Perhaps it is time to reconsider that now.
>
> [0]: https://www.postgresql.org/message-id/flat/cf0bec33-d965-664d-e0ec-fb15290f2273%402ndquadrant.com
I think it should be doable to add something roughly like the current distprep. The
cleanest way would be to use
https://mesonbuild.com/Reference-manual_builtin_meson.html#mesonadd_dist_script
to copy the files into the generated tarball.
Of course not adding it would be even easier ;)
> For the short term, I think the patches 0002, 0008, 0010, and 0011
> could be adopted, if they are finished as described.
Cool.
> Patch 0007 seems unrelated, or at least independently significant, and
> should be discussed separately.
It's related - it saves us from doing a lot of extra complexity on
windows. I've brought it up as a separate thread too:
https://postgr.es/m/20211101020311.av6hphdl6xbjbuif%40alap3.anarazel.de
I'm currently a bit stuck implementing this properly for the configure / make
system, as outlined in:
https://www.postgresql.org/message-id/20220111025328.iq5g6uck53j5qtin%40alap3.anarazel.de
> The rest is really all part of the same put-things-in-the-right-directory
> issue.
>
> For the overall patch set, I did a quick test with
>
> meson setup build
> cd build
> ninja
>
> which failed with
>
> Undefined symbols for architecture x86_64:
> "_bbsink_zstd_new", referenced from:
> _SendBaseBackup in replication_basebackup.c.o
>
> So maybe your patch set is not up to date with this new zstd build
> option.
Yep, I posted it before "7cf085f077d - Add support for zstd base backup
compression." went in, but after 6c417bbcc8f. So the meson build knew about
the zstd dependency, but didn't yet specify it for postgres /
pg_basebackup. So all that's needed was / is adding the dependency to those
two places.
Updated patches attached. This just contains the fix for this issue, doesn't
yet address review comments.
FWIW, I'd already pushed those fixes out to the git tree. There's frequent
enough small changes that reposting everytime seems too noisy.
> v6-0001-meson-prereq-output-and-depencency-tracking-work.patch.gz
>
> This all looks kind of reasonable, but lacks explanation in some
> cases, so I can't fully judge it.
I'll try to clean it up.
> v6-0007-meson-prereq-Can-we-get-away-with-not-export-all-.patch.gz
>
> This is a separate discussion. It's not clear to me why this is part
> of this patch series.
See above.
> v6-0008-meson-prereq-Handle-DLSUFFIX-in-msvc-builds-simil.patch.gz
>
> Part of this was already done in 0001, so check if these patches are
> split correctly.
>
> I think the right way here is actually to go the other way around:
> Move DLSUFFIX into header files for all platforms. Move the DLSUFFIX
> assignment from src/makefiles/ to src/templates/, have configure read
> it, and then substitute it into Makefile.global and pg_config.h.
>
> Then we also don't have to patch the Windows build code a bunch of
> times to add the DLSUFFIX define everywhere.
>
> There is code in configure already that would benefit from this, which
> currently says
>
> # We don't know the platform DLSUFFIX here, so check 'em all.
I'll try it out.
> v6-0009-prereq-make-unicode-targets-work-in-vpath-builds.patch.gz
>
> Another directory issue
I think it's a tad different, in that it's fixing something that's currently
broken in VPATH builds.
> v6-0010-ldap-tests-Don-t-run-on-unsupported-operating-sys.patch.gz
>
> Not sure what this is supposed to do, but it looks independent of this
> patch series. Does it currently not work on "unsupported" operating
> systems?
Right now if you run the ldap tests on windows, openbsd, ... the tests
fail. The only reason it doesn't cause trouble on the buildfarm is that we
currently don't run those tests by default...
> v6-0011-ldap-tests-Add-paths-for-openbsd.patch.gz
>
> The more the merrier, although I'm a little bit worried about pointing
> to a /usr/local/share/examples/ directory.
It's where the files are in the package :/.
> v6-0012-wip-split-TESTDIR-into-two.patch.gz
> v6-0013-meson-Add-meson-based-buildsystem.patch.gz
> v6-0014-meson-ci-Build-both-with-meson-and-as-before.patch.gz
>
> I suggest in the interim to add a README.meson to show how to invoke
> this. Eventually, of course, we'd rewrite the installation
> instructions.
Good idea.
Greetings,
Andres Freund
