Обсуждение: [PATCH] Fix missing comma in Requires.private with a Make macro

Hello!

Wolfgang reported over in [1] that I've missed a comma when appending
to the PKG_CONFIG_REQUIRES_PRIVATE list, making libpq.pc look ugly:

    Requires.private: libssl, libcrypto libcurl

pkg-config itself appears to be papering over my mistake (a quick code
inspection suggests it treats commas as equivalent to whitespace?) but
here's a patch to fix it. I went with a macro inspired by
Makefile.global's add_to_path, to try to ease this for whoever comes
next.

Tested with GNU Make 3.81 (the compilation of which was slightly
painful; does anyone want to debate pulling that minimum version up
sometime soon?) and 4.3.

Thanks,
--Jacob

[1] https://postgr.es/m/9badbeeb-a432-48d4-8710-c8254a54d428%40technowledgy.de

On Wed, May 7, 2025 at 11:55 AM Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> LGTM

Thanks for the review!

>> Tested with GNU Make 3.81 (the compilation of which was slightly
>> painful; does anyone want to debate pulling that minimum version up
>> sometime soon?) and 4.3.
>
> Not sure if all animals have a minimum 4.3 make version. Do you?

Oh, I doubt that very much. I don't immediately see the Make version
in the configure logs for the animals, but lapwing for example is
running Debian Wheezy.

--Jacob

Jacob Champion <jacob.champion@enterprisedb.com> writes:
> On Wed, May 7, 2025 at 11:55 AM Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
>> Not sure if all animals have a minimum 4.3 make version. Do you?

> Oh, I doubt that very much. I don't immediately see the Make version
> in the configure logs for the animals, but lapwing for example is
> running Debian Wheezy.

Apple is still shipping

$ which make
/usr/bin/make

$ make --version
GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

This program built for i386-apple-darwin11.3.0

I'm not sure why it claims to be an i386 build, because that's
the one thing it certainly isn't:

$ file /usr/bin/make
/usr/bin/make: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64] [arm64e:Mach-O
64-bitexecutable arm64e] 
/usr/bin/make (for architecture x86_64):        Mach-O 64-bit executable x86_64
/usr/bin/make (for architecture arm64e):        Mach-O 64-bit executable arm64e

But at any rate, dropping compatibility with that would likely
be rather painful for some of us, even though most people building
PG on Mac probably use Homebrew or MacPorts and could get a newer
make version from there.

The next oldest make version we still have to contend with in the
wild might be RHEL8's 4.2.1.

            regards, tom lane

On 2025-May-07, Tom Lane wrote:

> Apple is still shipping
> 
> $ which make
> /usr/bin/make
> 
> $ make --version
> GNU Make 3.81

Well, Jacob did say that he tested it with 3.81, so this patch should be
okay.  Upping the minimum version can be discussed elsewhere ... or
maybe never, if we end up replacing it wholesale with Meson/ninja.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Wed, May 7, 2025 at 3:39 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> Well, Jacob did say that he tested it with 3.81, so this patch should be
> okay.  Upping the minimum version can be discussed elsewhere ... or
> maybe never, if we end up replacing it wholesale with Meson/ninja.

To be fair, I did invite debate. I don't mind if it happens here. :D

Sounds like the debate might be fairly short, though. I was hoping the
choice of Make came from that old Debian box. But since it's actually
coming from Apple and I guess they have no plans to ever update... I
don't think you all need to change macOS setups on my account. I'd
imagine that people are not writing new Makefile macros very often.

Is there maybe a wiki page where we gather the reasoning for our
pinned minimum versions? None of my searches turned anything up, but
it might help accelerate some of these conversations.

Thanks!
--Jacob

Jacob Champion <jacob.champion@enterprisedb.com> writes:
> Is there maybe a wiki page where we gather the reasoning for our
> pinned minimum versions? None of my searches turned anything up, but
> it might help accelerate some of these conversations.

I'm not aware of one, but it seems like a reasonable idea ...

            regards, tom lane

On Thu, May 8, 2025 at 1:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm not aware of one, but it seems like a reasonable idea ...

I've created a skeleton at
https://wiki.postgresql.org/wiki/Minimum_Dependency_Versions, based on
our dev documentation and some recent threads. Still plenty to fill
in, if anyone knows bits and pieces.

Thanks!
--Jacob

Re: Jacob Champion
> Wolfgang reported over in [1] that I've missed a comma when appending
> to the PKG_CONFIG_REQUIRES_PRIVATE list, making libpq.pc look ugly:
> 
>     Requires.private: libssl, libcrypto libcurl

Is adding libcurl there even the right move? This will make all
programs using pkgconf and linking against libpq require
Build-Depending on libcurl-dev (or more specifically,
libcurl4-openssl-dev). Alternatively, we could make libpq-dev
Depends: libcurl on libcurl-dev. Both solutions seem wrong.

Since nothing in libpq should need curl for compiling, should we drop
it there instead?

Spotted by https://ci.debian.net/packages/h/hoel/unstable/amd64/60438300/
which used PG18. (Which might have more problems as well...)

Christoph

On Mon, May 12, 2025 at 3:50 AM Christoph Berg <myon@debian.org> wrote:
> Since nothing in libpq should need curl for compiling, should we drop
> it there instead?

The static build (libpq.a) still needs libcurl. The module is only
compiled for use by the shared library.

--Jacob

On Mon, May 12, 2025 at 8:49 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Mon, May 12, 2025 at 3:50 AM Christoph Berg <myon@debian.org> wrote:
> > Since nothing in libpq should need curl for compiling, should we drop
> > it there instead?
>
> The static build (libpq.a) still needs libcurl. The module is only
> compiled for use by the shared library.

Sorry, typing too fast in the airport. The _clients_ of libpq.a still
need libcurl, after they link libpq-oauth.a for the missing symbols.
Isn't that what Requires.private is for?

--Jacob

Re: Jacob Champion
> > > Since nothing in libpq should need curl for compiling, should we drop
> > > it there instead?
> >
> > The static build (libpq.a) still needs libcurl. The module is only
> > compiled for use by the shared library.
> 
> Sorry, typing too fast in the airport. The _clients_ of libpq.a still
> need libcurl, after they link libpq-oauth.a for the missing symbols.
> Isn't that what Requires.private is for?

Sorry for not following up here earlier. I've just uploaded 18rc1 to
Debian unstable to pave the way for the 18.0 release, and this
regression in "hoel" popped up again.

It turns out that pkg-config is picky when it doesn't find the .pc
files listed in Requires.private. So, without libcurl4-openssl-dev
installed:

Libs handling is ok:

$ pkgconf --libs libpq
-lpq

But CFLAGS is not:

$ pkgconf --cflags libpq
Package libcurl was not found in the pkg-config search path.
Perhaps you should add the directory containing `libcurl.pc'
to the PKG_CONFIG_PATH environment variable
Package 'libcurl', required by 'libpq', not found

This "we don't care about the difference between static and dynamic in
CFLAGS" part of pkg-confg is actually a FAQ item:

  3. My library z uses libx internally, but does not expose libx data
  types in its public API. What do I put in my z.pc file?

  Again, add the module to Requires.private if it supports pkg-config.
  In this case, the compiler flags will be emitted unnecessarily, but
  it ensures that the linker flags will be present when linking
  statically. If libx does not support pkg-config, add the necessary
  linker flags to Libs.private.

https://people.freedesktop.org/~dbn/pkg-config-guide.html#faq

Since most libpq users are linking dynamically (and certainly
everything in Debian), I will now patch libpq.pc to drop libcurl from
Requires.private and add -lcurl to Libs.private.

That way, users trying to link statically will get a meaningful
"libcurl not found" message instead of randomly missing symbols.

I would say this change should make it into 18.1's libpq.pc file.

Thanks to Timo Röhling for helping me understand this issue.

Christoph

Re: To Jacob Champion
> It turns out that pkg-config is picky when it doesn't find the .pc
> files listed in Requires.private. So, without libcurl4-openssl-dev
> installed:

What I didn't say explicitly in the last message is that I want to
avoid that all programs now build-depending on libpq-dev would have to
add libcurl4-openssl-dev, or libpq-dev would have to depend on that.

> But CFLAGS is not:
> 
> $ pkgconf --cflags libpq
> Package libcurl was not found in the pkg-config search path.

Furthermore, with libcurl4-openssl-dev installed, the pkgconf output
changes, as said in that FAQ entry:

$ pkgconf --cflags libpq
-I/usr/include/postgresql -I/usr/include/x86_64-linux-gnu -isystem /usr/include/mit-krb5 -I/usr/include/p11-kit-1

That might be harmless, but we should avoid it.


For reference, the current 18.0 behavior:

$ pkgconf --libs libpq
-lpq
$ pkgconf --cflags libpq
-I/usr/include/postgresql -I/usr/include/x86_64-linux-gnu -isystem /usr/include/mit-krb5 -I/usr/include/p11-kit-1
$ pkgconf --libs libpq
-lpq
$ pkgconf --libs --static libpq
-lpq -lpgcommon -lpgport -lpq-oauth -lssl -lcrypto -lgssapi_krb5 -lm -lldap -lssl -lz -lzstd -ldl -pthread -lcrypto -lz
-lzstd-ldl -pthread -lcurl -lidn2 -llber -lldap -llber -lssh2 -lssl -lcrypto -lcrypto -lunistring -lz -lbrotlidec
-lbrotlicommon-lzstd -pthread -L/usr/lib/x86_64-linux-gnu/mit-krb5 -lgssapi_krb5 -L/usr/lib/x86_64-linux-gnu/mit-krb5
-lkrb5-lk5crypto -lcom_err -lkrb5support -lssl -lz -lzstd -ldl -pthread -lcrypto -lz -lzstd -ldl -pthread -lpsl -lssh2
-lssl-lcrypto -lz -lgpg-error -lcrypto -lzstd -ldl -pthread -lz -lrtmp -lz -lgmp -lgnutls -lgmp -lunistring -latomic
-lnettle-lgmp -lnettle -ltasn1 -lidn2 -lunistring -lp11-kit -lhogweed -lgmp -lnettle -lnghttp2 -lnghttp3
 

(ugh)

And after the libpq.pc change I plan:

$ pkgconf --cflags libpq
-I/usr/include/postgresql
$ pkgconf --cflags --static libpq
-I/usr/include/postgresql
$ pkgconf --libs libpq
-lpq
$ pkgconf --libs --static libpq
-lpq -lpgcommon -lpgport -lpq-oauth -lssl -lcrypto -lgssapi_krb5 -lm -lldap -lcurl -lssl -lz -lzstd -ldl -pthread
-lcrypto-lz -lzstd -ldl -pthread
 


Christoph

On Tue, Sep 23, 2025 at 7:01 AM Christoph Berg <myon@debian.org> wrote:
> Libs handling is ok:
>
> $ pkgconf --libs libpq
> -lpq
>
> But CFLAGS is not:
>
> $ pkgconf --cflags libpq
> Package libcurl was not found in the pkg-config search path.
> Perhaps you should add the directory containing `libcurl.pc'
> to the PKG_CONFIG_PATH environment variable
> Package 'libcurl', required by 'libpq', not found

Odd.

> This "we don't care about the difference between static and dynamic in
> CFLAGS" part of pkg-confg is actually a FAQ item:
>
>   3. My library z uses libx internally, but does not expose libx data
>   types in its public API. What do I put in my z.pc file?
>
>   Again, add the module to Requires.private if it supports pkg-config.
>   In this case, the compiler flags will be emitted unnecessarily, but
>   it ensures that the linker flags will be present when linking
>   statically. If libx does not support pkg-config, add the necessary
>   linker flags to Libs.private.

1) We're doing what they recommend, aren't we?
2) pkgconf does care about the difference [1]. Windows builds of
libpq-oauth aren't relevant yet, but libcurl does indeed ship a
Cflags.private setting.

> Since most libpq users are linking dynamically (and certainly
> everything in Debian), I will now patch libpq.pc to drop libcurl from
> Requires.private and add -lcurl to Libs.private.

Makes sense.

> I would say this change should make it into 18.1's libpq.pc file.

As in, 18.1 on Debian? Or are you suggesting we apply that change in Postgres?

On Tue, Sep 23, 2025 at 7:16 AM Christoph Berg <myon@debian.org> wrote:
> What I didn't say explicitly in the last message is that I want to
> avoid that all programs now build-depending on libpq-dev would have to
> add libcurl4-openssl-dev, or libpq-dev would have to depend on that.

For a platform that doesn't really do staticlibs, I think I definitely
agree with that.

> Furthermore, with libcurl4-openssl-dev installed, the pkgconf output
> changes, as said in that FAQ entry:
>
> $ pkgconf --cflags libpq
> -I/usr/include/postgresql -I/usr/include/x86_64-linux-gnu -isystem /usr/include/mit-krb5 -I/usr/include/p11-kit-1
>
> That might be harmless, but we should avoid it.

I think we can't avoid it; if anyone wants to get Windows support
working they'll need -DCURL_STATICLIB as provided by libcurl.pc.

> $ pkgconf --libs --static libpq
> -lpq -lpgcommon -lpgport -lpq-oauth -lssl -lcrypto -lgssapi_krb5 -lm -lldap -lssl -lz -lzstd -ldl -pthread -lcrypto
-lz-lzstd -ldl -pthread -lcurl -lidn2 -llber -lldap -llber -lssh2 -lssl -lcrypto -lcrypto -lunistring -lz -lbrotlidec
-lbrotlicommon-lzstd -pthread -L/usr/lib/x86_64-linux-gnu/mit-krb5 -lgssapi_krb5 -L/usr/lib/x86_64-linux-gnu/mit-krb5
-lkrb5-lk5crypto -lcom_err -lkrb5support -lssl -lz -lzstd -ldl -pthread -lcrypto -lz -lzstd -ldl -pthread -lpsl -lssh2
-lssl-lcrypto -lz -lgpg-error -lcrypto -lzstd -ldl -pthread -lz -lrtmp -lz -lgmp -lgnutls -lgmp -lunistring -latomic
-lnettle-lgmp -lnettle -ltasn1 -lidn2 -lunistring -lp11-kit -lhogweed -lgmp -lnettle -lnghttp2 -lnghttp3 
>
> (ugh)

Very ugly, but looks correct to me at a glance. Transitive
dependencies are fun...

> And after the libpq.pc change I plan:
> [...]
> $ pkgconf --libs --static libpq
> -lpq -lpgcommon -lpgport -lpq-oauth -lssl -lcrypto -lgssapi_krb5 -lm -lldap -lcurl -lssl -lz -lzstd -ldl -pthread
-lcrypto-lz -lzstd -ldl -pthread 

Prettier, but wrong? I don't think that's going to link, is it?

Thanks,
--Jacob

[1] https://www.msys2.org/docs/pkgconfig/#cflagsprivate-static-libraries

Re: Jacob Champion
> 1) We're doing what they recommend, aren't we?

Maybe by the letter, but I think we have a real problem and need to do
something.

> > I would say this change should make it into 18.1's libpq.pc file.
> 
> As in, 18.1 on Debian? Or are you suggesting we apply that change in Postgres?

In Postgres. If we want more libpq.pc adoption in client programs (and
less snowflake solutions based on pg_config), we should make it
hassle-free. Otherwise these users will have to also install libcurl
to compile, even if it's not used, and then the security people will
fire up their scanning tools which we wanted to avoid.

> > $ pkgconf --cflags libpq
> > -I/usr/include/postgresql -I/usr/include/x86_64-linux-gnu -isystem /usr/include/mit-krb5 -I/usr/include/p11-kit-1
> >
> > That might be harmless, but we should avoid it.
> 
> I think we can't avoid it; if anyone wants to get Windows support
> working they'll need -DCURL_STATICLIB as provided by libcurl.pc.

That would be in Cflags.private.

> > $ pkgconf --libs --static libpq
> > -lpq -lpgcommon -lpgport -lpq-oauth -lssl -lcrypto -lgssapi_krb5 -lm -lldap -lcurl -lssl -lz -lzstd -ldl -pthread
-lcrypto-lz -lzstd -ldl -pthread
 
> 
> Prettier, but wrong? I don't think that's going to link, is it?

Wrong how?

The patch I ended up using is this:

--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -110,10 +110,7 @@ $(call add_to_list,PKG_CONFIG_REQUIRES_P
 endif
 
 ifeq ($(with_libcurl),yes)
-# libpq.so doesn't link against libcurl, but libpq.a needs libpq-oauth, and
-# libpq-oauth needs libcurl. Put both into *.private.
-$(call add_to_list,PKG_CONFIG_REQUIRES_PRIVATE,libcurl)
-%.pc: override SHLIB_LINK_INTERNAL += -lpq-oauth
+%.pc: override SHLIB_LINK_INTERNAL += -lpq-oauth -lcurl
 endif
 
 all: all-lib libpq-refs-stamp

I tried looking at meson.build as well, but couldn't figure out how to
do exactly this, and since the Debian package hasn't switched yet,
this was good enough.

Christoph

On Tue, Sep 23, 2025 at 12:42 PM Christoph Berg <myon@debian.org> wrote:
> Maybe by the letter, but I think we have a real problem and need to do
> something.

If I break Wolfgang's use case again, we will still have a real problem.

> > I think we can't avoid it; if anyone wants to get Windows support
> > working they'll need -DCURL_STATICLIB as provided by libcurl.pc.
>
> That would be in Cflags.private.

Yes.

> > > $ pkgconf --libs --static libpq
> > > -lpq -lpgcommon -lpgport -lpq-oauth -lssl -lcrypto -lgssapi_krb5 -lm -lldap -lcurl -lssl -lz -lzstd -ldl -pthread
-lcrypto-lz -lzstd -ldl -pthread 
> >
> > Prettier, but wrong? I don't think that's going to link, is it?
>
> Wrong how?

It doesn't link. You'll be missing an incredible number of symbols.

--Jacob

On Tue, Sep 23, 2025 at 2:15 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> > That would be in Cflags.private.
>
> Yes.

The FAQ you cite says

> In either case, pkg-config will output the [private dependency's] compiler flags when --static is used or not.

which seems awfully counterproductive, and was apparently
controversial when it was proposed in 2005 [1, 2]. This is why --libs
works and --cflags doesn't. Now that pkg-config's successor supports
Cflags.private, I wonder if there's room to fix these
mutually-conflicting use cases...

--Jacob

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=340904#10
[2] https://bugzilla.gnome.org/show_bug.cgi?id=322240

On Tue, Sep 23, 2025 at 2:57 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Now that pkg-config's successor supports
> Cflags.private, I wonder if there's room to fix these
> mutually-conflicting use cases...

This appears to be discussed in great detail here:

    https://gitlab.freedesktop.org/pkg-config/pkg-config/-/issues/7

On my machine, pkgconf appears to recognize Requires.internal, but
unfortunately it doesn't seem to ignore the internal requirements for
--cflags. So... I don't get it.

--Jacob

On Tue, Sep 23, 2025 at 4:30 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> On my machine, pkgconf appears to recognize Requires.internal, but
> unfortunately it doesn't seem to ignore the internal requirements for
> --cflags. So... I don't get it.

To avoid having this hang in the air forever: there appears to be
considerable confusion and/or disagreement [1] about the meaning of an
internal dependency. Probably not too surprising, since the proposal I
linked above was implemented in pkgconf before the final patchset was
proposed to pkg-config. So it just kind of does whatever it does
today, I guess. (This behavior is problematic for Meson as well [2].)

The original pkg-config maintainers in that freedesktop thread appear
to have responded to the Debian use case (where you want to minimize
the number of needed developer dependencies) with EDONTCARE. Not much
I can do about that. I think that moving this forward is going to need
multiple interested communities politely lobbying pkgconf to change
behavior.

I'm not currently planning to change the .pc file for 18.1, since
there's a way forward for Debian (albeit one we really don't like),
and the current behavior was crafted to meet a requested use case.
IMO, changes we make here should not regress what was tested in [3]
unless we really cannot bear the cost. We need a working
Requires.internal to make everyone happy.

Thanks,
--Jacob

[1] https://github.com/pkgconf/pkgconf/issues/318
[2] https://github.com/mesonbuild/meson/pull/12227
[3] https://postgr.es/m/9badbeeb-a432-48d4-8710-c8254a54d428%40technowledgy.de