[PATCH] Fix missing comma in Requires.private with a Make macro
Hello!
Wolfgang reported over in [1]/messages/by-id/9badbeeb-a432-48d4-8710-c8254a54d428@technowledgy.de 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]: /messages/by-id/9badbeeb-a432-48d4-8710-c8254a54d428@technowledgy.de
Attachments:
0001-oauth-Correct-missing-comma-in-Requires.private.patchapplication/octet-stream; name=0001-oauth-Correct-missing-comma-in-Requires.private.patchDownload
From 26aa7d22abf1d16d5514c01bbff268126464eacd Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Tue, 6 May 2025 12:49:33 -0700
Subject: [PATCH] oauth: Correct missing comma in Requires.private
I added libcurl to the Requires.private section of libpq.pc in commit
b0635bfda, but I missed that the Autoconf side needs commas added
explicitly. Configurations which used both --with-libcurl and
--with-openssl ended up with the following entry:
Requires.private: libssl, libcrypto libcurl
The pkg-config parser appears to be fairly lenient in this case, and
accepts the whitespace as an equivalent separator, but let's not rely on
that. Add an add_to_list macro (inspired by Makefile.global's
add_to_path) to build up the PKG_CONFIG_REQUIRES_PRIVATE list correctly.
Reported-by: Wolfgang Walther <walther@technowledgy.de>
---
src/interfaces/libpq/Makefile | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index c6fe5fec7f6..853aab4b1b8 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -98,14 +98,21 @@ SHLIB_PREREQS = submake-libpgport
SHLIB_EXPORTS = exports.txt
+# Appends to a comma-separated list.
+comma := ,
+define add_to_list
+$(eval $1 := $(if $($1),$($1)$(comma) $2,$2))
+endef
+
ifeq ($(with_ssl),openssl)
-PKG_CONFIG_REQUIRES_PRIVATE = libssl, libcrypto
+$(call add_to_list,PKG_CONFIG_REQUIRES_PRIVATE,libssl)
+$(call add_to_list,PKG_CONFIG_REQUIRES_PRIVATE,libcrypto)
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.
-PKG_CONFIG_REQUIRES_PRIVATE += libcurl
+$(call add_to_list,PKG_CONFIG_REQUIRES_PRIVATE,libcurl)
%.pc: override SHLIB_LINK_INTERNAL += -lpq-oauth
endif
--
2.34.1
On Wed, May 7, 2025 at 2:17 PM Jacob Champion <
jacob.champion@enterprisedb.com> wrote:
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.
LGTM
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?
--
Fabrízio de Royes Mello
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-bit executable 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
-lpqBut 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]https://www.msys2.org/docs/pkgconfig/#cflagsprivate-static-libraries. 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-1That 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-1That 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 -pthreadPrettier, 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
endifall: 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 -pthreadPrettier, 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]https://github.com/pkgconf/pkgconf/issues/318 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]https://github.com/mesonbuild/meson/pull/12227.)
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]/messages/by-id/9badbeeb-a432-48d4-8710-c8254a54d428@technowledgy.de
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]: /messages/by-id/9badbeeb-a432-48d4-8710-c8254a54d428@technowledgy.de