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

platform_tag is wrong on macOS for Homebrew Python #91

Closed
mkoeppe opened this issue Jul 3, 2022 · 26 comments
Closed

platform_tag is wrong on macOS for Homebrew Python #91

mkoeppe opened this issue Jul 3, 2022 · 26 comments
Labels
dependency-bug A bug experienced by users of meson-python caused by a dependency, rather than in code in this repo

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 3, 2022

meson-python 0.6.0 generates a wheel the platform tag macosx_12_x86_64, which cannot be installed by pip.

Ref: https://trac.sagemath.org/ticket/34081#comment:15

Why not use https://github.com/pypa/packaging/blob/main/packaging/tags.py?

@rgommers
Copy link
Contributor

rgommers commented Jul 4, 2022

I see a second issue, the ABI tag is incorrect:

% # Note that the --no-isolation is needed to work around the strip issue (gh-90)
% python -m build --skip-dependency-check --no-isolation
scipy-1.10.0.dev0-py3-none-macosx_11_0_arm64.whl

The WHEEL metadata is:

Wheel-Version: 1.0
Generator: meson
Root-Is-Purelib: false
Tag: py3-none-macosx_11_0_arm64

@rgommers
Copy link
Contributor

rgommers commented Jul 4, 2022

I double checked on Linux, and (as expected) it's fine there: scipy-1.10.0.dev0-cp310-cp310-linux_x86_64.whl.

@rgommers
Copy link
Contributor

rgommers commented Jul 5, 2022

Hmm, after looking into this some more, this is indeed very incomplete:

  • mesonpy/_tags.py is writting from scratch and only supports Linux, and Windows partially (plus a stable ABI tag)
  • _select_abi_tag in __init__.py does some checks, but if something doesn't match the linux/windows/stable tags, it happily returns an incorrect tag

Why not use https://github.com/pypa/packaging/blob/main/packaging/tags.py?

I agree, there's no need to reinvent this wheel. That file is compatibly-licensed, so can be reused.

@FFY00 FFY00 added the bug Something isn't working label Jul 5, 2022
@FFY00
Copy link
Member

FFY00 commented Jul 5, 2022

The big difference from our code to pypa/packaging is that we derive our tags directly from the extension modules, while pypa/packaging tries to list all compatible tags for the current interpreter.

While I still think our code is valuable, but we should definitely make use of pypa/packaging too.

@rgommers
Copy link
Contributor

rgommers commented Jul 5, 2022

The big difference from our code to pypa/packaging is that we derive our tags directly from the extension modules

Does that help much / is that going to be robust? On Linux I see a useful extension, like *.cpython-310-x86_64-linux-gnu.so. On macOS that doesn't seem to be the case though, the extensions are simply .cpython-39-darwin.so. No architecture included, nor the macOS version (typically given by MACOSX_DEPLOYMENT_TARGET).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 5, 2022

https://github.com/pypa/wheel/blob/main/src/wheel/bdist_wheel.py and https://github.com/pypa/wheel/blob/main/src/wheel/macosx_libfile.py also have relevant code.

@FFY00
Copy link
Member

FFY00 commented Jul 6, 2022

Does that help much / is that going to be robust?

Yep, because the modules can be built with the stable ABI for eg. In Linux it seems the extension tag are a direct mapping but this is not the case for macOS.

Unfortunately, the spec says the correct platform-specific tag is defined by distutils.util.get_platform, which we have to recreate now in the downstream because distutils is deprecated. We can use wheel.macosx_libfile.calculate_macosx_platform_tag.

@rgommers
Copy link
Contributor

rgommers commented Jul 6, 2022

Makes sense. I actually wouldn't know how we'd go from "detect this uses the stable ABI" to extension name. Does sysconfig check for Py_LIMITED_API when retrieving the extension or some such thing?

@FFY00
Copy link
Member

FFY00 commented Jul 6, 2022

No, I don't think sysconfig has any knowledge of this, as it doesn't really need to. We just check if all extensions are using the stable ABI and if so we use the stable ABI tag in the wheel.

@rgommers
Copy link
Contributor

rgommers commented Jul 6, 2022

We just check if all extensions are using the stable ABI and if so we use the stable ABI tag in the wheel.

Maybe I am missing the point, so let me rephase: I think you are checking the extension name in meson-python, but what determines that extension name and makes it includes the stable ABI tag to begin with?

@FFY00
Copy link
Member

FFY00 commented Jul 6, 2022

That would be Meson. IIRC Meson won't let you easily create a stable ABI extension easily, but that can be done manually. For this to be done correctly, you should set Py_LIMITED_API when compiling and use abi3 tag instead of sysconfig.get_config_var('SOABI') as the tag in the extension name.

@eli-schwartz
Copy link
Member

I suppose that is something that could be added to Meson if people want it. Personally, I haven't really seen the limited API used in projects much...

The implementation would probably look something like limited: true as a kwarg to extension module which automatically defines Py_LIMITED_API and sets the soname? Incidentally, Meson uses EXT_SUFFIX. :)

@FFY00
Copy link
Member

FFY00 commented Jul 7, 2022

Hi Eli, that sounds like a good proposal. I would probably call the option something like limited_abi to be more explicit, though. Additionally, the argument should be the version to set in Py_LIMITED_API.

See https://docs.python.org/3/c-api/stable.html#stable-application-binary-interface

Meson uses EXT_SUFFIX

Which itself is based on SOABI 😛. The correct way to build the name in this situation would be

'.'.join(extension_name, 'abi3', sysconfig.get_config_var('SHLIB_SUFFIX'))'

@eli-schwartz
Copy link
Member

Sure, many of the sysconfig variables are based on each other. It was convenient for simplicity to use that one.

I feel a bit scared to actually change the variables used, considering that it's not really well documented what variables are there or which ones to use for any given situation. :(

@FFY00
Copy link
Member

FFY00 commented Jul 7, 2022

You can keep using SOABI in the normal code path, and only use the code I provided when the limited ABI is used. That's why I hardcoded abi3.

@FFY00
Copy link
Member

FFY00 commented Jul 19, 2022

Now that I have a macOS machine, I have reproduced this. The thing is, macosx_12_x86_64 appears to be a valid tag according to the spec, as it the normalized form of the value returned by distutils.util.get_platform. We are currently using sysconfig.get_platform, which should work the same as the distutils counterpart.

>>> distutils.util.get_platform()
'macosx-12-arm64'
>>> sysconfig.get_platform()
'macosx-12-arm64'

The issue here is that pypa/packaging does not consider macosx_12_x86_64 as a valid option, only macosx_12_0_x86_64, even though AFAICT it is perfectly valid.

>>> list(packaging.tags.mac_platforms())
['macosx_12_0_arm64',
 'macosx_12_0_universal2',
 'macosx_11_0_arm64',
 'macosx_11_0_universal2',
 'macosx_10_16_universal2',
 'macosx_10_15_universal2',
 'macosx_10_14_universal2',
 'macosx_10_13_universal2',
 'macosx_10_12_universal2',
 'macosx_10_11_universal2',
 'macosx_10_10_universal2',
 'macosx_10_9_universal2',
 'macosx_10_8_universal2',
 'macosx_10_7_universal2',
 'macosx_10_6_universal2',
 'macosx_10_5_universal2',
 'macosx_10_4_universal2']

I see a second issue, the ABI tag is incorrect:

I think that was just #95, can you check against main? I am having some trouble building SciPy on mac.

@FFY00
Copy link
Member

FFY00 commented Jul 20, 2022

Opened pypa/packaging#578, so I'll close this. @rgommers once you manage to verify if the bug you mentioned still exists, please open a new issue if it does. Once that is done, I think we can do a release. Thanks!

@mkoeppe do let me know if you have further feedback or you think I missed anything 😊

@rgommers
Copy link
Contributor

I see a second issue, the ABI tag is incorrect:

I think that was just #95, can you check against main?

Correct, that was gh-95, unrelated to this issue.

Opened pypa/packaging#578, so I'll close this.

I determined that the problem is Homebrew-specific, so I'll update the title of this issue. It's not actually fixed until a new packaging release with the fix comes out, so other people are likely to run into this. Hence I'll reopen this and add a "dependency-bug" label. Once the fix is out, we should bump the minimum packaging version in this repo so it includes that fix.

@rgommers rgommers reopened this Jul 22, 2022
@rgommers rgommers changed the title platform_tag is wrong on macOS platform_tag is wrong on macOS for Homebrew Python Jul 22, 2022
@rgommers rgommers added dependency-bug A bug experienced by users of meson-python caused by a dependency, rather than in code in this repo and removed bug Something isn't working labels Jul 22, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

It's not actually fixed until a new packaging release with the fix comes out

More precisely, until a new pip release comes out that vendors a fixed packaging (https://github.com/pypa/pip/tree/main/src/pip/_vendor/packaging)

@rgommers
Copy link
Contributor

Thanks for the correction. Or, alternatively, if Homebrew fixes what they're doing here - or we add a workaround for it in this package. pip is a pain, because even if it is fixed in a new release, people tend to use older pip versions for a long time.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

+1 on preparing a workaround.

@rgommers
Copy link
Contributor

This was fixed in gh-111, which is in 0.8.0.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 26, 2022

Thanks a lot, I confirm that it fixes the problem.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 1, 2022

I now have a similar problem on Linux 32-bit userspace on 64-bit, opened #123

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2022

This problem on macOS has come back with meson-python 0.9.0

https://trac.sagemath.org/ticket/34081#comment:90

@rgommers
Copy link
Contributor

That's not the same, let me open a new issue to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency-bug A bug experienced by users of meson-python caused by a dependency, rather than in code in this repo
Projects
None yet
Development

No branches or pull requests

4 participants