-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Python: Fix limited API under mingw #13171
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
Changes from all commits
d1abdce
4023bbf
f66a527
fea7f94
328011f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| from limited import hello | ||
|
|
||
| def test_hello(): | ||
| assert hello() == "hello world" | ||
|
|
||
| test_hello() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be condensed to be just from limited import hello
assert hello() == 'hello world' |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ | |
| from .baseplatformtests import BasePlatformTests | ||
| from .helpers import * | ||
|
|
||
| from mesonbuild.mesonlib import MachineChoice, TemporaryDirectoryWinProof | ||
| from mesonbuild.mesonlib import MachineChoice, TemporaryDirectoryWinProof, is_windows | ||
| from mesonbuild.modules.python import PythonModule | ||
|
|
||
| class PythonTests(BasePlatformTests): | ||
|
|
@@ -86,3 +86,32 @@ def test_bytecompile_single(self): | |
| if shutil.which('python2') or PythonModule._get_win_pythonpath('python2'): | ||
| raise self.skipTest('python2 installed, already tested') | ||
| self._test_bytecompile() | ||
|
|
||
| def test_limited_api_linked_correct_lib(self): | ||
| if not is_windows(): | ||
| return self.skipTest('Test only run on Windows.') | ||
|
|
||
| testdir = os.path.join(self.src_root, 'test cases', 'python', '9 extmodule limited api') | ||
|
|
||
| self.init(testdir) | ||
| self.build() | ||
|
|
||
| from importlib.machinery import EXTENSION_SUFFIXES | ||
| limited_suffix = EXTENSION_SUFFIXES[1] | ||
|
|
||
| limited_library_path = os.path.join(self.builddir, f'limited{limited_suffix}') | ||
| self.assertPathExists(limited_library_path) | ||
|
|
||
| limited_dep_name = 'python3.dll' | ||
| if shutil.which('dumpbin'): | ||
| # MSVC | ||
| 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) | ||
|
Comment on lines
+108
to
+114
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we want to look in stderr as well?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| self.assertIn(limited_dep_name, output.decode()) | ||
| else: | ||
| raise self.skipTest('Test needs either dumpbin(MSVC) or objdump(mingw).') | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change to the Windows checking logic introduced in 663700c means that a
PythonPkgConfigDependencymight now be found where previously only aPythonSystemDependencywas found.The
find_libpy_windowsfunction previously only existed onPythonSystemDependency.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.