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

Fix python-config: no longer links with libpython. #87

Closed
wants to merge 1 commit into from

Conversation

rdb
Copy link
Contributor

@rdb rdb commented Dec 21, 2016

This monkey patches the python-config script so that build scripts that use it don't try to link to libpythonX.X.so, which is absent from the manylinux distribution. See #69 for more information.

This also adds python-config -> python3-config symlink to Python 3 installs, analogous to the existing python -> python3 symlink.

Closes: #85

Also adds python-config symlink to Python 3 installs.

Closes: pypa#85
@njsmith
Copy link
Member

njsmith commented Dec 21, 2016 via email

@rdb
Copy link
Contributor Author

rdb commented Dec 21, 2016

I'm reluctant to file a bug on the Python tracker, because I'm not quite convinced that this is a good change to make to Python in general. It would break too many existing and valid use cases.

  • I see the use of this approach for building universally deployable wheels, but leaving symbols undefined when linking can be considered sloppy. Projects may also link with --no-undefined for this reason. In many situations there is no harm in linking to libpython.
  • Platforms may not necessarily enable the use of undefined symbols or use a flat namespace for symbol lookup by default (eg. Mac).
  • A valid use case for linking to libpython is to embed the interpreter. These use cases still need a straightforward way to link to the Python library. This is also relevant when freezing Python applications.
    This argument could also apply to manylinux (since people may want to ship programs that embed the interpreter), but manylinux doesn't even have libpython, rendering that use case moot.

@njsmith
Copy link
Member

njsmith commented Dec 21, 2016

The thing about pushing it upstream is that while it's debatable whether Python extensions should link against -lpython, right now upstream is inconsistent about it: if you build CPython with --disable-shared (like we do) then distutils will not use -lpython when building extensions, but apparently python-config does. That's a bug no matter how you look at it. We didn't invent this thing with not linking to libpython.so, it comes directly from upstream :-). I actually agree that it would make more sense for all extensions to link against libpython.so, but it turns out that a common, standard, upstream-supported configuration on Linux is to ship python without any libpython.so included, so...

(And these code paths are all totally platform specific BTW. On Windows you're forced to link against python.dll because there's no such thing as pulling a symbol out of the environment. And on Mac distutils jumps through hoops to disable the default two-level namespace and --no-undefined stuff when linking.)

@dbogdanov
Copy link

What's the status of this fix? Will there be a merge anytime soon? I've stumbled into this issue in my project.

@njsmith
Copy link
Member

njsmith commented May 9, 2018

I still think we need to at least file an issue upstream before applying patches to our copy of Python's source code.

@dbogdanov
Copy link

I've created an issue on the upstream. You can expand it with more details if necessary.

dbogdanov added a commit to MTG/essentia that referenced this pull request May 17, 2018
Manylinux provides docker images for building wheels on CentOS 5
(https://github.com/pypa/manylinux)

Add script for building Essentia wheels using these images
- Install newer yasm and cmake to build dependencies (CentOS 5 is
  in stone age)
- Patch provided python-config scripts to avoid explicit linking to
  libpython (pypa/manylinux#87)
- Build dependencies and wheels
@trishankatdatadog
Copy link
Member

Closing because too old. Please reopen if still interested. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants