PkgConfigDependency: Parse library paths in a separate step

[nirbheek]

pkg-config and pkgconf treat additional search paths in PKG_CONFIG_PATH and PKG_CONFIG_LIBDIR differently when PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 is set.

pkg-config always outputs -L flags for the additional paths first, and pkgconf always outputs -L flags for the default paths first.

To account for this inconsistency, we now sort the library paths into two separate sets: system (default) and prefix (additional) paths. We can do this because we always query pkg-config twice: once with PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 set and once without it.

Then, we ensure that the prefix paths are searched before the system paths.

Closes #4023
Closes #3951

[nirbheek]

@jadahl @chergert @ondrejholy can you please confirm that jhbuild is linking correctly with this branch?

[jadahl]

This seems to fix the problem for me.

[textshell]

can we rename out and out_raw to something that gives a clue what they contain?

[nirbheek]

out means "pkg-config output" and out_raw means "pkg-config output without PKG_CONFIG_ALLOW_SYSTEM_LIBS". I documented it in the docstring what each means.

[textshell]

Yes, it's documented. But i don't think the names are very good. Maybe something like out_with_system and out_without_system would be more clear and contrasting.

[nirbheek]

I mean, sure, I can rename them, but I don't see the point because they're just used once (shlex.split()) and then never again ;)

[nirbheek]

Also, we use the term self.raw_link_args on the object itself, so there is precedent for this terminology.

[textshell]

There is, but as instance variable i think it's even worse, because it does have even less context. 'raw' just mean, without something or some processing that is unspecified...

[nirbheek]

So I was making the change when I realized that it'll touch a lot of other code unnecessarily, since out and out_raw is used elsewhere too. Let's not churn the CI unnecessarily, since this PR has to go into the stable branch.

[textshell]

I think this needlessly duplicates all paths in prefix_libpaths? Maybe something with an combined ordered dict?
e.g. combined_paths = a copy of prefix_libpaths before the loop and adding to combined_paths also in the loop that parses out? This would in effect move all pathes that are only in out to the end relative to those that are in both.

[nirbheek]

No, it does not duplicate any paths. prefix_libpaths and system_libpaths are disjoint sets. See the loop right above this line. I'm not sure I understand what you mean.

[textshell]

right. I somehow overlooked the arg[2:] not in prefix_libpaths part. Sorry for the noise.

[textshell]

Are we sure this really only happens with system directories? Or is the deeper problem here something that could also happen with multiple prefixes mentioned in the pkg config path?

I still don't really understand why this problem happens at all, i.e. what differences cause both implementations to output different orders in a breaking way.

[nirbheek]

Are we sure this really only happens with system directories? Or is the deeper problem here something that could also happen with multiple prefixes mentioned in the pkg config path?

Like I said above, this only happens when PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 is set, which force-adds system paths to the --libs output.

I still don't really understand why this problem happens at all, i.e. what differences cause both implementations to output different orders in a breaking way.

See: #4023 (comment), does that clarify things?

[textshell]

Like I said above, this only happens when PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 is set, which force-adds system paths to the --libs output.

PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 does certainly not force add anything. It removes filtering of the system directories when needed. So my mental model says it's likely that whatever hits us here might be a more general problem.

See: #4023 (comment), does that clarify things?

No, i've read that before. Also it does not seem to match up with the manpage of pkg-config. i.e. The default directory will always be searched after searching PKG_CONFIG_PATH.

Maybe a PKG_CONFIG_DEBUG_SPEW=1 would shed some light, but i never used that with an problem. From my POV all i know is that the output is bad and that my mental model of the process does not even begin to explain how this can only happen with the system pathes and not with other pathes as well.

But maybe i'm missing an important clue.

[nirbheek]

I actually don't understand your confusion. I'm simply comparing the output of pkgconf and pkg-config given the same input, and they treat the directories in PKG_CONFIG_PATH differently. We don't need to care how they come up with that output, and the solution in this PR also doesn't care. The only reason why we add PKG_CONFIG_ALLOW_SYSTEM_LIBS is so that we are told explicitly by pkg-config where to search for libraries even when the prefix is the system prefix. This PR ensures that we search those paths last, since that's what basically everyone who installs .pc files into a custom prefix wants.

[textshell]

I don't habe enough data for that conclusion. It could very well be something completely different and the fix could be just papering over that. From what i know there is no way to tell.
I just looked at the source and what i am certain of is that PKG_CONFIG_ALLOW_SYSTEM_LIBS just controls a filter, and does not force add anything. So somehow a .pc file is selected and processed that does contain that path.

Maybe it's because of broken .pc files. Maybe it's because some interaction between different prefixes or maybe it's because of an interaction that only happens with the system prefix.

Does this PR help in the concrete case? Yes.

Is this PR the right fix? Unsure. Is this PR maybe better than doing nothing? Even more unsure.

[nirbheek]

Oh, sorry, I didn't realize you weren't familiar with that env var. From the pkg-config manpage:

PKG_CONFIG_ALLOW_SYSTEM_LIBS Don't strip -L/usr/lib or -L/lib out of libs.

The values of those are different on each distro. For instance, on Debian it's -L/usr/lib/x86_64-linux-gnu. So that flag forces pkg-config to output those even when it thinks they're not necessary because the compiler will always look in that dir if it can't find it in any other -L paths.

These paths are also usually omitted because otherwise they would override other -L flags in "dumb" build systems that simply concatenate linker args such as Autotools and CMake. We, of course, want those flags.

This is not due to broken .pc files, and you can verify that by looking at the attached pkg-config files from the other issue. There is a test that reproduces this too, see the unit tests.

There is no interaction between different prefixes. I verified that.

I am quite sure that this is the best fix we can do for this, and it's overall more correct anyway because the logic now more exactly matches what the linker will do (collect all -L flags once, then do a second pass over the link args to resolve -l args).

[textshell]

So what you are saying is that we want to mimic the system linker which prefers the explicitly provided (-L) paths when linking and only then uses the build in paths.

For this we instruct pkg-config to allow system paths in it's output and then call it again without. Now we use the result of the call without the system paths as primary search path and then fall back to the system paths we got from the other invocation?

In that frame of mind this whole PR could be seen as trying to use pkg-config to get the system paths and apart from that just use the output with filtered out system paths like a simple build system would do.

But this does only work when some of the libraries in in the pkg-config result explicitly included the system paths. Which they might or might not do.

Why are we doing that pkg-config invocation with system directories when other parts of meson already know what the system paths are? That seems a bit odd.

[nirbheek]

But this does only work when some of the libraries in in the pkg-config result explicitly included the system paths. Which they might or might not do.

If they don't, they are broken and unusable. No distro will ship with such pc files, and if you build such a pc file for use in a custom prefix, without the -L flag it will not work at all.

Why are we doing that pkg-config invocation with system directories when other parts of meson already know what the system paths are? That seems a bit odd.

By "other parts of meson" you mean CCompiler.get_library_dirs()? That fetches the search dirs from the compiler, which is a much larger list of dirs. Querying pkg-config is more correct because it knows exactly where the library is. For instance, if you're cross-compiling for MinGW and using a pkg-config file to link to your own libintl, looking in the compiler paths will link to the MinGW libintl, which is not what you want.

It is true that in Autotools and other build systems all those dirs are searched implicitly by the compiler, but in those cases you have no way to control whether you're linking to a shared or static library and it also leads to warnings from the compiler where it says "Ignored 32-bit libs for 64-bit compilation". We don't make that mistake.

Also, we shipped this change months ago and it seems to be working fine overall. Not sure why you're talking about it on this PR.

[textshell]

With pkgconf, i can reproduce the problem with two non system prefixes it seems.

$ cat /tmp/test-prefixes/prefix-a/a.pc 
Name: A
Version: 1.0
Libs: -L/prefix/a -la
Description: A
Requires: b

$ cat /tmp/test-prefixes/prefix-b/b.pc 
Description: B
Version: B
Name: B
Libs: -L/prefix/b -lb

$ PKG_CONFIG_PATH=/tmp/test-prefixes/prefix-b:/tmp/test-prefixes/prefix-a pkgconf --libs a
-L/prefix/a -la -L/prefix/b -lb 

If /prefix/a would contain an older version of libb.so this would just trigger the same way as in #3951
pkgconfig (the fdo one) explicitly sorts all -L by their sources position in the PKG_CONFIG_PATH (which seems like the smart thing to do). But pkgconf does not seem to do that.

The fdo implementation does not have this problem:

$ PKG_CONFIG_PATH=/tmp/test-prefixes/prefix-b:/tmp/test-prefixes/prefix-a pkg-config --libs a
-L/prefix/b -L/prefix/a -la -lb

[nirbheek]

Summary of discussion on IRC is:

1. This is a bug in pkgconf
2. It should be reported upstream
3. It's not a regression and affects all build systems, so not a blocker for the stable release
4. Nothing we can do to fix it in Meson outside of reimplementing pkg-config