aleaxit / gmpy

General Multi-Precision arithmetic for Python 2.6+/3+ (GMP, MPIR, MPFR, MPC)
https://gmpy2.readthedocs.io/en/latest/
GNU Lesser General Public License v3.0
521 stars 86 forks source link

cython C-API is broken in published wheels (and in the build_wheels.yml CI workflow) #447

Closed skirpichev closed 11 months ago

skirpichev commented 1 year ago

The problem is that we bundle libraries.

Then, first, probably we should also supply relevant header files (gmp.h, etc). Next, document how user can specify a custom path (gmpy2.libs) to libraries in the setup.py.

casevh commented 1 year ago

This is a fundamental flaw with binary wheels, a C-API, and gmpy2. An extension that uses gmpy2's CAPI must(*) use the same GMP, MPFR, and MPC libraries. The name-mangling that occurs on Linux and MacOS makes that somewhere between complicated and impossible. The assumption was that Python and the extensions use the same OS provided libraries. So the Cython tests can practically only be done when gmpy2 with the OS provided GMP, MPFR, and MPC libraries.

I think we could require that the development version of gmpy2 can be successfully compiled on the most recent Ubuntu LTS version. A workflow that tests gmpy2 against the most recent Ubuntu LTS and its libraries would be the best place to test Cython and the C-API.

Windows binary wheels are different. I'm currently including the header files, the DLLs, and the LIB definition files MSVC compiler under the gmpy2 directory. It should work with Cython and other extensions but I still need to test it.

(*) "Must" may be too strong of a word. It may work. But we can't guarantee it.

skirpichev commented 1 year ago

The assumption was that Python and the extensions use the same OS provided libraries.

Hmm, I don't think it's impossible. If you put header files to the gmpy2 package - they will be found. To find libraries (gmpy2.libs) - users could use LD_LIBRARY_PATH & set library_dirs option of the Extension class.

A workflow that tests gmpy2 against the most recent Ubuntu LTS and its libraries would be the best place to test Cython and the C-API.

We run these tests (pip_install_gmpy2.yml, but not for Ubuntu yet). But I don't see too much value in untested wheels with C-API for Cython...

casevh commented 11 months ago

I was able to successfully compile and run test_cython on Windows. I need to make some changes to the Windows process. More details to follow.

skirpichev commented 11 months ago

@casevh, if this was about 8a39f52 - I doubt this commit does fix anything. Cython tests works in the pip_install_gmpy2 for Windows too: https://github.com/aleaxit/gmpy/blob/8a39f525a766d15944e0c24b3b34e6be3f999407/.github/workflows/pip_install_gmpy2.yml#L82-L107

What doesn't work (and this issue is about): cython tests after installation of binary wheels in a clean environment.

skirpichev commented 11 months ago

Ok, I have successful cython tests locally (Linux) after installation of repaired (of repaired :)) wheels:

sk@note:~/src/gmpy/tmp-xxx/lib/python3.12/site-packages/gmpy2.libs $ ls -l
total 2.3M
-rw-r----- 1 sk sk  83K Dec 20 05:05 gmp.h
-rwxr-x--x 1 sk sk 728K Dec 20 03:55 libgmp-e0c82b6b.so.10.5.0
-rwxr-x--x 1 sk sk 660K Dec 20 03:55 libmpc-b4e96b6f.so.3.3.1
lrwxrwxrwx 1 sk sk   24 Dec 20 05:04 libmpc.so -> libmpc-b4e96b6f.so.3.3.1
-rwxr-x--x 1 sk sk 793K Dec 20 03:55 libmpfr-da886a52.so.6.2.1
lrwxrwxrwx 1 sk sk   25 Dec 20 04:09 libmpfr.so -> libmpfr-da886a52.so.6.2.1
lrwxrwxrwx 1 sk sk   25 Dec 20 04:08 libmpfr.so.6 -> libmpfr-da886a52.so.6.2.1
-rw-r----- 1 sk sk  17K Dec 20 04:06 mpc.h
-rw-r----- 1 sk sk  57K Dec 20 04:06 mpfr.h

As you can see, (1) header files were added. And (2) also symlinks to workaround library name mangling. include_dirs/library_dirs of the test_cython extension should include gmpy2.libs:

diff --git a/test_cython/setup_cython.py b/test_cython/setup_cython.py
index ad2de54..1e15895 100644
--- a/test_cython/setup_cython.py
+++ b/test_cython/setup_cython.py
@@ -1,19 +1,15 @@
 from setuptools import Extension, setup
 from Cython.Build import cythonize
-import platform
 import sys
 import os
 import gmpy2

-ON_WINDOWS = platform.system() == 'Windows'

 extensions = [
     Extension("test_cython", ["test_cython.pyx"],
-                include_dirs=sys.path + ([os.path.dirname(gmpy2.__file__)] if ON_WINDOWS else []),
-                library_dirs=sys.path + ([os.path.dirname(gmpy2.__file__)] if ON_WINDOWS else []),
-                libraries=['mpc','mpfr','gmp']
-            )
-]
+              include_dirs=sys.path + ([os.path.dirname(gmpy2.__file__), gmpy2.__path__[0] + '/../gmpy2.libs/']),
+              library_dirs=sys.path + ([os.path.dirname(gmpy2.__file__), gmpy2.__path__[0] + '/../gmpy2.libs/']),
+              libraries=['mpc','mpfr','gmp'])]

 setup(
     name="cython_gmpy_test",

But, probably, we should instead set PYTHONPATH to gmpy2.libs.

Similar workaround should work for MacOS (no name mangling, but a different directory: .dylibs) and Windows.

casevh commented 11 months ago

I've been debugging this while you were making also debugging it. I converged to roughly the same changes as you did.

Just some comments.

The combination of a C-API for gmpy2, name-mangling of libraries in a binary wheel, libraries that maintain their own state, and the preference to use binary wheels versus Linux pre-compiled versions or compiling from source is a challenging problem.

Thanks for working on the fixes. I'll look at them tomorrow.

skirpichev commented 11 months ago

Please don't use symlinks.

Actually they aren't symlinks anymore after wheel pack (on the Linux host, but I believe - on any platform, as zipfile doesn't support symlinks).

Name mangling is required on Linux since we can't shadow the OS provided libraries.

Yes, I realize there are reasons for this. On another hand I doubt it's too easy to interfere with the OS provided libraries (gmpy2.libs is a very non-standard place to search for headers/libraries).

So on Linux, gmpy2 and Cython must both use the OS provided libraries or they must both use the same name-mangled libraries.

That's something we could document. Other third-party cython extensions, intended to interoperate with gmpy2 could use libraries from our wheels just as shown above. I will work on above solution for Linux/MacOS wheels and hopefully will come with a pr in a few days.

An easy way: remove cython-related stuff from binary wheels and warn users that to have cython interface - they must build the package from sources.

casevh commented 11 months ago

Actually they aren't symlinks anymore after wheel pack (on the Linux host, but I believe - on any platform, as zipfile doesn't support symlinks).

And on Windows, "gmpy2.h" just contains "../src/gmpy2.h". It's technically not a symlink anymore...

Yes, I realize there are reasons for this. On another hand I doubt it's too easy to interfere with the OS provided libraries (gmpy2.libs is a very non-standard place to search for headers/libraries).

Is it possible do disable the name mangling when creating a Linux binary wheel? If so, could we use one consistent set of name-mangled libraries for an entire release cycle? For Windows, I plan to use the same GMP, MPFR, and MPC libraries for the entire 2.2.x release cycle. A change to those libraries would force a version change.

That's something we could document. Other third-party cython extensions, intended to interoperate with gmpy2 could use libraries from our wheels just as shown above. I will work on above solution for Linux/MacOS wheels and hopefully will come with a pr in a few days.

An easy way: remove cython-related stuff from binary wheels and warn users that to have cython interface - they must build the package from sources.

Remove the C-API when a binary wheel is created? Or is it possible to identify if gmpy2 was distributed as a binary wheel (look for gmpy2.libs??) and not import _C_API into the main gmpy2 namespace?

skirpichev commented 11 months ago

And on Windows, "gmpy2.h" just contains "../src/gmpy2.h".

Hmm, seems to be a platform-specific issue.

Is it possible do disable the name mangling when creating a Linux binary wheel?

This is non-opt feature of auditwheel.

Remove the C-API when a binary wheel is created?

Just remove headers. Then we can decide to not import C-API at runtime.