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

[FindPython] Work around PyPy bug #13

Merged
merged 17 commits into from
Sep 22, 2022

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Sep 22, 2022

Squash & merge into your PR.

https://foss.heptapod.net/pypy/pypy/-/issues/3816 CC @mattip. SOABI is supposed to be the source of truth but PyPy gets it wrong in sysconfig (& distutils.sysconfig, which CMake claims to prefer) so we are using EXT_SUFFIX instead.

@@ -9,9 +9,20 @@ project(jarowinkler LANGUAGES C CXX)
if(CMAKE_VERSION VERSION_LESS 3.18)
find_package(Python COMPONENTS Interpreter Development REQUIRED)
else()
set(Python_ARTIFACTS_INTERACTIVE TRUE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required but simpler.

@@ -11,7 +11,7 @@ function(create_cython_target _name)
endfunction(create_cython_target)

create_cython_target(_initialize_cpp)
Python_add_library(_initialize_cpp MODULE ${_initialize_cpp})
Python_add_library(_initialize_cpp MODULE WITH_SOABI ${_initialize_cpp})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only PyPy seems to care if this is missing. Maybe to avoid reading a CPython .so?

@henryiii henryiii changed the title [Find python] Work around PyPy bug [FindPython] Work around PyPy bug Sep 22, 2022
@maxbachmann maxbachmann changed the base branch from FindPython to FindPython2 September 22, 2022 13:08
@maxbachmann maxbachmann merged commit 92ffc15 into rapidfuzz:FindPython2 Sep 22, 2022
maxbachmann added a commit that referenced this pull request Sep 22, 2022
@mattip
Copy link

mattip commented Sep 22, 2022

This is strange, I was sure we had ironed the issues around CMake and PyPy out a while ago. Did something change? Do you have any documentation about SONAME being the source of truth?

@henryiii
Copy link
Contributor Author

henryiii commented Sep 22, 2022

In pybind11, we just use EXT_SUFFIX as the source of truth and ignore SOABI. But FindPython uses SOABI over EXT_SUFFIX; since PyPy returns an invalid SOABI, that means FindPython is broken if you add WITH_SOABI. It's also broken if you do not include it, since PyPy doesn't allow the SOABI to be missing. We iron around it in pybind11, but that's not a general solution for all the other inferior binding tools like Cython. :P

I believe the problem we fixed in 7.3.2 or something like that is SO was wrong, and pybind11 used to use that. But SOABI we didn't fix. (Unless it got fixed in a development release) https://cmake.org/cmake/help/latest/module/FindPython.html#result-variables

This has been true at least since CMake 3.17 AFAICT.

@henryiii
Copy link
Contributor Author

(Since you have to scroll a bit on the page, here's the relevant bit)

Information returned by distutils.sysconfig.get_config_var('SOABI') or computed from distutils.sysconfig.get_config_var('EXT_SUFFIX') or python-config --extension-suffix. If package distutils.sysconfig is not available, sysconfig.get_config_var('SOABI') or sysconfig.get_config_var('EXT_SUFFIX') are used.

Not sure how I feel about distutils.sysconfig being preferred over sysconfig, but there you go.

Is there a quick way to grab a dev build of PyPy, like a docker image? I tried the nightly download & fought through macOS blocking every little library used, and it seems SOABI is still broken by missing the platform even on today's build.

@henryiii
Copy link
Contributor Author

henryiii commented Sep 22, 2022

And this is why it works on pybind11:

https://github.com/pybind/pybind11/blob/f743bdf8e61b2dc4a8995a0485111bedd9c2cbb1/tools/pybind11NewTools.cmake#L96-L105

It was a workaround for EXT_SUFFIX being broken, but the fix unfortunately masks the fact that SOABI is still broken even after EXT_SUFFIX got fixed. SO was replaced by EXT_SUFFIX (starts with a dot and end with an extension) and SOABI (just the tag to add, no leading dot and no extension).

@mattip
Copy link

mattip commented Sep 22, 2022

Looking around for the implications of changing the SOABI. I think I noticed the missing bit and started fixing it, but got blocked by this issue and never followed up with the promised change to the SOABI.

@henryiii
Copy link
Contributor Author

Wheels get named correctly (and I'm guessing they will continue to be due to that PR merged in 2020), but PyPy can't open files created using the SONAME it reports - libname$SONAME.so is not picked up (and libname.so is not either).

@mattip
Copy link

mattip commented Oct 12, 2022

Fixed the SOABI in PyPY, the fix will be part of the next release.

@mattip
Copy link

mattip commented Nov 21, 2022

No, that fix broke packaging/tags, which would have broken pip and wheel, so I backed out the change. See pypa/packaging#607

@henryiii
Copy link
Contributor Author

This will be fixed in CMake 3.26 by computing the SOABI from EXT_SUFFIX. Scikit-build-core has a workaround and might even soon backport the fixed version.

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.

3 participants