Fix pkg-config dependencies leaking out (debbug 892956) #3251

jpakkane · 2018-03-16T22:42:16Z

Do not add private dependencies of shared libraries. Pkg-config leaks them out with --cflags and thus requires that all dependency dev libraries are installed when using this. That should not be required. If the thing to be installed is a static library, the dependencies are added because there is no other way.

This will fail in CI because the old tests verifying pkg-config output obviously no longer work. @sarum9in and @xclaesse, you seemed to be the last persons to touch these. Could you check out what should be the correct way to fix those? Thanks.

This builds a project with pkg-config file, installs it and then builds a second project that uses that dependency and runs the result.

ignatenkobrain · 2018-03-16T22:43:11Z

run_unittests.py

+            os.environ['PKG_CONFIG_PATH'] = pkg_dir
+            # Private internal libraries must not leak out.
+            pkg_out = subprocess.check_output(['pkg-config', '--static', '--libs', 'libpkgdep'])
+            #self.assertFalse(b'libpkgdep-int' in pkg_out, 'Internal library leaked out.')


[Flake8]

[E265] block comment should start with '# '

xclaesse · 2018-03-16T23:25:52Z

I don't understand when does it make sense for pkg-config to list cflags of private deps.

xclaesse · 2018-03-16T23:26:38Z

I mean, this seems a bug in pkgconfig to me... Unless there is something I don't understand and that's likely :)

textshell · 2018-03-16T23:29:17Z

mesonbuild/modules/pkgconfig.py

+                # This way (hopefully) "pkgconfig --libs --static foobar" works
+                # and "pkgconfig --cflags/--libs foobar" does not have any trace
+                # of dependencies that the build file creator has not explicitly
+                # added to the dependency list.
                processed_libs.append(obj)
                if public:
                    if not hasattr(obj, 'generated_pc'):


I think this if should also be done for build.SharedLibrary. (i.e. obj.generated_pc = self.name if public)

Doing that is exactly what caused this bug.

Why?
I think what causes this bug is one (or both) of the self.add_priv_libs( lines.

obj.generated_pc is only used if something else depends on this library, possibly even explicitly.

textshell · 2018-03-16T23:34:01Z

All we currently know why pkg-config lists cflags of private deps is this commit:
https://cgit.freedesktop.org/pkg-config/commit/pkg.c?id=0936824bf02c604457147af1858ae6f5b504155f

I just noticed that it does reference https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=340904

jpakkane · 2018-03-16T23:36:25Z

It is either a bug or a feature of pkg-config, yes. If you specify private depends on a pc file, then those arguments are used for --libs if you specify --static and for --cflags always. This means that pkg-config requires that all transitive dependencies must be installed when doing --cflags because those are needed to resolve the flags. There does not seem to be a Cflags.private or something similar to make this work properly.

If we install a shared library foo, we know that it is shared so it could not even be compiled with --static. Thus we don't put any depends there because doing so would break things.

xclaesse · 2018-03-16T23:47:00Z

That's going to hit back when we add both_library() then. I need to read that Debian bug, I still think it makes no sense to have private cflags.

jpakkane · 2018-03-17T00:00:17Z

It seems to be currently impossible to do a pkg-config file that:

does not add cflags of private dependencies when linking dynamically
does add libs of private dependencies when linking statically

Pkg-config's static linking is a bit broken anyway. It does not actually make the flags point to the .a files. (as an example pkg-config --libs --static zlib is identical to the version without static, i.e. -lz). It just adds the -lfoo flags of all deps to the list.

textshell · 2018-03-17T00:01:38Z

this issue with #3093 / #3074 argues, that we should indeed set .private only with both_library().

Edit: But that should be a separate change outside of this hotfix.

textshell · 2018-03-17T00:12:59Z

debian 340904 is indeed an interesting read. From my reading they needed a cflags only variant of Requires and used Requires.private because it looked close enough.

nirbheek · 2018-03-17T00:17:11Z

So from what I understand, the summary is:

Many projects have (had?) headers that #include headers from private-only dependencies (ex. atk)
These dependencies are not needed at link time, only at compile time (hence only added in --cflags)
No one has ever created an accurate list of Requires.private: because people have always managed that list manually

As a result, every pkg-config file now requires the entire dependency tree of development packages to be installed because our Requires.private: is completely accurate.

This is nuts.

The correct solution, as @textshell said, is to have something like Requires.cflags: (or similar) and everyone made the wrong decision 12 years ago.

nirbheek · 2018-03-17T00:22:50Z

I think the solution in this PR is incorrect, pkgconfig doesn't care what your installed libraries look like, and making the outputted pkgconfig file conditional on that doesn't sound right.

One solution here is to never add private requires automatically, and require people to provide those manually. I am sure there are other solutions, suggestions welcome.

xclaesse · 2018-03-17T00:26:48Z

I think meson does this right thing here, and finally after 10+ years we have correct PC files. I think we should revert the commit in pkg-config, add requires.cflags, and wait another 10 years for everyone to fix their pc files.

nirbheek · 2018-03-17T00:31:19Z

I think meson does this right thing here, and finally after 10+ years we have correct PC files. I think we should revert the commit in pkg-config, add requires.cflags, and wait another 10 years for everyone to fix their pc files.

That's not really how you make progress though. You can't break the world and tell people to deal with it. That just makes them patch your code away and not do anything.

For the future, we will want to extend the pkgconfig format after talking to the pkg-config and pkgconf maintainers. The way I see it, one way would be by changing the meaning of Requires.private: to mean cflags-only requires (which is what it effectively is right now), and adding two new entries:

Requires.private-libs: which will contain the library private requires (exhaustive list)
Requires.private-cflags: which will contain the cflags-only private requires

There is no such thing as "correct PC files" in this case. The format is fundamentally broken.

textshell · 2018-03-17T00:38:49Z

I think static / dynamic linking of libraries in pkg-config is just not working well and meson has to work with pkg-config limitations as best as possible. And that fundamentally depends on what variants of a library are going to be available.

For libraries that are only going to be installed in a dynamic variant, outputting .private dependencies only breaks users like this bug shows.

For libraries that are only going to be installed in a static variant, outputting .private dependencies forces consumers of the library to be aware that currently only a static variant is available, so i argue that meson should in the future change to outputting the in that case always needed dependencies as non private to make using the static library easy. This will not be worse than using .private in leaking cflags because .private cflags leak anyway (debian 340904).

For libraries that are only going to be installed in both a static and a dynamic variant, meson has no other choice than to output the dependencies needed only for static linking to .private. This will then have the side effect of requiring -dev packages for the parts of the dependency tree that allow static linking. But i think this will not be a regression as long as libraries are not introducing the static option where they did not offer that before.

I agree with nirbheek that breaking the world right now will simply not work. So the approach of this PR at least for the short term is the right solution. Libraries that want to have the additional dependencies still can request them explicitly in their meson.build, right?

I also agree that in the long term pkg-config should be extended. Preferably when there is a good collection of use cases in form of unit tests that can be used to validate proposals so they don't end like what we are dealing with here(i.e. seemed reasonable at that time)

xclaesse · 2018-03-17T01:48:15Z

I agree with @textshell, for SharedLibrary we don't need private deps at all, and for StaticLibrary we could put private deps in public. The problem will appear when we'll add BothLibraries.

I guess we should deprecate Requires.private because it's ambiguous and add Requires.private-libs and Requires.private-cflags. Meson will use Requires.private-libs when generating pc file for BothLibraries and depend on a future version of pkg-config.

AdrianBunk · 2018-03-17T10:29:16Z

It seems to be currently impossible to do a pkg-config file that:
does not add cflags of private dependencies when linking dynamically
does add libs of private dependencies when linking statically

With autoconf or cmake this is usually done with something like
Libs.private: @PRIVATE_LIBS@
and then substituting @PRIVATE_LIBS@ with whatever is appropriate.

I am not convinced that this will always work correctly with several levels of static libraries, but if there are problems they are at least not widespread.

So as a short-term solution putting into Libs.private the pkg-config --libs of what you currently place in Requires.private might be an option (untested).

nirbheek · 2018-03-17T10:56:52Z

So as a short-term solution putting into Libs.private the pkg-config --libs of what you currently place in Requires.private might be an option (untested).

That is one possible solution, but it has a few problems:

Unsure if -L params work in Libs.private:, they should though
Have to ensure that the -L params are of the form -L${libdir} or similar. i.e., they derive from ${prefix}, would need to reverse-engineer the expanded arguments to do this
Version requirements cannot be represented
Library paths are hard-coded to whatever is found at library build time, which means you can't statically link against a different library while using it in another project

nirbheek · 2018-03-17T10:58:03Z

I agree with @textshell, for SharedLibrary we don't need private deps at all, and for StaticLibrary we could put private deps in public. The problem will appear when we'll add BothLibraries.

I think this problem already exists; I know of projects that build both shared and static by reusing built objects. BothLibrary will merely formalize the breakage.

textshell · 2018-03-17T11:45:28Z

Yes, while meson doesn't yet have both_libraries, there are libraries that manually build both variants.
And the pkgconfig module should have a way to support those. Not sure how, maybe it needs to be an explicit way. A kwarg static_and_shared or something could enable generating .pc files that are usable in both modes (just like what including a both_libraries target would generate)

But libraries build with meson that both
a) use the pkgconfig module to generate .pc files and
b) support installing static and shared variants of the library at the same time
do not yet seem many. Looking through Users.md, i could only identify

Dpdk, Data plane development kit, a set of libraries and drivers for fast packet processing

to be affected. Maybe finding a workaround for just dpdk might be ok for now. But of course i might have false negatives. Does anyone know other cases?

I think we should try to have some official guidance in 0.46 how to use the pkgconfig module for manual both_library like usecases. Currently meson would have to guess based on target name and install: true of all shared and static libraries, which seems like something we don't want.

xclaesse · 2018-03-17T13:02:11Z

Projects that does both libraries manually could just switch to BothLibraries to get special treatment in pc generator.

textshell · 2018-03-17T14:15:03Z

I did not dare to hope that both_libraries would be ready for inclusion in the 0.46 time-frame and i think it is important to have some/any supported option for doing shared/static with pkgconfig when we break it here.
Looking at the both_libraries PR is has come along a lot on the consensus front, so it might be possible.
So if both_libraries is ready i'm perfectly happy with suggesting that for now, and ignoring the (may even theoretic) cases where both_libraries is not flexible enough.

xclaesse · 2018-03-17T14:49:36Z

I reported the pkg-config bug there: https://bugs.freedesktop.org/show_bug.cgi?id=105572

What I would suggest here:

For SharedLibrary do not add private deps at all.
For StaticLibrary add private deps as public since they'll always been needed anyway.
Merge BothLibraries and consider that manually doing both libraries is not a supported case for pc generator.
For BothLibraries, pc generator will NOT add Requires.private but instead explicitly add ldflags into Libs.private which is what cmake does according to @AdrianBunk. It will do the right thing most of the time, it's just that you'll have to regenerate your pc file when the pc file of one of your internal deps change.
Later when pkg-config and pkgconf add new field (I suggested Requires.private-libs in upstream bug) use it for BothLibraries and bump or minimum pkg-config version requirement.

textshell · 2018-03-17T15:08:41Z

@jpakkane I had a look at the failing unit tests and created a branch with suggestions for getting the tests to work again https://github.com/mesonbuild/meson/compare/fixpkgconfigdeps...textshell:fixpkgconfigdeps-suggestions?expand=1

The fix for test_pkgconfig_gen_deps just switches the test to use a static_library, for now the fixes the test. That test likely needs to be rethought depending on what further actions we decide.

These suggestion also include setting generated_pc again as suggested in #3251 (comment)

I think with these fixes this PR implements step 1 in @xclaesse 's plan in #3251 (comment)

jpakkane · 2018-03-17T17:53:01Z

Updated with test fixes.

xclaesse · 2018-03-17T20:49:53Z

run_unittests.py

+            test_exe = os.path.join(self.builddir, 'pkguser')
+            self.assertTrue(os.path.isfile(test_exe))
+            subprocess.check_call(test_exe, env=myenv)
+


test_pkgconfig_gen_deps() already pretty much does the same thing. I think it would be better to add test cases there instead of a completely new test case. Also I added self.new_builddir() for this kind of test.

xclaesse · 2018-03-17T20:51:38Z

run_unittests.py

@@ -510,7 +510,8 @@ def tearDown(self):
                windows_proof_rmtree(path)
            except FileNotFoundError:
                pass
-        os.environ = self.orig_env
+        os.environ.clear()
+        os.environ.update(self.orig_env)


I'm curious, why is this needed?

os.environ is a dict like object but not actually a dict. setUp and tearDown previously tried to restore the state of os.environ by assigning os.environ.copy() to os.environ. But that is a normal dict and thus after that os.environ no longer has effect on processes started with subprocess.

I spend some time debugging why the test worked in isolation but not with the whole test suit and decided to fix the root cause, by not assigning but by replacing the contents back into the special object.

jpakkane · 2018-03-18T11:17:52Z

Since this is urgent we'll need to get a point release out with this fix either today or, at the very latest, tomorrow. This may mean that the end result won't work perfectly with static dependencies, but that is not really such a huge problem because the old version did not work either.

textshell · 2018-03-18T12:03:13Z

Afaik this PR doesn't produce any regressions in the static libraries only use case.
The manual both_libraries emulation use case is more work than to be realistic in the one day time-frame and can be worked around with manual private depencencies in the worst case (switching to the static library in the generate call is not future proof).

From my pov the only real blocker is getting rid of the OS X test failures caused by glib2 not being found in some way.
But if the unit tests are not as they are supposed to be we should make sure to fix that for 0.46. But with the differences between static, shared and both, we likely need a lot more tests anyway.

For point 2 (changes for static libs) of @xclaesse 's plan i will rebase #3093 after this PR lands so we can discuss that there.
Point 3 (both_libraries) already has an open PR in #2711.

nirbheek · 2018-03-18T12:19:01Z

I reported the pkg-config bug there: https://bugs.freedesktop.org/show_bug.cgi?id=105572

Note that Fedora moved to pkgconf a while ago, so we need to report it there too. CCing @kaniini

smcv · 2018-03-18T12:45:54Z

I've written up my understanding of the situation on https://bugs.freedesktop.org/show_bug.cgi?id=105572. The tl;dr version is that there are three use-cases, of different levels of "privateness".

I don't understand when does it make sense for pkg-config to list cflags of private deps.

Hopefully my comment on the pkg-config bug makes it clearer. If it doesn't, let's use the pkg-config bug (since pkg-config provides the "specification" that Meson, pkgconf and library authors need to follow) to reach a common understanding of what's happening here and what needs to be done about it.

@AdrianBunk: I've tried to summarize the problems caused by having too many CFLAGS, paraphrasing what you said on IRC. I know we have different opinions on what the severity of those problems is, but hopefully I've described the facts correctly? If there are some that I missed, please describe them on the pkg-config bug. Thanks!

xclaesse · 2018-03-18T20:07:58Z

I added patches to add Requires.internal in pkg-config. In the bright future, I think dependencies automatically pulled by meson's pkgconfig generator should all go into Requires.internal because meson as no way to know if they are exposed in API or not. Up to the user to manually pass a list of deps to promote to requires or requires_private.

nirbheek · 2018-03-18T20:59:30Z

pkgconf now supports Requires.internal, so if we will exclude the full list of static deps from Requires.private, we should add it to Requires.internal.

xclaesse · 2018-03-18T21:07:09Z

Wow that was quick.

kaniini · 2018-03-18T21:21:32Z

It was literally one line of code to implement it as I proposed :)

textshell · 2018-03-18T21:45:49Z

~~pkgconf's Requires.internal sadly is only an alias for Requires.private with all it's problems at the moment.~~

Edit: It is now implemented closer to @xclaesse 's proposal.

nirbheek · 2018-03-18T22:17:20Z

pkgconf's Requires.internal sadly is only an alias for Requires.private

Sure, but it means we have a way of getting a correct set of libraries with --static --libs on distros that use pkgconf, like Fedora. It would be unfortunate if the fallout from that Debian bug meant that static linking was just broken everywhere.

ignatenkobrain · 2018-03-18T22:48:48Z

run_unittests.py

+            myenv = os.environ.copy()
+            myenv['LD_LIBRARY_PATH'] = lib_dir
+            self.assertTrue(os.path.isdir(lib_dir))
+            #self.assertTrue(os.path.isfile(os.path.join(lib_dir, 'libpkgdep.so')))


[Flake8]

[E265] block comment should start with '# '

jon-turney · 2018-03-19T20:04:55Z

Try adding jon-turney@2e90399 to fix the new test on Cygwin.

ignatenkobrain · 2018-03-19T20:23:38Z

run_unittests.py

+                bin_dir = os.path.join(tempdirname, 'bin')
+                myenv['PATH'] = bin_dir + os.pathsep + myenv['PATH']
+            self.assertTrue(os.path.isdir(lib_dir))
+            #self.assertTrue(os.path.isfile(os.path.join(lib_dir, 'libpkgdep.so')))


[Flake8]

[E265] block comment should start with '# '

Add test for pkgconfig generation and usage.

d532650

This builds a project with pkg-config file, installs it and then builds a second project that uses that dependency and runs the result.

ignatenkobrain reviewed Mar 16, 2018

View reviewed changes

Do not leak out private dependencies for shared libraries.

c385f79

jpakkane force-pushed the fixpkgconfigdeps branch from 7b405a1 to c385f79 Compare March 16, 2018 22:45

textshell reviewed Mar 16, 2018

View reviewed changes

jpakkane added this to the 0.45.1 milestone Mar 17, 2018

xclaesse reviewed Mar 17, 2018

View reviewed changes

jpakkane force-pushed the fixpkgconfigdeps branch from 0cea660 to 79a343f Compare March 18, 2018 20:31

jpakkane force-pushed the fixpkgconfigdeps branch from 79a343f to b331ce6 Compare March 18, 2018 22:48

ignatenkobrain reviewed Mar 18, 2018

View reviewed changes

jpakkane force-pushed the fixpkgconfigdeps branch from b331ce6 to 26a1fa6 Compare March 19, 2018 20:19

ignatenkobrain reviewed Mar 19, 2018

View reviewed changes

Fix existing tests.

cf5f1a8

jpakkane force-pushed the fixpkgconfigdeps branch from 26a1fa6 to cf5f1a8 Compare March 19, 2018 21:33

jpakkane merged commit e984e10 into master Mar 19, 2018

jpakkane deleted the fixpkgconfigdeps branch March 19, 2018 21:43

Fix pkg-config dependencies leaking out (debbug 892956) #3251

Fix pkg-config dependencies leaking out (debbug 892956) #3251

Conversation

jpakkane commented Mar 16, 2018

Choose a reason for hiding this comment

xclaesse commented Mar 16, 2018

xclaesse commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

textshell commented Mar 16, 2018

jpakkane commented Mar 16, 2018

xclaesse commented Mar 16, 2018

jpakkane commented Mar 17, 2018

textshell commented Mar 17, 2018 • edited Loading

textshell commented Mar 17, 2018

nirbheek commented Mar 17, 2018

nirbheek commented Mar 17, 2018 • edited Loading

xclaesse commented Mar 17, 2018

nirbheek commented Mar 17, 2018 • edited Loading

textshell commented Mar 17, 2018

xclaesse commented Mar 17, 2018

AdrianBunk commented Mar 17, 2018

nirbheek commented Mar 17, 2018

nirbheek commented Mar 17, 2018

textshell commented Mar 17, 2018 • edited Loading

xclaesse commented Mar 17, 2018

textshell commented Mar 17, 2018

xclaesse commented Mar 17, 2018 • edited Loading

textshell commented Mar 17, 2018

jpakkane commented Mar 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

textshell Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

jpakkane commented Mar 18, 2018

textshell commented Mar 18, 2018

nirbheek commented Mar 18, 2018

smcv commented Mar 18, 2018

xclaesse commented Mar 18, 2018

nirbheek commented Mar 18, 2018

xclaesse commented Mar 18, 2018

kaniini commented Mar 18, 2018

textshell commented Mar 18, 2018 • edited Loading

nirbheek commented Mar 18, 2018

Choose a reason for hiding this comment

jon-turney commented Mar 19, 2018

Choose a reason for hiding this comment

textshell commented Mar 17, 2018 •

edited

Loading

nirbheek commented Mar 17, 2018 •

edited

Loading

nirbheek commented Mar 17, 2018 •

edited

Loading

textshell commented Mar 17, 2018 •

edited

Loading

xclaesse commented Mar 17, 2018 •

edited

Loading

textshell Mar 17, 2018 •

edited

Loading

textshell commented Mar 18, 2018 •

edited

Loading