Skip to content

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

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

Closed
vapier wants to merge 2 commits into from
Closed

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

Changes from 1 commit
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
setupext: do not mess with PKG_CONFIG_PATH 
The current code always sets PKG_CONFIG_PATH to build paths in / which
breaks cross-compiling -- things like /usr/lib are for the build system
(e.g. x86) and not for the target (e.g. arm).  Since we're adding paths
that are already the default for pkg-config, there's no point in trying
to be smart here.  Just punt the code.

This basically reverts commit 101beb9.
  • Loading branch information
@vapier
vapier committed May 29, 2018
commit 5836c98f06c4731f864e4e644e633ed23487e277
15 changes: 0 additions & 15 deletions setupext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,27 +270,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):

@anntzer anntzer May 28, 2018

Copy link
Copy Markdown
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).

@vapier vapier May 28, 2018

Copy link
Copy Markdown
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 ?

@anntzer anntzer May 29, 2018

Copy link
Copy Markdown
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.

@jklymak jklymak May 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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!

@anntzer anntzer May 29, 2018

Copy link
Copy Markdown
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.)

@vapier vapier May 29, 2018

Copy link
Copy Markdown
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

@tacaswell tacaswell May 29, 2018

Copy link
Copy Markdown
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