Python: Fix limited API under mingw #13171

amcn · 2024-05-02T12:08:49Z

Link to the correct Python limited API DLL under mingw

This aims to fix #13167

amcn · 2024-05-02T13:07:17Z

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).

lpsinger · 2024-05-03T18:53:03Z

I find that this PR fixes the issue: nasa-gcn/hpx#21

amcn · 2024-05-08T17:11:34Z

I've reworked this PR to do two things:

Tests have been updated: a new unittest has been added (for windows only), and the existing limited API test has been extended with an extra section to load the built module.
The original fix has been split into two commits: one to move the code that has to be moved, and a second to implement the fix.

Hopefully this is easier to review now. Feedback very welcome.

eli-schwartz

The previous commit made it possible for a PythonPkgConfigDependency to be used in a context where previously only a PythonSystemDependency would be used.

It sounds like maybe commits 3 and 4 should be flipped around maybe?

eli-schwartz · 2024-05-17T02:56:02Z

test cases/python/9 extmodule limited api/meson.build

+  args: [files('test_limited.py')],
+  env: { 'PYTHONPATH': meson.current_build_dir() },
+  workdir: meson.current_source_dir()
+)


File endings look weird. :P

Oof, I always forget about this. Fix incoming.

eli-schwartz · 2024-05-17T02:58:46Z

unittests/pythontests.py

+        testdir = os.path.join(
+            self.src_root,
+            'test cases',
+            'python',
+            '9 extmodule limited api'
+        )


I don't see a good reason to split this across 6 different lines, we don't do so elsewhere.

Fix incoming.

eli-schwartz · 2024-05-17T03:06:52Z

unittests/pythontests.py

+            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)


Why do we want to look in stderr as well?

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?

amcn · 2024-05-27T09:58:28Z

The previous commit made it possible for a PythonPkgConfigDependency to be used in a context where previously only a PythonSystemDependency would be used.

It sounds like maybe commits 3 and 4 should be flipped around maybe?

Yeah, flipping them around is clearer. I'll push a new series up now with them flipped (and other issues addressed).

jpakkane · 2024-06-10T19:48:06Z

Rebasing against current master should make the build failure go away.

dnicolodi · 2024-06-11T09:25:22Z

test cases/python/9 extmodule limited api/test_limited.py

+from limited import hello
+
+def test_hello():
+    assert hello() == "Hello, world"


What's the point of the assert if it is not executed in the test? It is no executed because the test function returns hello world not Hello, world and the test would fail. If the point is to check that from limited import hello works, there is no need to define the test_hello() function. At that point, you can make the test definition in meson.build simply execute python -m limited.hello.

Good catch, you're very right this doesn't work at all how it should. When I was originally implementing this, it was sufficient to simply load the built module via from limited import hello to check whether or not the module was built correctly or not.

I can't remember why I added the test_hello part, but you're absolutely right that it doesn't work as it should. I will implement your suggestion.

So, I'm implementing what you suggested and in the process have learned the python -m foo.bar does not work for modules that are implemented as C extensions. This was news to me, so instead what I propose to do instead is to simply fix my original broken test such that test_hello is actually called, and that it actually checks for the right value. Does that sound ok?

That would have been my suggestion anyway, yeah.

dnicolodi · 2024-06-11T09:29:12Z

mesonbuild/dependencies/python.py

        self.link_args = largs
        self.is_found = True

+class PythonPkgConfigDependency(PkgConfigDependency, _PythonDependencyBase):


I may be missing something, but the only thing this patch does is to move the code around unmodified in the same file. What's the point? Why does the code need to be moved?

The change to the Windows checking logic introduced in 663700c means that a PythonPkgConfigDependency might now be found where previously only a PythonSystemDependency was found.

The find_libpy_windows function previously only existed on PythonSystemDependency.

Moving this function (and all the functions it calls transitively) into the base class seemed like the easiest solution to the AttributeErrors that I was seeing during the initial development of this patch. It is a bit brutal though, I admit.

The change in 663700c is not enough to resolve the initial issue that this PR addresses. Ultimately the change in b26f306 is necessary in order to find the right library under mingw.

The diff is deceptive, since a giant block of code gets moved around but it looks like a bunch of classes got moved around some functions, but what actually happened is some functions got moved from one class to another.

It's not easy to tell that the moved code starts halfway through a class definition (thus indicating that the semantics changed).

I had to stare at it some more to get it. It is deceptive indeed that the moved functions are the ones not appearing in the diff. Now I get it.

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.

dnicolodi · 2024-06-11T18:42:42Z

test cases/python/9 extmodule limited api/test_limited.py

+def test_hello():
+    assert hello() == "hello world"
+
+test_hello()


This can be condensed to be just

from limited import hello assert hello() == 'hello world'

astro-stan · 2024-07-01T18:35:48Z

Hi all,

sorry for posting on a closed PR.

I am using 1.15-rc1 (I believe this patch made it in).

But I still have the same issue. I am Running CIBW on GH actions. I am trying to build ABI3 wheel for 3.7+. My build-system deps are:

[build-system]
requires = [
  "meson-python>=0.16.0",
  'meson>=1.5.0rc1',
  'ninja',
  "cffi>=1.15.1; python_version<'3.8'",
  "cffi>=1.16; python_version>='3.8'",
  "setuptools; python_version>='3.12'",
]

Any ideas?

amcn · 2024-07-01T22:37:14Z

Hi all,

sorry for posting on a closed PR.

No worries. It certainly sounds like your issue is relevant to this PR so thanks for bringing this to my attention.

Any ideas?

Would you be able to run the following command:

$ objdump -p module.pyd | grep python3

on the built module and let me know what it reports?

lpsinger · 2024-07-02T02:11:45Z

I haven't investigated why yet, but the recent release candidate broke my Windows build: https://github.com/nasa-gcn/hpx/actions/runs/9753713026/job/26919424832?pr=21

eli-schwartz · 2024-07-02T02:53:09Z

E ImportError: DLL load failed while importing _core: The specified module could not be found.

That's going to be a pain to debug lol.

Something that may help is using meson compile -v for the compilation step to see the exact compiler invocations and compare the working one to the broken one.

lpsinger · 2024-07-02T04:51:14Z

E ImportError: DLL load failed while importing _core: The specified module could not be found.

That's going to be a pain to debug lol.

Something that may help is using meson compile -v for the compilation step to see the exact compiler invocations and compare the working one to the broken one.

It was easier to debug than I expected: the tests for cp310-win_amd64 are failing because the abi3 wheel is still linked against python39.dll instead of python3.dll. So this PR didn't fix it.

astro-stan · 2024-07-02T08:20:23Z

Would you be able to run the following command:

$ objdump -p module.pyd | grep python3

on the built module and let me know what it reports?

I haven't done that but I compiled with the -v flag. I can confirm it is getting linked to the python37.dll, rather than to the python3.dll:

...
[8/8] "gcc"  -o _project.pyd _project.pyd.p/meson-generated_..__project.c.obj "-Wl,--allow-shlib-undefined" "-Wl,-O1" "-shared" "-Wl,--start-group" "-Wl,--out-implib=_project.dll.a" "vendor/projectLib/projectLib/build/projectLib.a" "C:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\nuget-cpython\python.3.7.9\tools\python37.dll" "-lkernel32" "-luser32" "-lgdi32" "-lwinspool" "-lshell32" "-lole32" "-loleaut32" "-luuid" "-lcomdlg32" "-ladvapi32" "-Wl,--end-group"

 INFO: autodetecting backend as ninja
  INFO: calculating backend command to run: C:\Users\runneradmin\AppData\Local\Temp\build-env-b61dgxh_\Scripts\ninja.EXE -v
  [1/8] D:\a\project\project\.mesonpy-vs_apush\_project.pyd
  ...
  Successfully built project-0.1.0-cp37-abi3-win_amd64.whl

amcn · 2024-07-02T20:49:55Z

I think I see the problem. I'll post a fix and open a new PR.

amcn · 2024-07-02T21:06:35Z

I've opened a PR for the potential fix: #13379

cclauss · 2024-07-27T16:02:42Z

Is this fixed in mason v1.5.1? https://github.com/mesonbuild/meson/releases

Automates on an ongoing basis the Actions upgrades featured in * mesonbuild#13171 Fixes software supply chain safety warnings like at the bottom right of https://github.com/mesonbuild/meson/actions/runs/10125136437 * [Keeping your actions up to date with Dependabot](https://docs.github.com/en/code-security/dependabot/working-with-dependabot/keeping-your-actions-up-to-date-with-dependabot) * [Configuration options for the dependabot.yml file - package-ecosystem](https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#package-ecosystem)

eli-schwartz · 2024-07-28T04:35:09Z

Is this fixed in mason v1.5.1? https://github.com/mesonbuild/meson/releases

It is in 1.5.0 as well.

amcn requested a review from jpakkane as a code owner May 2, 2024 12:08

amcn force-pushed the mingw-python-limited-api branch from fd9c8b0 to 155a3e9 Compare May 8, 2024 17:08

amcn force-pushed the mingw-python-limited-api branch from 155a3e9 to 2a7aeb3 Compare May 8, 2024 20:18

eli-schwartz reviewed May 17, 2024

View reviewed changes

amcn force-pushed the mingw-python-limited-api branch 2 times, most recently from 9677704 to 06c45b3 Compare May 27, 2024 11:33

amcn force-pushed the mingw-python-limited-api branch from 06c45b3 to b26f306 Compare June 11, 2024 08:24

dnicolodi suggested changes Jun 11, 2024

View reviewed changes

amcn added 5 commits June 11, 2024 19:47

Python: add load test to limited API test

d1abdce

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.

unittests: Add Python unittest for limited API

4023bbf

This new unittest runs the existing Python limited API test case, and checks that the resulting library was linked to the correct library.

Python: move Windows functions to dependency base

f66a527

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.

Python: link correct limited API lib on mingw

328011f

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 b26f306 to 328011f Compare June 11, 2024 17:48

dnicolodi reviewed Jun 11, 2024

View reviewed changes

jpakkane merged commit 0352c90 into mesonbuild:master Jun 11, 2024

astro-stan mentioned this pull request Jul 1, 2024

Is there a way to programmatically tell if CFFI is using the limited API? python-cffi/cffi#88

Closed

amcn mentioned this pull request Jul 2, 2024

Python: fix limited API logic under GCC on Windows #13379

Merged

cclauss mentioned this pull request Jul 27, 2024

Keep GitHub Actions up to date with GitHub's Dependabot #13479

Closed

Uh oh!

Python: Fix limited API under mingw #13171

Python: Fix limited API under mingw #13171

Uh oh!

Conversation

amcn commented May 2, 2024

Uh oh!

amcn commented May 2, 2024

Uh oh!

lpsinger commented May 3, 2024

Uh oh!

amcn commented May 8, 2024

Uh oh!

eli-schwartz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcn commented May 27, 2024

Uh oh!

jpakkane commented Jun 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astro-stan commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amcn commented Jul 1, 2024

Uh oh!

lpsinger commented Jul 2, 2024

Uh oh!

eli-schwartz commented Jul 2, 2024

Uh oh!

lpsinger commented Jul 2, 2024

Uh oh!

astro-stan commented Jul 2, 2024

Uh oh!

amcn commented Jul 2, 2024

Uh oh!

amcn commented Jul 2, 2024

Uh oh!

cclauss commented Jul 27, 2024

Uh oh!

eli-schwartz commented Jul 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

astro-stan commented Jul 1, 2024 •

edited

Loading