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

python: only link extension modules against libpython when distutils does too. Fixes #4117 #4187

Merged
merged 1 commit into from
Dec 10, 2018

Conversation

lazka
Copy link
Contributor

@lazka lazka commented Sep 15, 2018

Windows requires things to be linked, on macOS distutils doesn't link by default.
On Linux etc. things are not so clear, some distros like Debian patch distutils to not link,
some don't. In addition the manylinux wheels spec prohibits linking against libpython
and upstream is thinking about changing the default:
https://bugs.python.org/issue34814

Call into distutils to figure out what distutils does and in case it doesn't link
against libpython replace the passed in Python dependency with a partial one.

@jpakkane
Copy link
Member

@MathieuDuponchelle does this seem ok to you?

@MathieuDuponchelle
Copy link
Contributor

@MathieuDuponchelle does this seem ok to you?

Well, while I can't help but think this seems a bit unsafe, if that's how distutils works, we should follow that. In addition, once we have module level options, we should probably expose this as an option so that distros don't have to patch meson :)

Not sure what the appveyor failure is about?

@lazka
Copy link
Contributor Author

lazka commented Sep 22, 2018

I can change it to call into distutils and mirror its behavior if wanted.

@nirbheek
Copy link
Member

Appveyor failure is in Cygwin and the job took 1+hour, which means it probably just timed out. I've restarted it.

@lazka
Copy link
Contributor Author

lazka commented Nov 16, 2018

Upstream CPython is thinking about making not-linking the default for Python 3.8: python/cpython#9912

@jpakkane
Copy link
Member

So this MR would mirror that behaviour for all versions of Python?

@jpakkane
Copy link
Member

ping

@lazka
Copy link
Contributor Author

lazka commented Nov 29, 2018

So this MR would mirror that behaviour for all versions of Python?

It doesn't currently. I'll look into that.

…does too. Fixes mesonbuild#4117

Windows requires things to be linked, on macOS distutils doesn't link by default.
On Linux etc. things are not so clear, some distros like Debian patch distutils to not link,
some don't. In addition the manylinux wheels spec prohibits linking against libpython
and upstream is thinking about changing the default:
https://bugs.python.org/issue34814

Call into distutils to figure out what distutils does and in case it doesn't link
against libpython replace the passed in Python dependency with a partial one.
@lazka lazka changed the title python: only link extension modules against libpython when building for Windows. Fixes #4117 python: only link extension modules against libpython when distutils does too. Fixes #4117 Dec 6, 2018
@lazka
Copy link
Contributor Author

lazka commented Dec 6, 2018

So this MR would mirror that behaviour for all versions of Python?

It doesn't currently. I'll look into that.

It does now.

@jpakkane
Copy link
Member

jpakkane commented Dec 6, 2018

Since this is a behavioural change and the dev freeze is in effect, we'll get this merged after the release on Sunday.

@jpakkane jpakkane merged commit 8e85a7b into mesonbuild:master Dec 10, 2018
@pkgw pkgw mentioned this pull request Jan 8, 2019
pkgw added a commit to pkgw/meson-feedstock that referenced this pull request Jan 25, 2019
In my testing, mesonbuild/meson#4187 is necessary to successfully compile
conda-forge/gobject-introspection-feedstock#9 on macOS. This patch can be
taken out whenever the next Meson release comes out.
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