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.

Sign up for GitHub

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

Jump to bottom

fix pkg-config handling to make cross-compiling work #11218

Closed
wants to merge 2 commits into from
Closed

fix pkg-config handling to make cross-compiling work #11218

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5836c98
setupext: do not mess with PKG_CONFIG_PATH
vapier May 15, 2015
cd06e1b
setupext: do not hardcode system -I/-L paths when pkg-config is avail…
vapier May 15, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 10 additions & 24 deletions setupext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ def make_extension(name, files, *args, **kwargs):
"""
Make a new extension. Automatically sets include_dirs and
library_dirs to the base directories appropriate for this
platform.
platform when pkg-config is not available.

`name` is the name of the extension.

Expand All @@ -231,14 +231,15 @@ def make_extension(name, files, *args, **kwargs):
`distutils.core.Extension` constructor.
"""
ext = DelayedExtension(name, files, *args, **kwargs)
for dir in get_base_dirs():
include_dir = os.path.join(dir, 'include')
if os.path.exists(include_dir):
ext.include_dirs.append(include_dir)
for lib in ('lib', 'lib64'):
lib_dir = os.path.join(dir, lib)
if os.path.exists(lib_dir):
ext.library_dirs.append(lib_dir)
if not pkg_config.has_pkgconfig:
for dir in get_base_dirs():
include_dir = os.path.join(dir, 'include')
if os.path.exists(include_dir):
ext.include_dirs.append(include_dir)
for lib in ('lib', 'lib64'):
lib_dir = os.path.join(dir, lib)
if os.path.exists(lib_dir):
ext.library_dirs.append(lib_dir)
ext.include_dirs.append('.')

return ext
Expand Down Expand Up @@ -270,27 +271,12 @@ def __init__(self):
self.has_pkgconfig = False
else:
self.pkg_config = os.environ.get('PKG_CONFIG', 'pkg-config')
self.set_pkgconfig_path()
self.has_pkgconfig = shutil.which(self.pkg_config) is not None
if not self.has_pkgconfig:
print("IMPORTANT WARNING:\n"
" pkg-config is not installed.\n"
" matplotlib may not be able to find some of its dependencies")

def set_pkgconfig_path(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed?
Note that (unchecked...) this may be needed to make things work in a conda env which does install the libraries in the environment but where pkg-config is "outside" the env (because sysconfig.get_config_var("LIBDIR") will correctly point inside the env in that case).

Copy link
Author

Choose a reason for hiding this comment

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

i explained in the commit message. did you see that ?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but in that case I would like someone (perhaps me, just not now) to check whether building Matplotlib as follows still works: in a conda env, freetype and libpng installed from conda, pkg-config not installed from conda but available outside of conda (e.g. from the distro). Also, what gets linked (the conda libs or the distro libs) in the shared objects in that case?
I'm not saying this won't work or that this absolutely needs to work, but if it used to and doesn't anymore, then one should make sure that the docs clearly state that a conda pkg-config is needed in that setup.

Copy link
Member

@jklymak jklymak May 29, 2018 •
edited

Choose a reason for hiding this comment

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

Personally, I usually don't read the commit messages. We usually include important info in the PR body above and/or the code itself. (EDIT:) We usually try and keep the commit messages quite minimal. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

(I don't think that "trying to keep commit messages minimal" is a good advice (quite the contrary...). But yes, it's good to also good to duplicate all the info in the thread.)

Copy link
Author

Choose a reason for hiding this comment

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

sorry, but i'm not familiar with conda (assuming you're referring to https://conda.io/), so i really have no idea how to test it

the goal here isn't to make pkg-config required, but to make it so that if the package supports pkg-config, we only use that info

Copy link
Member

Choose a reason for hiding this comment

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

Please put details in the commit messages! Those are what help you in 5 years while checking git blame 😉

pkgconfig_path = sysconfig.get_config_var('LIBDIR')
if pkgconfig_path is None:
return

pkgconfig_path = os.path.join(pkgconfig_path, 'pkgconfig')
if not os.path.isdir(pkgconfig_path):
return

try:
os.environ['PKG_CONFIG_PATH'] += ':' + pkgconfig_path
except KeyError:
os.environ['PKG_CONFIG_PATH'] = pkgconfig_path

def setup_extension(self, ext, package, default_include_dirs=[],
default_library_dirs=[], default_libraries=[],
alt_exec=None):
Expand Down