-
Notifications
You must be signed in to change notification settings - Fork 86
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
Drop gmpy2 directory & move source files to src/gmpy2/ #474
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #474 +/- ##
=======================================
Coverage 85.52% 85.52%
=======================================
Files 50 50
Lines 11656 11656
Branches 2202 2202
=======================================
Hits 9969 9969
Misses 1687 1687 ☔ View full report in Codecov by Sentry. |
@casevh, this is a draft pr, but let me know if you like this idea. |
I installed a Linux wheel. It imports and runs just fine. I then looked at the file layout. The shared library is installed in The following comments are based on memories from a discussion that occurred several years. I personally liked having I do like the faster import time (so +1), but I don't know if it is worth the possible long-term stability risks (-0.5) and the code churn (also -0.5). I'll continue to think about this... |
2d67094
to
0322c26
Compare
I don't sure I realize which scenario do you mean. The module so-file in the site-packages directory is a long-standing default for the |
0322c26
to
90909a6
Compare
This is ready for review. |
90909a6
to
41e61fd
Compare
This uses more modern directory layout (like the ``python-flint`` does, for example) and avoids extra pure-python code while doing import. The drawback is that you have to use extra ``--follow`` arguments for some ``git`` commands (e.g. ``git log``) to access early history. With this patch: ``` $ time python -c 'from gmpy2 import *' real 0m0.065s user 0m0.050s sys 0m0.015s ``` On the master: ``` $ time python -c 'from gmpy2 import *' real 0m0.120s user 0m0.095s sys 0m0.024s ``` Uses patched delocate from my repo: git+https://github.com/skirpichev/delocate.git@fix-lib-sdir
41e61fd
to
0f499bd
Compare
Hmm, it seems delvewheel in this settings puts all libraries to the site-packages/ dir. Will think if it's possible to workaround this. Meanwhile, mark this as a draft. Please don't merge. |
Well, the problem is that PE format has nothing like RPATH in ELF... So, options are either
The delvewheel uses both options, respectively, (1) - if extension module is top-level, and (2) - if it has I think (1) is fine, if we will also use name-mangling on M$ Windows (now - turned off). Not sure if it's file without name-mangling. Maybe the better alternative is postpone this pr and, instead, package GMP, MPFR and MPC libraries as separate packages like scipy-openblas64? |
I propose option (1) but renaming the DLLs to well-defined names. The
renaming is done before gmpy2 is compiled.
Look at https://github.com/emilbayes/rename-dll for a utility to do the
renaming.
For a naming convention, how about libgmp-10.dll becomes
lib_gmpy2v2_2-10.dll and similarly for mpfr and mpc. The implies that the
DLLs were distributed as part of gmpy2 version 2.2.x. The name would change
if the DLLs change in a backwards incompatible way. And that should trigger
a new version of gmpy2.
We won't conflict with standard names. And C extensions are able to use the
renamed DLLs.
…On Fri, Mar 29, 2024 at 6:57 AM Sergey B Kirpichev ***@***.***> wrote:
Well, the problem is that PE format has nothing like RPATH in ELF... So,
options are either
1. put dll's alongside with the module dll.
2. use __init__.py file and put here suitable os.add_dll_directory()
calls, that will point to the gmpy2.libs dir.
The delvewheel uses both options, respectively, (1) - if extension module
is top-level, and (2) - if it has __init__.py file.
I think (1) is fine, if we will also use name-mangling on M$ Windows (now
- turned off). Not sure if it's file without name-mangling. Maybe the
better alternative is postpone this pr and, instead, package GMP, MPFR and
MPC libraries as separate packages like scipy-openblas64?
—
Reply to this email directly, view it on GitHub
<#474 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMR235AJ6DPLVES3FDMYK3Y2VXNRAVCNFSM6AAAAABFEL2QICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXGI4DIMBZGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I guess you meant something like "libgmp_gmpy2v2_2-10.dll". Your variant doesn't include library name.
Do you suggest to restrict this workaround only for M$ Windows? I would prefer a common setup for all platforms. That's why I'm thinking about separate packages for libraries. Probably, this should be discussed in #352: now we have binary wheels for all platforms, what are your plans for the referenced issue? |
Correct.
It looks like flint has updated their build system to meson. flintlib/python-flint#52 I would like to focus on resolving the challenges with creating extensions that want to use gmpy2's C-API. I think it is fine to require installing a binary wheel of gmpy2 to get the GMP, MPFR, and MPC libraries. The fundamental issues are (1) predictable names and (2) finding the required files. For Windows, I slightly prefer to place the supporting files in the same directoy as the gmpy2 pyd file. That layout has been used in the past. This does require importing gmpy2 before importing the C extension but I don't think that is unreasonable. |
There are many unresolved questions in this PR. Would you be okay if we release gmpy2 2.2.0 quickly to address support for Python 3.12 and 3.13 and delay this PR until gmpy2 2.3 (hopefully released when Python 3.14 is released)? If you agree, I'll bump the version to b1 and release it ASAP for testing. Then release rc1 shortly after 3.13.0b1 is released. |
@casevh, sure! It's a draft pr, it shouldn't block anything. But maybe you would like also to do a minor release for 2.1.x with support for CPython 3.12/13? |
Maybe just release 2.1.6 as source-only and release 2.2.0 as rc1. The "pip install gmpy2" should grab the rc1 version and a standard 2.1.6 version is available for Linux distributions. I'll probably increment the version to b1 this evening. Are there any last minute changes you want to make? |
Does make sense for me.
No:) I would like to solve #467, but it seems that new C API will not land in the CPython 3.13. |
This uses more modern directory layout (like python-flint does, for example) and avoids extra pure-python code while doing import. The drawback is that you have to use extra
--follow
arguments for some git commands (e.g.git log
) to access early history.With this patch:
On the master:
Uses patched delocate from my repo: git+https://github.com/skirpichev/delocate.git@fix-lib-sdir