Bug 3550 - [PENDING] Result caching in PKG_CHECK_MODULES() is broken / causes breakage
[PENDING] Result caching in PKG_CHECK_MODULES() is broken / causes breakage
Status: RESOLVED NOTABUG
Product: pkg-config
Classification: Unclassified
Component: src
unspecified
x86 (IA32) Linux (All)
: high normal
Assigned To: Tollef Fog Heen
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-06-16 07:52 UTC by James Henstridge
Modified: 2012-09-10 08:38 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description James Henstridge 2005-06-16 07:52:39 UTC
One of the new features added to pkg.m4 in 0.16 was caching of
PKG_CHECK_MODULES() results.  This caching is done on the basis of the variable
name prefix, which means that consecutive calls using the same variable name but
different package list will get the same result even when they shouldn't.  This
can lead to the following problems:

1. In evolution (before I patched it), it passed "EVOLUTION" as the variable
prefix for a number of unrelated lists of package lists.  The caching caused
them all to get the same set of cflags/libs.

2. In eog, it does the following:
    PKG_CHECK_MODULES(EXIF, libexif >= $LIBEXIF_REQUIRED,
                      have_exif=yes, have_exif=no)
    PKG_CHECK_MODULES(EXIF, libexif = $LIBEXIF_REQUIRED,
                      have_old_libexif=yes, have_old_libexif=no)

The second call sets have_old_libexif=yes even if you have a more recent version
because of the result caching.


If the macro is going to do result caching, it should be on the basis of the
package list passed in rather than the variable name.  However it is a bit
difficult to construct an appropriate shell variable to use as the cache key
(package requirement lists can be quite long, so you might run into variable
name length limits too).

Given that pkg-config invocations are pretty cheap, the easy option would be to
just get rid of the caching behaviour entirely.

What do you think?
Comment 1 Tollef Fog Heen 2005-06-26 13:01:52 UTC
2005-06-26  Tollef Fog Heen  <tfheen@err.no>

	* pkg.m4: Get rid of caching again.  This breaks too much stuff,
	and pkg-config doesn't take much time to run.

Comment 2 Tollef Fog Heen 2005-06-27 14:00:32 UTC
pkg-config 0.18 has now been released with a fix for this bug.
Comment 3 Stanislav Brabec 2006-02-21 22:52:57 UTC
I can reproduce with version 0.20:

----- configure.ac -----
AC_INIT
PKG_CHECK_MODULES([PKGCONF], [pkg-config >= 0.1])
PKG_CHECK_MODULES([PKGCONF], [pkg-config < 0.1])
------------------------
sbrabec> aclocal ; autoconf ; ./configure
checking for pkg-config... /usr/bin/pkg-config
checking pkg-config is at least version 0.9.0... yes
checking for PKGCONF... yes
checking for PKGCONF... yes
sbrabec> pkg-config --version
0.20

It is because you set FOO_CFLAGS resp. FOO_LIBS, and then you skip the check, if
FOO_CFLAGS resp. FOO_LIBS is not empty.

I guess, that you can revert original implementation, add new variable
pkg_cv_FOO_check, which will cache check string and then reply either:

checking for PKGCONF... yes
checking for PKGCONF...
configure: error: PKG_CHECK_MODULES called twice with the same variable PKGCONF
but different checks.

or in case if both tests are equal:

checking for PKGCONF... yes
checking for PKGCONF... yes (cached)

or in case if variables are set:

checking for FOO... yes (using predefined FOO_CFLAGS, FOO_LDFLAGS)
Comment 4 Slawomir Czarko 2012-01-27 01:51:37 UTC
(In reply to comment #1)
> 2005-06-26  Tollef Fog Heen  <tfheen@err.no>
> 
> 	* pkg.m4: Get rid of caching again.  This breaks too much stuff,
> 	and pkg-config doesn't take much time to run.
> 
> 

I have a case when pkg-config takes over 30 seconds to run. Is that normal or does that indicate problem with my .pc files?

Hardware is: 3.3 GHz CPU, SSD and 16 GB of RAM.
Comment 5 Slawomir Czarko 2012-01-27 02:04:45 UTC
I ran callgrind on pkg-config for the case which takes over 30 seconds and I got this:

99.33% of total time was spent in g_slist_last (almost 400K calls).

g_slist_last is called mostly by g_slist_concat (almost 1.2M calls, 99.36% of total time).

Would it make sense to use some other data structure which allows for faster list concatenation?
Comment 6 Dan Nicholson 2012-08-22 18:11:09 UTC
(In reply to comment #3)
> I can reproduce with version 0.20:
> 
> ----- configure.ac -----
> AC_INIT
> PKG_CHECK_MODULES([PKGCONF], [pkg-config >= 0.1])
> PKG_CHECK_MODULES([PKGCONF], [pkg-config < 0.1])
> ------------------------
> sbrabec> aclocal ; autoconf ; ./configure
> checking for pkg-config... /usr/bin/pkg-config
> checking pkg-config is at least version 0.9.0... yes
> checking for PKGCONF... yes
> checking for PKGCONF... yes
> sbrabec> pkg-config --version
> 0.20
> 
> It is because you set FOO_CFLAGS resp. FOO_LIBS, and then you skip the check, if
> FOO_CFLAGS resp. FOO_LIBS is not empty.

This is old now, but I'm inclinced to close this NOTABUG and just document not to do this unless you understand the limitations. The reason is because we definitely want to allow the user to override FOO_CFLAGS/LIBS from the environment and we cannot differentiate this case from repeated PKG_CHECK_MODULES(FOO, ...). Once the PKG_CHECK_MODULES succeeds, it needs to set FOO_CFLAGS/LIBS. The next invocation sees this and wouldn't have any way to know that it wasn't the user that supplied them.

This is actually no different from how AC_CHECK_PROG and friends work except they have an explicit cache variable to use. If you call AC_CHECK_PROG repeatedly with the same variable, you'll get the first answer back everytime. So, I don't think PKG_CHECK_MODULES is doing anything unusual here. What we could do is warn the developer about this situation:

diff --git a/pkg.m4 b/pkg.m4
index f26f84c..87687cc 100644
--- a/pkg.m4
+++ b/pkg.m4
@@ -105,6 +105,13 @@ fi[]dnl
 # --------------------------------------------------------------
 AC_DEFUN([PKG_CHECK_MODULES],
 [AC_REQUIRE([PKG_PROG_PKG_CONFIG])dnl
+
+dnl Warn if we've already seen this tag
+m4_ifdef([_PKG_SEEN_TAG_$1],
+  [m4_warn([syntax],
+    [$0 tag $1 is already registered and should probably not be reused.])],
+  [m4_define([_PKG_SEEN_TAG_$1])])
+
 AC_ARG_VAR([$1][_CFLAGS], [C compiler flags for $1, overriding pkg-config])dnl
 AC_ARG_VAR([$1][_LIBS], [linker flags for $1, overriding pkg-config])dnl

However, this could be obnoxious since there are legitimate ways to use the same tag repeatedly if you know that you only want the first result or each use is only used conditionally.
Comment 7 Dan Nicholson 2012-08-22 18:19:46 UTC
(In reply to comment #5)
> I ran callgrind on pkg-config for the case which takes over 30 seconds and I
> got this:
> 
> 99.33% of total time was spent in g_slist_last (almost 400K calls).
> 
> g_slist_last is called mostly by g_slist_concat (almost 1.2M calls, 99.36% of
> total time).

This is a different bug that has nothing to do with caching in PKG_CHECK_MODULES. Please open a separate bug with a specific test case if you're still hitting this problem. This would seem to indicate some extreme recursion that is definitely not the normal case.

> Would it make sense to use some other data structure which allows for faster
> list concatenation?

It might not be smart for us to be concatenating the lists, but I'd like to see the specific case first.
Comment 8 Dan Nicholson 2012-08-22 18:41:40 UTC
I added some documentation in pkg-config(1) in 1baefdf discouraging the use repeated variable prefixes. I think that's the best we can do without breaking legitimate uses.
Comment 9 Slawomir Czarko 2012-09-10 08:38:27 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > I ran callgrind on pkg-config for the case which takes over 30 seconds and I
> > got this:
> > 
> > 99.33% of total time was spent in g_slist_last (almost 400K calls).
> > 
> > g_slist_last is called mostly by g_slist_concat (almost 1.2M calls, 99.36% of
> > total time).
> 
> This is a different bug that has nothing to do with caching in
> PKG_CHECK_MODULES. Please open a separate bug with a specific test case if
> you're still hitting this problem. This would seem to indicate some extreme
> recursion that is definitely not the normal case.
> 
> > Would it make sense to use some other data structure which allows for faster
> > list concatenation?
> 
> It might not be smart for us to be concatenating the lists, but I'd like to see
> the specific case first.

New bug created - bug 54716