Feedback-ID: i131946ab:Fastmail
Content-Type: multipart/mixed; boundary="------------qvaZz9nPpS4djhHELPkod5Qn"
Message-ID: <7dae5979-c6c0-cec5-7a36-76a85aa8053d@enterprisedb.com>
Date: Wed, 24 Aug 2022 11:39:06 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.12.0
Subject: Re: [RFC] building postgres with meson - v11
Content-Language: en-US
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, samay sharma <smilingsamay@gmail.com>,
 Nazir Bilal Yavuz <byavuz81@gmail.com>
References: <20211012083721.hvixq4pnh2pixr3j@alap3.anarazel.de>
 <20220811002012.ju3rrz47i2e5tdha@awork3.anarazel.de>
 <e0c44fb2-8b66-a4b9-b274-7ed3a1a0ab74@enterprisedb.com>
 <20220817215317.poeofidf7o7dy6hy@awork3.anarazel.de>
 <8992c637-0922-8cf8-9188-104153eb3561@enterprisedb.com>
 <20220820180139.6uj5etquyonz4otf@awork3.anarazel.de>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <20220820180139.6uj5etquyonz4otf@awork3.anarazel.de>
Archived-At: 
 <https://www.postgresql.org/message-id/7dae5979-c6c0-cec5-7a36-76a85aa8053d%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------qvaZz9nPpS4djhHELPkod5Qn
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

I have looked at your branch at 0545eec895:

258f6dc0a7 Don't hardcode tmp_check/ as test directory for tap tests
8ecc33cf04 Split TESTDIR into TESTLOGDIR and TESTDATADIR

I think these patches are split up a bit incorrectly.  If you apply
the first patch by itself, then the output appears in tab_comp_dir/
directly under the source directory.  And then the second patch moves
it to tmp_check/tap_comp_dir/.  If there is an intent to apply these
patches separately somehow, this should be cleaned up.

I haven't checked the second patch in detail yet, but it looks like
the thought was that the first patch is about ready to go.

834a40e609 meson: prereq: Extend gendef.pl in preparation for meson

I'm not qualified to check that in detail, but it looks reasonable
enough to me.

See attached patch (0001) for a perlcritic fix.

97a0b096e8 meson: prereq: Add src/tools/gen_export.pl

This produces leading whitespace in the output files that at least on
darwin wasn't there before.  See attached patch (0002).  This should
be checked again on other platforms as well.

Other than that this looks good.  Attached is a small cosmetic patch (0003).

40e363b263 meson: prereq: Refactor PG_TEST_EXTRA logic in autoconf build

Since I last looked, this has been turned into a meson option.  Which
is probably the best solution.  But then we should probably make this
a configure option as well.  Otherwise, it could get a bit confusing.
For example, I just unset PG_TEST_EXTRA in my environment to test
something with the meson build, but I was unaware that meson captures
the value at setup time, so my unsetting had no effect.

In any case, maybe adjust the regular expressions to check for word
boundaries, to maintain the original "whitespace-separated"
specification.  For example,

     elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)

e0a8387660 solaris: Use versioning scripts instead of -Bsymbolic

This looks like a good idea.  The documentation clearly states that
-Bsymbolic shouldn't be used, at least not in the way we have been
doing.  Might as well go ahead with this and give it a whirl on the
build farm.

0545eec895 meson: Add docs

We should think more about how to arrange the documentation.  We
probably don't want to copy-and-paste all the introductory and
requirements information.  I think we can make this initially much
briefer, like the Windows installation chapter.  For example, instead
of documenting each setup option again, just mention which ones exist
and then point (link) to the configure chapter for details.


I spent a bit of time with the test suites.  I think there is a
problem in that selecting a test suite directly, like

     meson test -C _build --suite recovery

doesn't update the tmp_install.  So if this is the first thing you run
after a build, everything will fail.  Also, if you run this later, the
tmp_install doesn't get updated, so you're not testing up-to-date
code.
--------------qvaZz9nPpS4djhHELPkod5Qn
Content-Type: text/plain; charset=UTF-8; name="0001-Fix-for-perlcritic.patch"
Content-Disposition: attachment; filename="0001-Fix-for-perlcritic.patch"
Content-Transfer-Encoding: base64

RnJvbSAyZjI1YjQ4MjcxYmNlYjdhYTE1NTFlMDE1YjAzZmMyMGI5YWZmMTYyIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQZXRlciBFaXNlbnRyYXV0IDxwZXRlckBlaXNlbnRy
YXV0Lm9yZz4KRGF0ZTogV2VkLCAyNCBBdWcgMjAyMiAxMToyMzo1MiArMDIwMApTdWJqZWN0
OiBbUEFUQ0ggMS8zXSBGaXggZm9yIHBlcmxjcml0aWMKCi0tLQogc3JjL3Rvb2xzL21zdmMv
Z2VuZGVmLnBsIHwgMiArLQogMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspLCAxIGRl
bGV0aW9uKC0pCgpkaWZmIC0tZ2l0IGEvc3JjL3Rvb2xzL21zdmMvZ2VuZGVmLnBsIGIvc3Jj
L3Rvb2xzL21zdmMvZ2VuZGVmLnBsCmluZGV4IGYwODI2OGU3ODEuLmNmYmRlZjkwMDcgMTAw
NjQ0Ci0tLSBhL3NyYy90b29scy9tc3ZjL2dlbmRlZi5wbAorKysgYi9zcmMvdG9vbHMvbXN2
Yy9nZW5kZWYucGwKQEAgLTE3NCw3ICsxNzQsNyBAQCBzdWIgdXNhZ2UKIHsKIAlpZiAoLWQg
JGluKQogCXsKLQkJcHVzaCBAZmlsZXMsIDwkaW4vKi5vYmo+OworCQlwdXNoIEBmaWxlcywg
Z2xvYiAiJGluLyoub2JqIjsKIAl9CiAJZWxzZQogCXsKLS0gCjIuMzcuMQoK
--------------qvaZz9nPpS4djhHELPkod5Qn
Content-Type: text/plain; charset=UTF-8;
 name="0002-Fix-whitespace-in-output-of-gen_export.pl.patch"
Content-Disposition: attachment;
 filename="0002-Fix-whitespace-in-output-of-gen_export.pl.patch"
Content-Transfer-Encoding: base64

RnJvbSBhNjRjOTBlNzU2ZDY5OTZiN2Q4ZDlkNjNkNDJlMzBjNzJkNGNlMDk4IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQZXRlciBFaXNlbnRyYXV0IDxwZXRlckBlaXNlbnRy
YXV0Lm9yZz4KRGF0ZTogV2VkLCAyNCBBdWcgMjAyMiAxMToyNDoyMiArMDIwMApTdWJqZWN0
OiBbUEFUQ0ggMi8zXSBGaXggd2hpdGVzcGFjZSBpbiBvdXRwdXQgb2YgZ2VuX2V4cG9ydC5w
bAoKLS0tCiBzcmMvdG9vbHMvZ2VuX2V4cG9ydC5wbCB8IDIgKy0KIDEgZmlsZSBjaGFuZ2Vk
LCAxIGluc2VydGlvbigrKSwgMSBkZWxldGlvbigtKQoKZGlmZiAtLWdpdCBhL3NyYy90b29s
cy9nZW5fZXhwb3J0LnBsIGIvc3JjL3Rvb2xzL2dlbl9leHBvcnQucGwKaW5kZXggNmE4YjE5
NmFkOC4uMTI2NTU2NDQ3MyAxMDA2NDQKLS0tIGEvc3JjL3Rvb2xzL2dlbl9leHBvcnQucGwK
KysrIGIvc3JjL3Rvb2xzL2dlbl9leHBvcnQucGwKQEAgLTU2LDcgKzU2LDcgQEAKIAkJfQog
CQllbHNpZiAoJGZvcm1hdCBlcSAnZGFyd2luJykKIAkJewotCQkJcHJpbnQgJG91dHB1dF9o
YW5kbGUgIiAgICBfJDFcbiI7CisJCQlwcmludCAkb3V0cHV0X2hhbmRsZSAiXyQxXG4iOwog
CQl9CiAJCWVsc2lmICgkZm9ybWF0IGVxICdnbnUnKQogCQl7Ci0tIAoyLjM3LjEKCg==
--------------qvaZz9nPpS4djhHELPkod5Qn
Content-Type: text/plain; charset=UTF-8;
 name="0003-Some-Perl-code-simplification-in-gen_export.pl.patch"
Content-Disposition: attachment;
 filename*0="0003-Some-Perl-code-simplification-in-gen_export.pl.patch"
Content-Transfer-Encoding: base64

RnJvbSA1Yjc5YmFmNmJiZjRmYTYyZjE1M2E1ZTk2ZTk3ZjhkNWE2MzQ1ODIxIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQZXRlciBFaXNlbnRyYXV0IDxwZXRlckBlaXNlbnRy
YXV0Lm9yZz4KRGF0ZTogV2VkLCAyNCBBdWcgMjAyMiAxMToyNDozNiArMDIwMApTdWJqZWN0
OiBbUEFUQ0ggMy8zXSBTb21lIFBlcmwgY29kZSBzaW1wbGlmaWNhdGlvbiBpbiBnZW5fZXhw
b3J0LnBsCgotLS0KIHNyYy90b29scy9nZW5fZXhwb3J0LnBsIHwgNCArLS0tCiAxIGZpbGUg
Y2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDMgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEv
c3JjL3Rvb2xzL2dlbl9leHBvcnQucGwgYi9zcmMvdG9vbHMvZ2VuX2V4cG9ydC5wbAppbmRl
eCAxMjY1NTY0NDczLi43MjcxMDViYTA4IDEwMDY0NAotLS0gYS9zcmMvdG9vbHMvZ2VuX2V4
cG9ydC5wbAorKysgYi9zcmMvdG9vbHMvZ2VuX2V4cG9ydC5wbApAQCAtNDgsNyArNDgsNyBA
QAogCXsKIAkJIyBkb24ndCBkbyBhbnl0aGluZyB3aXRoIGEgY29tbWVudAogCX0KLQllbHNp
ZiAoL14oW15cc10rKVxzKyhbXlxzXSspLykKKwllbHNpZiAoL14oXFMrKVxzKyhcUyspLykK
IAl7CiAJCWlmICgkZm9ybWF0IGVxICdhaXgnKQogCQl7CkBAIC03OSw1ICs3OSwzIEBACiB9
OwogIjsKIH0KLQotZXhpdCgwKTsKLS0gCjIuMzcuMQoK

--------------qvaZz9nPpS4djhHELPkod5Qn--