Bug 87154

Took me a sec to understand what was being proposed until I read the AX_PKG_CHECK_MODULES page. I definitely see the usefulness of this. If there's a clean way to add it, I have no objections.

I don't think this could go in now for a couple reasons.

1. There's no handling of errors from PKG_CHECK_MODULES. The ACTION-IF-FOUND and ACTION-IF-NOT-FOUND args aren't wrapped, so there's no way to handle errors. It's quite common to call PKG_CHECK_MODULES and then fall back to a different package set if it fails. This only allows the pattern where a fixed set of required packages is used.

2. Some naming needs to be figured out. Since the interface doesn't match PKG_CHECK_MODULES, it needs to be a separate macro. I can't think of what a good name would be.

(In reply to Dan Nicholson from comment #1)
> Took me a sec to understand what was being proposed until I read the
> AX_PKG_CHECK_MODULES page. I definitely see the usefulness of this. If
> there's a clean way to add it, I have no objections.

Sorry, bit of a waffly comment #0. I had hoped the macro page would explain it. :-)

> 1. There's no handling of errors from PKG_CHECK_MODULES. The ACTION-IF-FOUND
> and ACTION-IF-NOT-FOUND args aren't wrapped, so there's no way to handle
> errors. It's quite common to call PKG_CHECK_MODULES and then fall back to a
> different package set if it fails. This only allows the pattern where a
> fixed set of required packages is used.

That should be easily fixable.

https://github.com/peti/autoconf-archive/pull/9

> 2. Some naming needs to be figured out. Since the interface doesn't match
> PKG_CHECK_MODULES, it needs to be a separate macro. I can't think of what a
> good name would be.

Yeah, I was struggling to come up with a good name too. How about:
 • PKG_CHECK_MODULES_SPLIT  (generally naff)
 • PKG_CHECK_DEPENDENCIES  (could be confused with MODULES)
 • PKG_CHECK_REQUIRES  (could be confused with something which reads the Requires variable)

Those are the best I can come up with. :-(

(In reply to Philip Withnall from comment #2)
> That should be easily fixable.
> 
> https://github.com/peti/autoconf-archive/pull/9

One other suggested change from AX_PKG_CHECK_MODULES is to swap the order of public and private lists, so private comes first, since it should be used more commonly. I’m on the fence about this, for the reasons in my comment linked below:

https://tecnocode.co.uk/2014/12/09/a-checklist-for-writing-pkg-config-files/comment-page-1/#comment-5374

Ping?

Again I'm slow. Sorry about that.

I definitely like having private come first since (as the commenter said), it's likely you have no public deps at all. As for a name, how about PKG_CHECK_MODULES_SUBST? Then it's just like PKG_CHECK_MODULES, but it adds substitution of the specified modules. Which is the entire purpose to use this macro and not just open code the substitution of the modules.

If you agree with those changes, can you create a patch for pkg.m4? I think this would be a nice addition.

Created attachment 113745 [details] [review]
pkg: Add PKG_CHECK_MODULES_SUBST macro

This is a tweaked version of PKG_CHECK_MODULES which supports separating
out the private and public dependencies and substituting them into the
.pc file using autoconf magic.

See the documentation for the macro for more information.

Looks good. Thanks for adding all the documentation. Just 2 things that came to mind reading this.

In pkg.m4, please remove the indenting in the definition to match the rest of the macros. More importantly, I just caught that the substitution variables are by default PKG_REQUIRES_PRIVATE and PKG_REQUIRES. That to me doesn't match the PKG_CHECK_MODULES pattern where the PREFIX is used.

I.e., if it did PKG_CHECK_MODULES_SUBST([GLIB],[gthread-2.0],[glib-2.0 gio-2.0]), I'd expect that the substitution variables would be GLIB_REQUIRES_PRIVATE and GLIB_REQUIRES. What do you think?

Created attachment 114072 [details] [review]
pkg: Add PKG_CHECK_MODULES_SUBST macro

This is a tweaked version of PKG_CHECK_MODULES which supports separating
out the private and public dependencies and substituting them into the
.pc file using autoconf magic.

See the documentation for the macro for more information.

(In reply to Dan Nicholson from comment #7)
> Looks good. Thanks for adding all the documentation. Just 2 things that came
> to mind reading this.
> 
> In pkg.m4, please remove the indenting in the definition to match the rest
> of the macros.

Done.

> More importantly, I just caught that the substitution
> variables are by default PKG_REQUIRES_PRIVATE and PKG_REQUIRES. That to me
> doesn't match the PKG_CHECK_MODULES pattern where the PREFIX is used.

That’s intentional. The idea is that you can do:

PKG_CHECK_MODULES_SUBST([GLIB],[gthread-2.0],[glib-2.0])
PKG_CHECK_MODULES_SUBST([DBUS],[gio-2.0])
PKG_CHECK_MODULES_SUBST([OPTIONAL],[libprivate],[libpublic],[],[have_optional=no])

and afterwards you will have:

PKG_REQUIRES_PRIVATE="gthread-2.0 gio-2.0 libprivate"
PKG_REQUIRES="glib-2.0 libpublic"

This gives two variables to substitute into the .pc file. If the PREFIX were used in the variable names, you would have to remember to update the .pc file whenever you add a new PKG_CHECK_MODULES_SUBST() call in configure.in, which I bet people would forget to do — the whole point of this macro is to make it easy to keep Requires[.private] up to date in the .pc file.

(In reply to Philip Withnall from comment #9)
> (In reply to Dan Nicholson from comment #7)
> 
> That’s intentional. The idea is that you can do:
> 
> PKG_CHECK_MODULES_SUBST([GLIB],[gthread-2.0],[glib-2.0])
> PKG_CHECK_MODULES_SUBST([DBUS],[gio-2.0])
> PKG_CHECK_MODULES_SUBST([OPTIONAL],[libprivate],[libpublic],[],
> [have_optional=no])
> 
> and afterwards you will have:
> 
> PKG_REQUIRES_PRIVATE="gthread-2.0 gio-2.0 libprivate"
> PKG_REQUIRES="glib-2.0 libpublic"
> 
> This gives two variables to substitute into the .pc file. If the PREFIX were
> used in the variable names, you would have to remember to update the .pc
> file whenever you add a new PKG_CHECK_MODULES_SUBST() call in configure.in,
> which I bet people would forget to do — the whole point of this macro is to
> make it easy to keep Requires[.private] up to date in the .pc file.

Sure, I understand the reason. Just that the semantics are different than PKG_CHECK_MODULES. Not only is the prefix variable not being used, but this also accumulates whereas PKG_CHECK_MODULES (for better or worse) is used once per prefix.

People remembering to add new prefixes to their files is no different than when they need to add the PREFIX_CFLAGS/LIBS to their Makefile.am. I don't think forgetting to update a file is extremely compelling. More importantly, this also means that if you have multiple libraries with different requirements, then your PKG_CHECK_MODULES_SUBST calls become unnecessarily verbose.

PKG_CHECK_MODULES_SUBST([FOO], [liba], [], [], [], [FOO_PRIVATE], [FOO_PUBLIC])
PKG_CHECK_MODULES_SUBST([BAR], [libb], [], [], [], [BAR_PRIVATE], [BAR_PUBLIC])

If the default was to use the prefix, this obviously wouldn't happen.

I definitely like what you've done here and appreciate that it was done primarly to be an easy to use interface. I just need to make sure that's correct before committing to it. My gut tells me that I should make this follow the semantics of PKG_CHECK_MODULES since people know that interface extremely well.

(In reply to Dan Nicholson from comment #10)
> People remembering to add new prefixes to their files is no different than
> when they need to add the PREFIX_CFLAGS/LIBS to their Makefile.am. I don't
> think forgetting to update a file is extremely compelling.

I think updating Requires[.private] in the .pc.in file and adding PREFIX_[CFLAGS|LIBS] to the Makefile.am are quite different situations. Without PREFIX_[CFLAGS|LIBS] in the Makefile.am, the project won’t build, so developers have a definite incentive to get that right. That’s not true for updates to .pc.in — outdated dependency lists there are typically only detected months down the line, after something’s released.

Using multiple prefixed variables in Requires[.private] is one step better than listing the required packages separately, but still requires manual intervention to keep up-to-date, so I believe it’s doomed to get outdated in modules.

The only solutions I can think of are this one, or some kind of distcheck target which fails if Requires[.private] is outdated. I’m not sure how that would work though.

I realise this is not very helpful, but I can’t think of any other solutions. I feel the argument about _never_ having to edit the .pc.in file is quite compelling.

> More importantly, this also means that if you have multiple libraries with
> different requirements, then your PKG_CHECK_MODULES_SUBST calls become
> unnecessarily verbose.
>
> PKG_CHECK_MODULES_SUBST([FOO], [liba], [], [], [], [FOO_PRIVATE],
> [FOO_PUBLIC])
> PKG_CHECK_MODULES_SUBST([BAR], [libb], [], [], [], [BAR_PRIVATE],
> [BAR_PUBLIC])

We could rearrange the arguments to move PRIVATE-VARIABLE and PUBLIC-VARIABLE before ACTION-IF-FOUND and ACTION-IF-NOT-FOUND…but then we’d have the same problem if actions need to be performed based on the pkg-config checks.

I think the current variable ordering is optimal given how the macros currently function.

> I definitely like what you've done here and appreciate that it was done
> primarly to be an easy to use interface. I just need to make sure that's
> correct before committing to it. My gut tells me that I should make this
> follow the semantics of PKG_CHECK_MODULES since people know that interface
> extremely well.

Definitely, let’s get the right approach.

(In reply to Dan Nicholson from comment #10)
> PKG_CHECK_MODULES_SUBST([FOO], [liba], [], [], [], [FOO_PRIVATE],
> [FOO_PUBLIC])
> PKG_CHECK_MODULES_SUBST([BAR], [libb], [], [], [], [BAR_PRIVATE],
> [BAR_PUBLIC])

I think this is actually an anti-pattern: the pkg-config macros somewhat assume that the first argument is what you are looking for, not what you are using it for (as you can infer from the message being "checking for FOO...", not "checking for dependencies of FOO...").

So this would perhaps be more realistically:

PKG_CHECK_MODULES_SUBST([LIBA], [liba], [], [], [], [FOO_PRIVATE], [FOO_PUBLIC])
PKG_CHECK_MODULES_SUBST([LIBB], [libb], [], [], [], [BAR_PRIVATE], [BAR_PUBLIC])

May I suggest a compromise?

* PKG_CHECK_MODULES_SUBST([X], ...) redefines and substs X_PRIVATE and X_PUBLIC
* PKG_CHECK_MODULES_SUBST([X], ...) *also* appends to its 6th and 7th arguments,
  defaulting to PKG_PRIVATE and PKG_PUBLIC (or whatever)

For the common case where all modules (libraries/executables) in a project have the same dependencies (e.g. telepathy-glib), it's easy: put @PKG_PRIVATE@ and @PKG_PUBLIC@ in telepathy-glib.pc.in and the right thing happens.

For the slightly less common case where all libraries depend on a subset of the dependencies of all modules (e.g. dbus, where dbus-daemon and dbus-launch each have more dependencies than libdbus), you can use PKG_CHECK_MODULES_SUBST for libdbus' dependencies, and plain PKG_CHECK_MODULES for executables' dependencies.

For the case where a project builds more than one library (e.g. GLib), there are two options:

* list the dependencies explicitly in each .pc file (no better or worse
  than the status quo)
* use the 6th and 7th arguments

Semi-relatedly, I wonder whether these macros would be better like this:

PKG_CHECK_PUBLIC_DEPENDENCIES([GLIB], [glib-2.0 >= 2.42])
PKG_CHECK_PRIVATE_DEPENDENCIES([DBUS], [dbus-1 >= 1.8])

That's awkward if you're (mis?)using the prefix to represent "thing that depends on libraries", but just as convenient as anything else if you're using the prefix to represent "library or libraries on which we depend".

(In reply to Simon McVittie from comment #12)
> May I suggest a compromise?
> 
> * PKG_CHECK_MODULES_SUBST([X], ...) redefines and substs X_PRIVATE and
> X_PUBLIC
> * PKG_CHECK_MODULES_SUBST([X], ...) *also* appends to its 6th and 7th
> arguments,
>   defaulting to PKG_PRIVATE and PKG_PUBLIC (or whatever)

Seems reasonable to me, but it raises the question of what the PREFIX was originally intended for — to represent a group of related dependencies (potentially all the dependencies of the project, for example), or to represent a single dependency? As you say, that somewhat affects the design of this macro.

This macro, as currently present in the Autoconf Archive, is already very useful.
https://www.gnu.org/software/autoconf-archive/ax_pkg_check_modules.html
Thanks for it! However, it updates AX_PACKAGE_REQUIRES and AX_PACKAGE_REQUIRES_PRIVATE (or the variables specified in place of them) even if PKG_CHECK_MODULES takes the "action-if-not-found" branch. I think the update operations should be moved into the "action-if-found" branch, not taken unconditionally.

(In reply to Ferenc Wágner from comment #15)
> This macro, as currently present in the Autoconf Archive, is already very
> useful.
> https://www.gnu.org/software/autoconf-archive/ax_pkg_check_modules.html
> Thanks for it! However, it updates AX_PACKAGE_REQUIRES and
> AX_PACKAGE_REQUIRES_PRIVATE (or the variables specified in place of them)
> even if PKG_CHECK_MODULES takes the "action-if-not-found" branch. I think
> the update operations should be moved into the "action-if-found" branch, not
> taken unconditionally.

Hmmm. There are arguments for and against this. If your library’s public dependencies change, it’s not API stable and is going to be causing all kinds of problems for consumers of it — that’s an argument to unconditionally update AX_PACKAGE_REQUIRES. On the other hand, private dependencies don’t affect API or ABI stability — which is an argument for conditionally updating AX_PACKAGE_REQUIRES_PRIVATE.

At this point, I think it’s too late to change AX_PKG_CHECK_MODULES since that might break existing applications. However, this feedback should definitely be rolled back into the next iteration of the macro here, before it makes its way into a pkg-config release. I’ll try and do that next time I get round to working on this. Thanks!

(In reply to Philip Withnall from comment #16)
> Hmmm. There are arguments for and against this. If your library’s public
> dependencies change, it’s not API stable and is going to be causing all
> kinds of problems for consumers of it — that’s an argument to
> unconditionally update AX_PACKAGE_REQUIRES.

I much appreciate API stability. But how does updating AX_PACKAGE_REQUIRES with unused modules help with that? If you don't want to support using (non-terminating) action-if-not-found with checks for public modules, then it might be better to introduce AX_PKG_CHECK_PUBLIC_MODULES for that. And then you can have AX_PKG_CHECK_PRIVATE_MODULES for conditionally updating the other part... just some ideas.

(In reply to Ferenc Wágner from comment #17)
> If you don't want to support using
> (non-terminating) action-if-not-found with checks for public modules, then
> it might be better to introduce AX_PKG_CHECK_PUBLIC_MODULES for that. And
> then you can have AX_PKG_CHECK_PRIVATE_MODULES for conditionally updating
> the other part... just some ideas.

This sounds quite a lot like Comment #13 again.

(In reply to Philip Withnall from comment #16)
> If your library’s public
> dependencies change, it’s not API stable and is going to be causing all
> kinds of problems for consumers of it — that’s an argument to
> unconditionally update AX_PACKAGE_REQUIRES.
Forgot to expand on this use case: it isn't that the public dependencies change willy-nilly. They change when different configuration options are selected by the user, and I see nothing wrong with that. Being general enough to represent this would be a merit of the tool. So I'd rather not restrict action-if-not-found to the private case, but include this warning in the documentation.

Due to the rise in popularity of Meson, I don’t really have any interest in working on this macro any more. I’m going to close this as WONTFIX — but if someone else who still cares about autoconf wants to reopen it and update the patch to address the correct use cases, I’m happy for them. :-)