Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PkgConfigDependency: Parse library paths in a separate step #4061

Merged
merged 1 commit into from
Aug 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 45 additions & 20 deletions mesonbuild/dependencies/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -576,37 +576,67 @@ def _set_cargs(self):
self.compile_args = self._convert_mingw_paths(shlex.split(out))

def _search_libs(self, out, out_raw):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

Copy link
Member Author

@nirbheek nirbheek Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

link_args = []
raw_link_args = []
'''
@out: PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 pkg-config --libs
@out_raw: pkg-config --libs

We always look for the file ourselves instead of depending on the
compiler to find it with -lfoo or foo.lib (if possible) because:
1. We want to be able to select static or shared
2. We need the full path of the library to calculate RPATH values
3. De-dup of libraries is easier when we have absolute paths

Libraries that are provided by the toolchain or are not found by
find_library() will be added with -L -l pairs.
'''
# Library paths should be safe to de-dup
libpaths = OrderedSet()
raw_libpaths = OrderedSet()
#
# First, figure out what library paths to use. Originally, we were
# doing this as part of the loop, but due to differences in the order
# of -L values between pkg-config and pkgconf, we need to do that as
# a separate step. See:
# https://github.com/mesonbuild/meson/issues/3951
# https://github.com/mesonbuild/meson/issues/4023
#
# Separate system and prefix paths, and ensure that prefix paths are
# always searched first.
prefix_libpaths = OrderedSet()
# We also store this raw_link_args on the object later
raw_link_args = self._convert_mingw_paths(shlex.split(out_raw))
for arg in raw_link_args:
if arg.startswith('-L') and not arg.startswith(('-L-l', '-L-L')):
prefix_libpaths.add(arg[2:])
system_libpaths = OrderedSet()
full_args = self._convert_mingw_paths(shlex.split(out))
for arg in full_args:
if arg.startswith(('-L-l', '-L-L')):
# These are D language arguments, not library paths
continue
if arg.startswith('-L') and arg[2:] not in prefix_libpaths:
system_libpaths.add(arg[2:])
# Use this re-ordered path list for library resolution
libpaths = list(prefix_libpaths) + list(system_libpaths)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Track -lfoo libraries to avoid duplicate work
libs_found = OrderedSet()
# Track not-found libraries to know whether to add library paths
libs_notfound = []
libtype = 'static' if self.static else 'default'
# We always look for the file ourselves instead of depending on the
# compiler to find it with -lfoo or foo.lib (if possible) because:
# 1. We want to be able to select static or shared
# 2. We need the full path of the library to calculate RPATH values
#
# Libraries that are provided by the toolchain or are not found by
# find_library() will be added with -L -l pairs.
for lib in self._convert_mingw_paths(shlex.split(out)):
# Generate link arguments for this library
link_args = []
for lib in full_args:
if lib.startswith(('-L-l', '-L-L')):
# These are D language arguments, add them as-is
pass
elif lib.startswith('-L'):
libpaths.add(lib[2:])
# We already handled library paths above
continue
elif lib.startswith('-l'):
# Don't resolve the same -lfoo argument again
if lib in libs_found:
continue
if self.clib_compiler:
args = self.clib_compiler.find_library(lib[2:], self.env,
list(reversed(libpaths)), libtype)
libpaths, libtype)
# If the project only uses a non-clib language such as D, Rust,
# C#, Python, etc, all we can do is limp along by adding the
# arguments as-is and then adding the libpaths at the end.
Expand Down Expand Up @@ -650,16 +680,11 @@ def _search_libs(self, out, out_raw):
if lib in link_args:
continue
link_args.append(lib)
# Also store the raw link arguments, and store raw_libpaths
for lib in self._convert_mingw_paths(shlex.split(out_raw)):
if lib.startswith('-L') and not lib.startswith(('-L-l', '-L-L')):
raw_libpaths.add(lib[2:])
raw_link_args.append(lib)
# Add all -Lbar args if we have -lfoo args in link_args
if libs_notfound:
# Order of -L flags doesn't matter with ld, but it might with other
# linkers such as MSVC, so prepend them.
link_args = ['-L' + lp for lp in raw_libpaths] + link_args
link_args = ['-L' + lp for lp in prefix_libpaths] + link_args
return link_args, raw_link_args

def _set_libs(self):
Expand Down
2 changes: 1 addition & 1 deletion run_unittests.py
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ def fake_call_pkgbin(self, args, env=None):
if '--libs' not in args:
return 0, ''
if args[0] == 'foo':
return 0, '-L{} -lfoo -L{} -lbar'.format(p1.as_posix(), p2.as_posix())
return 0, '-L{} -lfoo -L{} -lbar'.format(p2.as_posix(), p1.as_posix())
if args[0] == 'bar':
return 0, '-L{} -lbar'.format(p2.as_posix())
if args[0] == 'internal':
Expand Down