python: Use correct extension filename suffix on Python < 3.8.7 #10961

dnicolodi · 2022-10-27T09:12:29Z

In Python version prior to 3.8.7 the sysconfig provides the wrong suffix. The right one can be obtained from distutils.sysconfig. distutils is deprecated and will be removed in Python 3.12 thus get the information from distutils only when building for a Python version know to present this issue.

Simplify the extension module filename suffix lookup to use the same method used by setuptools.

Fixes #10960.

jpakkane · 2022-10-27T10:28:00Z

I'd say this qualifies for inclusion to rc2.

dnicolodi · 2022-10-27T10:29:52Z

Does meson really support building extensions for Python 2.x? I didn't know that and the test failures reflect that. I just pushed an updated patch that should work building for Python 2.x as well.

xclaesse · 2022-10-27T11:23:47Z

Does meson really support building extensions for Python 2.x?

In theory yes, but I don't know if that's tested, so I would not be surprised if it's broken.

dnicolodi · 2022-10-27T11:26:12Z

Does meson really support building extensions for Python 2.x?

In theory yes, but I don't know if that's tested, so I would not be surprised if it's broken.

It is tested in the CI jobs, the test failures are what me realize it is a thing.

eli-schwartz · 2022-10-27T11:26:42Z

I mean, the fact that the CI failed presumably means that it is in fact tested? Not sure how else that would work lol.

eli-schwartz · 2022-10-27T11:29:49Z

In Python version prior to 3.8.7 the sysconfig provides the wrong suffix.

For the record, this is false. The sysconfig module does not provide the wrong suffix.

It provides a valid suffix, but a less awesome one.

...

According to mesonbuild/meson-python#189 this PR doesn't even fix the issue, though, and the real fix will be to stop expecting the wheel filename to exactly match the extension filename suffix? Do we even need to make this code more complex?

codecov · 2022-10-27T11:33:24Z

Codecov Report

Merging #10961 (904310f) into master (bfc8132) will decrease coverage by 18.12%.
The diff coverage is n/a.

❗ Current head 904310f differs from pull request most recent head d31cefc. Consider uploading reports for the commit d31cefc to get more accurate results

@@             Coverage Diff             @@
##           master   #10961       +/-   ##
===========================================
- Coverage   68.58%   50.46%   -18.13%     
===========================================
  Files         412      207      -205     
  Lines       87861    44888    -42973     
  Branches    20728     9919    -10809     
===========================================
- Hits        60261    22652    -37609     
+ Misses      23093    20196     -2897     
+ Partials     4507     2040     -2467

Impacted Files	Coverage Δ
modules/qt5.py	`0.00% <0.00%> (-100.00%)`	⬇️
modules/modtest.py	`0.00% <0.00%> (-100.00%)`	⬇️
scripts/run_tool.py	`0.00% <0.00%> (-100.00%)`	⬇️
scripts/msgfmthelper.py	`0.00% <0.00%> (-100.00%)`	⬇️
modules/icestorm.py	`0.00% <0.00%> (-97.15%)`	⬇️
modules/keyval.py	`0.00% <0.00%> (-95.24%)`	⬇️
scripts/clangtidy.py	`0.00% <0.00%> (-93.34%)`	⬇️
scripts/vcstagger.py	`0.00% <0.00%> (-91.67%)`	⬇️
modules/sourceset.py	`0.00% <0.00%> (-90.54%)`	⬇️
scripts/depscan.py	`0.00% <0.00%> (-83.45%)`	⬇️
... and 324 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mesonbuild/modules/python.py

dnicolodi · 2022-10-27T11:34:49Z

AFAIU, the extension module generated by Meson cannot be loaded by Python < 3.8.7 on Windows as it has the wrong file name. This is independent from packaging the extension into a Python wheel.

eli-schwartz · 2022-10-27T11:38:26Z

What makes you believe this? Python is supposed to load backwards-compatible (2.x-style) extension filename suffixes that don't have a version tag, for example on Linux it will try to load siphash24.so if it cannot find siphash.cpython-310-x86_64-linux-gnu.so.

(This can actually result in bugs if you manually mess with PYTHONPATH and end up accidentally adding python2 artifacts to a python3 interpreter -- it will try to load python2 modules, then fail with an ImportError due to missing C symbols for the CPython ABI.)

It even checks more candidate filenames too, because there's the .abi3.so suffix as well.

dnicolodi · 2022-10-27T11:50:12Z

I stand corrected. I fixed the commit message accordingly. Is having the more awesome suffix something desirable?

eli-schwartz · 2022-10-27T12:10:13Z

Well, the awesome suffix is, after all, more awesome.

The only reasons to think about not doing it are:

avoiding too much growth in the length of the script, which we used to run via python -c... but we don't do that anymore
distutils is deprecated and going away in python 3.12, and we already need to figure out how to replace the two things that sysconfig couldn't do (that being: figure out whether python is embedded or linked, and, figure out how debian patches distutils for the install paths). This adds a third use.

dnicolodi · 2022-10-27T12:14:05Z

distutils is deprecated and going away in python 3.12, and we already need to figure out how to replace the two things that sysconfig couldn't do (that being: figure out whether python is embedded or linked, and, figure out how debian patches distutils for the install paths). This adds a third use.

This is the reason why I decided to use distutils.sysconfig only for old Python versions, as mentioned in the commit message.

dnicolodi · 2022-10-27T13:50:04Z

The test failures are various timeouts, most likely unrelated to this patch.

eli-schwartz · 2022-10-27T13:59:21Z

This is the reason why I decided to use distutils.sysconfig only for old Python versions, as mentioned in the commit message.

Right. I don't even know that it's possible to just avoid using distutils at all (it's a nice thought to imagine we could).

dnicolodi · 2022-10-27T14:22:40Z

Right. I don't even know that it's possible to just avoid using distutils at all (it's a nice thought to imagine we could).

distutils will be removed from Python 3.12, thus Debian will not have anymore a distutils module to patch, thus they will have either to embed setuptools with the Python package or they will have to reflect their non-standard setup in the configuration data available via sysconfig. Thus this use-case will solve itself (but will require some more ugly Python version checking).

For the same reason, all the information now available via distutils will need to be made available via other means. Thus I am optimistic a distutils-less Meson is possible (at least when building for Python 3.12 or later).

eli-schwartz · 2022-10-27T14:29:25Z

I meant avoid using it at all on any version. :p

Might require debian to get serious about consistently patching their patches.

dnicolodi · 2022-10-27T14:34:58Z

I must admit that I don't completely understand the issue with Debian. It is a long time since I looked into this, but IIRC, the Debian Python is happy to load modules both from the site-packages and the dist-packages directories, the latter being ideally reserved for Python packages installed via Debian packages. Why does Meson need to install in dist-packages?

dnicolodi · 2022-10-27T14:41:43Z

Azure vs2019 vc2019x64ninja fails trying to upload to Codecov (I don't know why the same failure is reported twice).

macos failure seems to be the test runner timing out in an unorderly way. I reconstruct the traceback to be:

concurrent.futures.process._RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.10.8/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/subprocess.py", line 134, in wait
    return await self._transport._wait()
  File "/usr/local/Cellar/[email protected]/3.10.8/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/base_subprocess.py", line 235, in _wait
    return await waiter
asyncio.exceptions.CancelledError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.10.8/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/tasks.py", line 456, in wait_for
    return fut.result()
asyncio.exceptions.CancelledError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.10.8/Frameworks/Python.framework/Versions/3.10/lib/python3.10/concurrent/futures/process.py", line 246, in _process_worker
    r = call_item.fn(*call_item.args, **call_item.kwargs)
  File "/Users/runner/work/meson/meson/run_project_tests.py", line 623, in run_test
    return _run_test(test, build_dir, install_dir, extra_args, should_fail)
  File "/Users/runner/work/meson/meson/run_project_tests.py", line 713, in _run_test
    (returncode, tstdo, tstde, test_log) = run_test_inprocess(test_build_dir)
  File "/Users/runner/work/meson/meson/run_project_tests.py", line 543, in run_test_inprocess
    returncode_test = mtest.run_with_args(['--no-rebuild'])
  File "/Users/runner/work/meson/meson/mesonbuild/mtest.py", line 2086, in run_with_args
    return run(options)
  File "/Users/runner/work/meson/meson/mesonbuild/mtest.py", line 2073, in run
    return th.doit()
  File "/Users/runner/work/meson/meson/mesonbuild/mtest.py", line 1754, in doit
    self.run_tests(runners)
  File "/Users/runner/work/meson/meson/mesonbuild/mtest.py", line 1893, in run_tests
    loop.run_until_complete(self._run_tests(runners))
  File "/usr/local/Cellar/[email protected]/3.10.8/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/Users/runner/work/meson/meson/mesonbuild/mtest.py", line 1993, in _run_tests
    await complete_all(futures)
  File "/Users/runner/work/meson/meson/mesonbuild/mtest.py", line 1202, in complete_all
    check_futures(done)
  File "/Users/runner/work/meson/meson/mesonbuild/mtest.py", line 1192, in check_futures
    f.result()
asyncio.exceptions.TimeoutError
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/runner/work/meson/meson/./run_project_tests.py", line 1604, in <module>
    res = run_tests(all_tests, 'meson-test-run', options.failfast, options.extra_args, options.use_tmpdir, options.num_workers)
  File "/Users/runner/work/meson/meson/./run_project_tests.py", line 1128, in run_tests
    return _run_tests(all_tests, log_name_base, failfast, extra_args, use_tmp, num_workers, lf)
  File "/Users/runner/work/meson/meson/./run_project_tests.py", line 1273, in _run_tests
    result = f.result
  File "/Users/runner/work/meson/meson/./run_project_tests.py", line 1155, in result
    return self.future.result() if self.future else None
  File "/usr/local/Cellar/[email protected]/3.10.8/Frameworks/Python.framework/Versions/3.10/lib/python3.10/concurrent/futures/_base.py", line 451, in result
    return self.__get_result()
  File "/usr/local/Cellar/[email protected]/3.10.8/Frameworks/Python.framework/Versions/3.10/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result
    raise self._exception
asyncio.exceptions.TimeoutError

eli-schwartz · 2022-10-27T14:53:26Z

but IIRC, the Debian Python is happy to load modules both from the site-packages and the dist-packages directories

I'm pretty sure they only do the former in /usr/local and only do the latter in /usr.

dnicolodi · 2022-11-01T16:13:32Z

Force pushed to retry the failing tests.

eli-schwartz · 2022-11-01T16:16:39Z

I tried restarting it a couple times already though... rerunning the entire suite of 33 runners instead of pinging again to see if someone can restart it seems... slow.

dnicolodi · 2022-11-01T16:21:00Z

Yeah, sorry. I thought that pushing to the branch would have run all the tests without manual intervention from someone. It turns out that it still requires approval so no time spared for anyone.

eli-schwartz · 2022-11-01T18:51:49Z

Windows test failure is significant.

dnicolodi · 2022-11-01T19:01:36Z

Thanks @eli-schwartz The log ends with what looks like a timeout uploading to Coverity and I focused on that instead on the real failure. I guess the .lib filename needs to match the .pyd filename, thus the test harness needs to be fixed, not the implementation. WDYT?

eli-schwartz · 2022-11-01T19:14:09Z

Yes.

Also #10979 for avoiding confusion in the future, this is not the first time. :D

dnicolodi · 2022-11-02T10:24:45Z

Thanks @eli-schwartz The vs2019 tests pass now. Funny note: the tests passed before only because they run on Python 3.7 only, they would have failed with later Python version where the .lib have the more elaborate suffix that this PR introduces for Python 3.7. Can you make the other tests run again too, please?

eli-schwartz · 2022-11-02T11:36:24Z

Is it sufficient to have py_implib add the python suffix and instead of returning, continue on to the generic implib handling?

dnicolodi · 2022-11-02T13:02:42Z

I think it could be possible. I coded it this way because I am not sure that the filename prefix mangling done in the MSVC case applied to python extension modules and to keep the handling of the python specific file types in the same section of the code. If there is preference for having it coded as you suggest, I can change it in that direction.

dnicolodi · 2022-11-02T15:41:53Z

I've now implemented @eli-schwartz's suggestion.

dnicolodi · 2022-11-02T18:31:16Z

Nope, went back to the former implementation, it is much simpler.

dnicolodi · 2022-11-04T07:56:47Z

Anything missing for merging this? The remaining test failure is unrelated and should be fixed by recent commits. Maybe @eli-schwartz can restart it to verify

dnicolodi · 2022-11-16T09:43:14Z

ping?

eli-schwartz · 2022-11-16T12:17:32Z

Fixing the CI requires running it with recent commits, which in turn means rebasing -- when you restart CI, it just checks out the old merge result of "whatever the master branch was at when the PR was submitted", it doesn't try re-merging.

On Windows, in Python version prior to 3.8.7, the `sysconfig` modules provides an extension filename suffix that disagrees the one returned by `distutils.sysconfig`. Get the more awesome suffix from the latter when building for a Python version known to present this issue. Simplify the extension module filename suffix lookup to use the same method used by `setuptools`. Adjust project tests accordingly. Fixes mesonbuild#10960.

dnicolodi · 2022-11-16T12:26:05Z

Right, I forgot this, sorry. Rebased.

dnicolodi · 2022-11-16T16:31:32Z

All green!

dnicolodi · 2022-11-23T10:04:43Z

ping?

dnicolodi requested a review from jpakkane as a code owner October 27, 2022 09:12

dnicolodi mentioned this pull request Oct 27, 2022

Curious build failure with Python 3.7 on Windows mesonbuild/meson-python#189

Closed

dnicolodi force-pushed the python-ext-suffix branch from 5f6d17c to 0eef3af Compare October 27, 2022 10:25

dnicolodi force-pushed the python-ext-suffix branch from 0eef3af to aa74731 Compare October 27, 2022 11:18

eli-schwartz requested changes Oct 27, 2022

View reviewed changes

mesonbuild/modules/python.py Outdated Show resolved Hide resolved

mesonbuild/modules/python.py Outdated Show resolved Hide resolved

dnicolodi force-pushed the python-ext-suffix branch 2 times, most recently from 8096a82 to 06303a7 Compare October 27, 2022 11:46

dnicolodi requested review from eli-schwartz and removed request for jpakkane October 27, 2022 11:59

dnicolodi force-pushed the python-ext-suffix branch from 06303a7 to 95a7975 Compare November 1, 2022 16:10

dnicolodi force-pushed the python-ext-suffix branch from 95a7975 to f398b94 Compare November 1, 2022 19:28

dnicolodi force-pushed the python-ext-suffix branch from f398b94 to 82bb026 Compare November 2, 2022 15:39

dnicolodi force-pushed the python-ext-suffix branch from 82bb026 to 9a06d44 Compare November 2, 2022 17:49

dnicolodi force-pushed the python-ext-suffix branch from 9a06d44 to d31cefc Compare November 16, 2022 12:25

eli-schwartz approved these changes Nov 23, 2022

View reviewed changes

eli-schwartz merged commit 235f32f into mesonbuild:master Nov 23, 2022

python: Use correct extension filename suffix on Python < 3.8.7 #10961

python: Use correct extension filename suffix on Python < 3.8.7 #10961

Conversation

dnicolodi commented Oct 27, 2022

jpakkane commented Oct 27, 2022

dnicolodi commented Oct 27, 2022

xclaesse commented Oct 27, 2022

dnicolodi commented Oct 27, 2022

eli-schwartz commented Oct 27, 2022

eli-schwartz commented Oct 27, 2022

codecov bot commented Oct 27, 2022 • edited Loading

Codecov Report

dnicolodi commented Oct 27, 2022

eli-schwartz commented Oct 27, 2022 • edited Loading

dnicolodi commented Oct 27, 2022

eli-schwartz commented Oct 27, 2022 • edited Loading

dnicolodi commented Oct 27, 2022

dnicolodi commented Oct 27, 2022

eli-schwartz commented Oct 27, 2022

dnicolodi commented Oct 27, 2022

eli-schwartz commented Oct 27, 2022

dnicolodi commented Oct 27, 2022

dnicolodi commented Oct 27, 2022

eli-schwartz commented Oct 27, 2022

dnicolodi commented Nov 1, 2022

eli-schwartz commented Nov 1, 2022

dnicolodi commented Nov 1, 2022

eli-schwartz commented Nov 1, 2022

dnicolodi commented Nov 1, 2022

eli-schwartz commented Nov 1, 2022

dnicolodi commented Nov 2, 2022

eli-schwartz commented Nov 2, 2022

dnicolodi commented Nov 2, 2022

dnicolodi commented Nov 2, 2022

dnicolodi commented Nov 2, 2022

dnicolodi commented Nov 4, 2022

dnicolodi commented Nov 16, 2022

eli-schwartz commented Nov 16, 2022

dnicolodi commented Nov 16, 2022

dnicolodi commented Nov 16, 2022

dnicolodi commented Nov 23, 2022

codecov bot commented Oct 27, 2022 •

edited

Loading

eli-schwartz commented Oct 27, 2022 •

edited

Loading

eli-schwartz commented Oct 27, 2022 •

edited

Loading