Re: meson vs. llvm bitcode files

On 16.01.26 12:33, Nazir Bilal Yavuz wrote:
> Hi,
>
> On Fri, 31 Oct 2025 at 15:13, Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
>>
>> On Wed, 13 Aug 2025 at 16:25, Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
>>>
>>> Hi,
>>>
>>> On Mon, 7 Jul 2025 at 11:45, Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
>>>>
>>>> Mandatory rebase, v6 is attached.
>>>
>>> Rebase is needed due to 01d6832c10, v7 is attached.
>>
>> Rebase is needed due to 16607718c0, v8 is attached.
>
> Rebase is needed. Also, there is small functional change in 0002:
>
> def remove_duplicates(duplicate_str):
> - words = duplicate_str.split()
> + # Remove duplicates based on basename as there could be a mix of both full
> + # paths and bare binary names.
> + words = [os.path.basename(word) for word in duplicate_str.split()]
> return ' '.join(sorted(set(words), key=words.index))
>
> It is because MacOS was failing due to there being 2 instances of
> ccache, one is with full path '/opt/local/bin/ccache' and one is just
> the binary name 'ccache'. remove_duplicates() function did not remove
> them as it compared full strings before, now it compares only
> basenames.

Some review comments from me.

v9-0001-meson-Add-postgresql-extension.pc-for-building-ex.patch

Need to think about whether "extension" is the correct term.

New meson message:

NOTICE: Future-deprecated features used:
* 0.62.0: {'pkgconfig.generate variable for builtin directories'}

The comment that introduces postgresql-extension-warnings.pc says

+# Extension modules should likely also use -fwrapv etc. But it it's a
bit odd
+# to expose it to a .pc file?

but then -fwrapv ends up in postgresql-extension.pc anyway. Not sure
what was intended here.

Also, the description "PostgreSQL Extension Support - Compiler
Warnings" could be clarified, like "with recommended compiler
warnings" or "with compiler warnings same as core code" or similar.

The Requires list in my case is for example

Requires: krb5-gssapi, icu-uc, icu-i18n, ldap, libxml-2.0 >= 2.6.23,
liblz4, openssl, zlib, libzstd >= 1.4.0

but I don't think these are actually required for building extensions
(unless a particular extension directly makes use of one of them, in
which case they should declare that on their own).

If we are going to install these .pc files, we also need to build them
with with makefiles. Alternatively, we could not install them for now
and just use them internally.

v9-0002-meson-Test-building-extensions-by-using-postgresq.patch

Not sure if this was meant to be kept or it's just for local testing.

New meson warnings:

WARNING: Deprecated features used:
* 0.55.0: {'ExternalProgram.path'}
* 0.56.0: {'meson.build_root'}

src/test/modules/ seems like the wrong location, since it's not a
module or a test module.

I don't know if it's possible to make meson use a different file than
meson.build, but if so, it might be better to keep these test
meson.build files together with their extensions, like
contrib/amcheck/meson-test.build. Similar to how we have "PGXS" build
support in the makefiles. Otherwise, I'm afraid this will get
annoying and error-prone if one has to remember to update other files
under src/test/ when adding for example a new .sql file to amcheck.

Also, the driver script is at 'src/tools/ci/test_meson_extensions',
but you are using it outside of CI, so that's not a good location.

v9-0003-meson-WIP-Add-docs-for-postgresql-extension.pc.patch

Let's not rename existing ids.

It seems to me that the .pc file can also be used without meson.
Let's take that into account a bit. For example, the
id="extend-postgres-meson" could be id="extend-postgres-pkg-config" or
similar.

Your text ends with a colon. Did you mean to add more text? Maybe an
example meson.build would be good.

v9-0005-meson-Add-LLVM-bitcode-emissions-for-contrib-libr.patch

+# some libraries include "hstore/hstore.h" instead of "hstore.h"

It seems to me that the former is correct, but if not then we should
fix it.