Python: Fix limited API under mingw #13171
Conversation
|
Disappointingly the tests seem to have all passed which makes me think we are missing something which tests precisely whether the right library was linked or not by shelling out to ldd/dumpbin/otool. I'm going to add that now into unittests/pythontests (where I should have added such a test in the initial limited api submission now that I think about it). |
|
I find that this PR fixes the issue: nasa-gcn/hpx#21 |
amcn
force-pushed
the
mingw-python-limited-api
branch
from
fd9c8b0
to
155a3e9
Compare
|
I've reworked this PR to do two things:
Hopefully this is easier to review now. Feedback very welcome. |
amcn
force-pushed
the
mingw-python-limited-api
branch
from
155a3e9
to
2a7aeb3
Compare
| args: [files('test_limited.py')], | ||
| env: { 'PYTHONPATH': meson.current_build_dir() }, | ||
| workdir: meson.current_source_dir() | ||
| ) |
There was a problem hiding this comment.
File endings look weird. :P
There was a problem hiding this comment.
Oof, I always forget about this. Fix incoming.
unittests/pythontests.py
Outdated
| testdir = os.path.join( | ||
| self.src_root, | ||
| 'test cases', | ||
| 'python', | ||
| '9 extmodule limited api' | ||
| ) |
There was a problem hiding this comment.
I don't see a good reason to split this across 6 different lines, we don't do so elsewhere.
| output = subprocess.check_output(['dumpbin', '/DEPENDENTS', limited_library_path], | ||
| stderr=subprocess.STDOUT) | ||
| self.assertIn(limited_dep_name, output.decode()) | ||
| elif shutil.which('objdump'): | ||
| # mingw | ||
| output = subprocess.check_output(['objdump', '-p', limited_library_path], | ||
| stderr=subprocess.STDOUT) |
There was a problem hiding this comment.
Why do we want to look in stderr as well?
There was a problem hiding this comment.
Mostly out of paranoia. When I was implementing this it was handy to have both stderr and stdout so that when it didn't work correctly, the contents of both stdout and stderr ended up in the log. Should this be removed?
Yeah, flipping them around is clearer. I'll push a new series up now with them flipped (and other issues addressed). |
amcn
force-pushed
the
mingw-python-limited-api
branch
from
2a7aeb3
to
4ec0e34
Compare
Based on the example in GH issue mesonbuild#13167, the limited API test has been extended with a test to load the compiled module to ensure it can be loaded correctly.
This new unittest runs the existing Python limited API test case, and checks that the resulting library was linked to the correct library.
This is in preparation for a future commit which makes it possible for a PythonPkgConfigDependency to be used in a context where previously only a PythonSystemDependency would be used.
The Python Limited API support that was added in 1.2 had special handling of Windows, but the condition to check for Windows was not correct: it checked for MSVC and not for the target's OS. This causes mingw installations to not have the special handling applied. This commit fixes this to check explicitly for Windows.
This commit fixes GH issue mesonbuild#13167 by linking to the correct library under MINGW when the 'limited_api' kwarg is specified.
amcn
force-pushed
the
mingw-python-limited-api
branch
from
9677704
to
06c45b3
Compare
Link to the correct Python limited API DLL under mingw
This aims to fix #13167