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

ENH: do not check extension module file name against expected suffix #322

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

dnicolodi
Copy link
Member

Checking that the extensions modules filenames end with a suffix accepted by the current Python interpreter assumes that the wheel being assembled is for the same Python platform. This is not true when cross-compiling. The check protects against a very unlikely occurrence and makes life harder for tools that allow cross-compiling Python wheels. Remove it.

See #321.

Checking that the extensions modules filenames end with a suffix
accepted by the current Python interpreter assumes that the wheel
being assembled is for the same Python platform. This is not true when
cross-compiling. The check protects against a very unlikely occurrence
and makes life harder for tools that allow cross-compiling Python
wheels. Remove it.

See mesonbuild#321.
@rgommers rgommers added the enhancement New feature or request label Feb 26, 2023
@rgommers
Copy link
Contributor

+1 for doing this. We can either remove the check completely, or remove it when we know we're cross-compiling. Which is something that I think we anyway need to know - we already do know on macOS, and outside of macOS it's a matter of checking for a cross file in config-settings.

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.

We can either remove the check completely, or remove it when we know we're cross-compiling.

I'd prefeer if we just kept the check, but limited to when we are not cross-compiling.

@dnicolodi
Copy link
Member Author

I added the check, but I came to the conclusion that it is not a good idea. Among other things, it prevents installing a data file in {platlib}, which I think is an acceptable thing to do.

@FFY00
Copy link
Member

FFY00 commented Mar 1, 2023

it prevents installing a data file in {platlib}

A data file that ends with .so... but yeah, that's reasonable.

@dnicolodi
Copy link
Member Author

A data file that ends with .so... but yeah, that's reasonable.

Oh, you are right. I forgot about the double extension check. However, if someone goes into the trouble of putting something that looks like a Python extension module for the wront interpreter version in {platlib} they win the right of having thing break if they try to import it.

@dnicolodi dnicolodi merged commit 3678a77 into mesonbuild:main Mar 1, 2023
@doronbehar
Copy link
Contributor

Thanks @rgommers for linking this PR from #321, I missed it somehow when browsing #321. While thinking my self how can I fix this issue with a patch applied upon the 0.12.1 tag, I noticed a few things which perhaps were missing from this PR?

First of all (easy), the docstring wasn't trimmed:

diff --git i/mesonpy/__init__.py w/mesonpy/__init__.py
index 78db5d8..5832d4e 100644
--- i/mesonpy/__init__.py
+++ w/mesonpy/__init__.py
@@ -341,10 +341,6 @@ class _WheelBuilder():
         and .so on other platforms) and, if they all share the same
         PEP 3149 filename stable ABI tag, return it.
 
-        All files that look like extension modules are verified to
-        have a file name compatibel with what is expected by the
-        Python interpreter. An exception is raised otherwise.
-
         Other files are ignored.
 
         """

Secondly, this if:

soext = sorted(_EXTENSION_SUFFIXES, key=len)[0]
abis = []
for path, _ in self._wheel_files['platlib']:
if path.suffix == soext:

Doesn't assume that the "Host" machine and "Build" / "Target" machine have the same "shared object" file extension (so usually)? Maybe this is too much to ask - that meson-python will support cross compiling from Linux to Windows for example, but perhaps at least a comment above this conditional would be nice.

@rgommers
Copy link
Contributor

Good points, thanks @doronbehar.

Maybe this is too much to ask - that meson-python will support cross compiling from Linux to Windows for example, but perhaps at least a comment above this conditional would be nice.

It's not too much to ask - I'm hoping we can make that work at some point. But in practice it's not that common to do that, so it may take a while before we need to work around this one. In the meantime, there's some bigger cross-compilation fish to fry, in Meson itself and in dealing with sysconfig and other things that everyone has to monkeypatch right now.

doronbehar added a commit to doronbehar/meson-python that referenced this pull request Mar 17, 2023
@doronbehar
Copy link
Contributor

Here's a small fixup PR with docstring and a comment to that condition: #349

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants