Skip to content

bpo-14527: fix (some) _ctypes liffi build problems #20451

Closed
rupertnash wants to merge 2 commits intopython:mainfrom
rupertnash:fix-ffi-build
Closed

bpo-14527: fix (some) _ctypes liffi build problems #20451
rupertnash wants to merge 2 commits intopython:mainfrom
rupertnash:fix-ffi-build

Conversation

@rupertnash
Copy link

@rupertnash rupertnash commented May 27, 2020

Fundamentally, the problem appears to be that configure uses pkgconfig to find the libffi include directory, while setup.py's detect_ctypes only uses the global list of library directories.

I have made an attempt at fixing this by having configure produce the directory containing libffi (LIBFFI_LIBDIR) and setup.py use this. However I've hardly any experience with autotools, so I would be very happy to be corrected if this is no use at all.

https://bugs.python.org/issue14527

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@rupertnash

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Copy link

@lapsintra lapsintra left a comment

Choose a reason for hiding this comment

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

Worked for my setup.

Before following this commit, I had set PKG_CONFIG_PATH, LD_LIBRARY_PATH, LD_RUN_PATH and LIBRARY_PATH point towards the appropriate locations.

./configure did set correctly LIBFFI_INCLUDEDIR using pkgconfig

Still couldn't build module _ctypes

This commit solved my issue.

Copy link
Member

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

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

This patch seems correct to me.

setup.py Outdated
if ffi_inc is not None:
for lib_name in ('ffi', 'ffi_pic'):
if (self.compiler.find_library_file(self.lib_dirs, lib_name)):
fullpath = self.compiler.find_library_file(self.lib_dirs + ffi_libdir, lib_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use setup.py heuristics to find libname when you could just let pkgconfig gives the right hint for compilation host/target with pkg-config libffi --libs-only-l like you did with LIBFFI_LIBDIR and LIBFFI_INCLUDEDIR ?

Copy link
Author

Choose a reason for hiding this comment

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

That is a reasonable question.

The true answer is that while making my changes I tried to keep them as small as practical.

However when I came here to respond, I notice that this library search checks for libffi and libffi_pic, whether or not pkg-config is available on the compilation host.

If we simply create a new configure variable with say:

LIBFFI_LIBNAME="`"$PKG_CONFIG" libffi --libs-only-l 2> /dev/null | sed -e 's/^-l//;s/ *$//'`"

then we will lose the ability to find libffi_pic.{so,a} if there's no pkg-config.

We could set a default like LIBFFI_LIBNAME="ffi ffi_pic" and then have setup.py split it's value on space before looping much as now...

My feeling is to stick as we are, but happy to defer to core team

Copy link
Contributor

@pmp-p pmp-p Dec 5, 2020

Choose a reason for hiding this comment

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

whether or not pkg-config is available on the compilation host.

maybe if pkg-config is here then use it fully else use old way ( and good luck for cross compilation setup.py will pick anything in /usr instead of sysroot in most cases ).

ref: https://bugs.python.org/issue31710

Copy link
Contributor

@pmp-p pmp-p left a comment

Choose a reason for hiding this comment

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

my 3.9.1 android/wasm (with HAVE_FFI_PREP_CLOSURE_LOC and ffi git on wasm ) CI is quite happy with that patch, it looks like a good step forward into cross-compilation to me

Shillaker added a commit to faasm/cpython that referenced this pull request Dec 16, 2020
@rupertnash
Copy link
Author

Dear devs - I had forgotten about this PR until I had to rebuild python yesterday on a machine with libffi in a non-standard location. Due to some major changes in setup.py around libffi, I've basically had to re-implement my patch. I've done this and force-pushed this branch. (Old commits https://github.com/rupertnash/cpython/tree/fix-ffi-build-old)

I would appreciate your comments again and I will have a bit of time in the next week or two to actually get this merged

@josefwells
Copy link

I needed to make additional changes to pass -Wl,-rpath, to setup.py to get this to work. I wonder why there is support for building with an external SSL, but FFI lags behind.

@tom-kacvinsky
Copy link

I'd like to chime in and say this worked for Python 3.10.8 with a slight modification, but the patch would not apply against 3.11.0. It would be super nice if this PR was set up so it would merge cleanly into the main branch. My rationale for that is we use an embedded interpreter that runs on myriad Linux distributions, so we can't count on the system libffi being the same across distributions. Hence we need to use our own built from source libffi and point the python build at that libffi.

@encukou
Copy link
Member

encukou commented Mar 28, 2024

Sorry for the delay.
Meanwhile, this PR no longer applies; there is no setup.py now.

I've tested a build with env vars given by configure --help (see the issue), and everything looked good, but I'm not sure I tested correctly. Please do comment on the issue if there are more problems.

I'll close this PR. Sorry again for not getting to it earlier.

@encukou encukou closed this Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants