pkgconfig: Fix various corner cases

See unit tests for the exact scenarios this PR fixes.

6 files changed, 135 insertions, 30 deletions

diff --git a/mesonbuild/build.py b/mesonbuild/build.py
index a06979c..5e6db73 100644
--- a/mesonbuild/build.py
+++ b/mesonbuild/build.py
@@ -1011,23 +1011,16 @@ This will become a hard error in a future Meson release.''')
     def get_extra_args(self, language):
         return self.extra_args.get(language, [])
 
-    def get_dependencies(self, exclude=None, for_pkgconfig=False):
+    def get_dependencies(self, exclude=None):
         transitive_deps = []
         if exclude is None:
             exclude = []
         for t in itertools.chain(self.link_targets, self.link_whole_targets):
             if t in transitive_deps or t in exclude:
                 continue
-            # When generating `Libs:` and `Libs.private:` lists in pkg-config
-            # files we don't want to include static libraries that we link_whole
-            # or are uninstalled (they're implicitly promoted to link_whole).
-            # But we still need to include their transitive dependencies,
-            # a static library we link_whole would itself link to a shared
-            # library or an installed static library.
-            if not for_pkgconfig or (not t.is_internal() and t not in self.link_whole_targets):
-                transitive_deps.append(t)
+            transitive_deps.append(t)
             if isinstance(t, StaticLibrary):
-                transitive_deps += t.get_dependencies(transitive_deps + exclude, for_pkgconfig)
+                transitive_deps += t.get_dependencies(transitive_deps + exclude)
         return transitive_deps
 
     def get_source_subdir(self):
diff --git a/mesonbuild/modules/pkgconfig.py b/mesonbuild/modules/pkgconfig.py
index b7a12ff..1cb7698 100644
--- a/mesonbuild/modules/pkgconfig.py
+++ b/mesonbuild/modules/pkgconfig.py
@@ -36,6 +36,7 @@ class DependenciesHelper:
         self.priv_reqs = []
         self.cflags = []
         self.version_reqs = {}
+        self.link_whole_targets = []
 
     def add_pub_libs(self, libs):
         libs, reqs, cflags = self._process_libs(libs, True)
@@ -130,10 +131,7 @@ class DependenciesHelper:
                 if obj.found():
                     processed_libs += obj.get_link_args()
                     processed_cflags += obj.get_compile_args()
-                    if public:
-                        self.add_pub_libs(obj.libraries)
-                    else:
-                        self.add_priv_libs(obj.libraries)
+                    self._add_lib_dependencies(obj.libraries, obj.whole_libraries, obj.ext_deps, public)
             elif isinstance(obj, dependencies.Dependency):
                 if obj.found():
                     processed_libs += obj.get_link_args()
@@ -148,12 +146,13 @@ class DependenciesHelper:
                 processed_libs.append(obj)
             elif isinstance(obj, (build.SharedLibrary, build.StaticLibrary)):
                 processed_libs.append(obj)
-                if isinstance(obj, build.StaticLibrary) and public:
-                    self.add_pub_libs(obj.get_dependencies(for_pkgconfig=True))
-                    self.add_pub_libs(obj.get_external_deps())
-                else:
-                    self.add_priv_libs(obj.get_dependencies(for_pkgconfig=True))
-                    self.add_priv_libs(obj.get_external_deps())
+                # If there is a static library in `Libs:` all its deps must be
+                # public too, otherwise the generated pc file will never be
+                # usable without --static.
+                self._add_lib_dependencies(obj.link_targets,
+                                           obj.link_whole_targets,
+                                           obj.external_deps,
+                                           isinstance(obj, build.StaticLibrary) and public)
             elif isinstance(obj, str):
                 processed_libs.append(obj)
             else:
@@ -161,6 +160,31 @@ class DependenciesHelper:
 
         return processed_libs, processed_reqs, processed_cflags
 
+    def _add_lib_dependencies(self, link_targets, link_whole_targets, external_deps, public):
+        add_libs = self.add_pub_libs if public else self.add_priv_libs
+        # Recursively add all linked libraries
+        for t in link_targets:
+            # Internal libraries (uninstalled static library) will be promoted
+            # to link_whole, treat them as such here.
+            if t.is_internal():
+                self._add_link_whole(t, public)
+            else:
+                add_libs([t])
+        for t in link_whole_targets:
+            self._add_link_whole(t, public)
+        # And finally its external dependencies
+        add_libs(external_deps)
+
+    def _add_link_whole(self, t, public):
+        # Don't include static libraries that we link_whole. But we still need to
+        # include their dependencies: a static library we link_whole
+        # could itself link to a shared library or an installed static library.
+        # Keep track of link_whole_targets so we can remove them from our
+        # lists in case a library is link_with and link_whole at the same time.
+        # See remove_dups() below.
+        self.link_whole_targets.append(t)
+        self._add_lib_dependencies(t.link_targets, t.link_whole_targets, t.external_deps, public)
+
     def add_version_reqs(self, name, version_reqs):
         if version_reqs:
             if name not in self.version_reqs:
@@ -196,6 +220,32 @@ class DependenciesHelper:
         return ', '.join(result)
 
     def remove_dups(self):
+        # Set of ids that have already been handled and should not be added any more
+        exclude = set()
+
+        # We can't just check if 'x' is excluded because we could have copies of
+        # the same SharedLibrary object for example.
+        def _ids(x):
+            if hasattr(x, 'generated_pc'):
+                yield x.generated_pc
+            if isinstance(x, build.Target):
+                yield x.get_id()
+            yield x
+
+        # Exclude 'x' in all its forms and return if it was already excluded
+        def _add_exclude(x):
+            was_excluded = False
+            for i in _ids(x):
+                if i in exclude:
+                    was_excluded = True
+                else:
+                    exclude.add(i)
+            return was_excluded
+
+        # link_whole targets are already part of other targets, exclude them all.
+        for t in self.link_whole_targets:
+            _add_exclude(t)
+
         def _fn(xs, libs=False):
             # Remove duplicates whilst preserving original order
             result = []
@@ -206,19 +256,21 @@ class DependenciesHelper:
                 cannot_dedup = libs and isinstance(x, str) and \
                     not x.startswith(('-l', '-L')) and \
                     x not in known_flags
-                if x not in result or cannot_dedup:
-                    result.append(x)
+                if not cannot_dedup and _add_exclude(x):
+                    continue
+                result.append(x)
             return result
-        self.pub_libs = _fn(self.pub_libs, True)
+
+        # Handle lists in priority order: public items can be excluded from
+        # private and Requires can excluded from Libs.
         self.pub_reqs = _fn(self.pub_reqs)
-        self.priv_libs = _fn(self.priv_libs, True)
+        self.pub_libs = _fn(self.pub_libs, True)
         self.priv_reqs = _fn(self.priv_reqs)
+        self.priv_libs = _fn(self.priv_libs, True)
+        # Reset exclude list just in case some values can be both cflags and libs.
+        exclude = set()
         self.cflags = _fn(self.cflags)
 
-        # Remove from private libs/reqs if they are in public already
-        self.priv_libs = [i for i in self.priv_libs if i not in self.pub_libs]
-        self.priv_reqs = [i for i in self.priv_reqs if i not in self.pub_reqs]
-
 class PkgConfigModule(ExtensionModule):
 
     def _get_lname(self, l, msg, pcfile):
@@ -267,7 +319,6 @@ class PkgConfigModule(ExtensionModule):
     def generate_pkgconfig_file(self, state, deps, subdirs, name, description,
                                 url, version, pcfile, conflicts, variables,
                                 uninstalled=False, dataonly=False):
-        deps.remove_dups()
         coredata = state.environment.get_coredata()
         if uninstalled:
             outdir = os.path.join(state.environment.build_dir, 'meson-uninstalled')
@@ -460,6 +511,8 @@ class PkgConfigModule(ExtensionModule):
             if compiler:
                 deps.add_cflags(compiler.get_feature_args({'versions': dversions}, None))
 
+        deps.remove_dups()
+
         def parse_variable_list(stringlist):
             reserved = ['prefix', 'libdir', 'includedir']
             variables = []
diff --git a/run_unittests.py b/run_unittests.py
index 73131c7..2b0e4e1 100755
--- a/run_unittests.py
+++ b/run_unittests.py
@@ -5836,6 +5836,19 @@ class LinuxlikeTests(BasePlatformTests):
         out = self._run(cmd + ['--libs'], override_envvars=env).strip().split()
         self.assertEqual(out, ['-llibmain2', '-llibinternal'])
 
+        # See common/47 pkgconfig-gen/meson.build for description of the case this test
+        with open(os.path.join(privatedir1, 'simple2.pc')) as f:
+            content = f.read()
+            self.assertIn('Libs: -L${libdir} -lsimple2 -lz -lsimple1', content)
+
+        with open(os.path.join(privatedir1, 'simple3.pc')) as f:
+            content = f.read()
+            self.assertEqual(1, content.count('-lsimple3'))
+
+        with open(os.path.join(privatedir1, 'simple5.pc')) as f:
+            content = f.read()
+            self.assertNotIn('-lstat2', content)
+
     def test_pkgconfig_uninstalled(self):
         testdir = os.path.join(self.common_test_dir, '47 pkgconfig-gen')
         self.init(testdir)
diff --git a/test cases/common/47 pkgconfig-gen/meson.build b/test cases/common/47 pkgconfig-gen/meson.build
index eb2afe4..8c16cd5 100644
--- a/test cases/common/47 pkgconfig-gen/meson.build
+++ b/test cases/common/47 pkgconfig-gen/meson.build
@@ -1,5 +1,12 @@
 project('pkgconfig-gen', 'c')
 
+# Some CI runners does not have zlib, just skip them as we need some common
+# external dependency.
+cc = meson.get_compiler('c')
+if not cc.find_library('z', required: false).found()
+  error('MESON_SKIP_TEST: zlib missing')
+endif
+
 # First check we have pkg-config >= 0.29
 pkgconfig = find_program('pkg-config', required: false)
 if not pkgconfig.found()
@@ -59,3 +66,32 @@ pkgg.generate(
   version : libver,
   dataonly: true
 )
+
+# Regression test for 2 cases:
+# - link_whole from InternalDependency used to be ignored, but we should still
+#   recurse to add libraries they link to. In this case it must add `-lsimple1`
+#   in generated pc file.
+# - dependencies from InternalDependency used to be ignored. In this it must add
+#   `-lz` in generated pc file.
+simple1 = shared_library('simple1', 'simple.c')
+stat1 = static_library('stat1', 'simple.c', link_with: simple1)
+dep = declare_dependency(link_whole: stat1, dependencies: cc.find_library('z'))
+simple2 = library('simple2', 'simple.c')
+pkgg.generate(simple2, libraries: dep)
+
+# Regression test: as_system() does a deepcopy() of the InternalDependency object
+# which caused `-lsimple3` to be duplicated because generator used to compare
+# Target instances instead of their id.
+simple3 = shared_library('simple3', 'simple.c')
+dep1 = declare_dependency(link_with: simple3)
+dep2 = dep1.as_system()
+pkgg.generate(libraries: [dep1, dep2],
+  name: 'simple3',
+  description: 'desc')
+
+# Regression test: stat2 is both link_with and link_whole, it should not appear
+# in generated pc file.
+stat2 = static_library('stat2', 'simple.c', install: true)
+simple4 = library('simple4', 'simple.c', link_with: stat2)
+simple5 = library('simple5', 'simple5.c', link_with: simple4, link_whole: stat2)
+pkgg.generate(simple5)
diff --git a/test cases/common/47 pkgconfig-gen/simple5.c b/test cases/common/47 pkgconfig-gen/simple5.c
new file mode 100644
index 0000000..9f924bd
--- /dev/null
+++ b/test cases/common/47 pkgconfig-gen/simple5.c
@@ -0,0 +1,6 @@
+int simple5(void);
+
+int simple5(void)
+{
+    return 0;
+}
diff --git a/test cases/common/47 pkgconfig-gen/test.json b/test cases/common/47 pkgconfig-gen/test.json
index 1c6a452..702e7fe 100644
--- a/test cases/common/47 pkgconfig-gen/test.json
+++ b/test cases/common/47 pkgconfig-gen/test.json
@@ -1,9 +1,13 @@
 {
   "installed": [
     {"type": "file", "file": "usr/include/simple.h"},
+    {"type": "file", "file": "usr/lib/libstat2.a"},
     {"type": "file", "file": "usr/lib/pkgconfig/simple.pc"},
     {"type": "file", "file": "usr/lib/pkgconfig/libfoo.pc"},
     {"type": "file", "file": "usr/lib/pkgconfig/libhello.pc"},
-    {"type": "file", "file": "usr/lib/pkgconfig/libhello_nolib.pc"}
+    {"type": "file", "file": "usr/lib/pkgconfig/libhello_nolib.pc"},
+    {"type": "file", "file": "usr/lib/pkgconfig/simple2.pc"},
+    {"type": "file", "file": "usr/lib/pkgconfig/simple3.pc"},
+    {"type": "file", "file": "usr/lib/pkgconfig/simple5.pc"}
   ]
 }