-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
modulegraph: Detect C-extension #4950
base: develop
Are you sure you want to change the base?
Conversation
It first determines if the module is a C-extension, which will return the correct metadata.
* Module in Package * Extension in Package
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.
Thanks for this pull-request. Very interesting to lern this to be (part of) the reason for the recursion-to-dep errors.
tests/unit/test_modulegraph
contains the upstream test-suite, and shall be kept near upstream. Please move your tests totests/unit/test_modulegraph_more.py
, which also leverages the modern pytest style of testing.- Please add a test validating that a module within the package (e.g.
myextpkg/submod.py
) will be found and frozen, too.
Please also submit a changelog entry so our users can learn about your change. There will be three changelog entries:
- 4346.bugfix.rst about recursion error in pydantic
- 4950.bugfix.rst describing this pull-request's change
- 4406.bugfix.rst about partial fix.
Thnx.
@@ -290,6 +295,38 @@ def test_find_module(self): | |||
self.assertEqual(description, (ext, 'rb', imp.C_EXTENSION)) | |||
self.assertEqual(fp, None) | |||
|
|||
# Module in Package | |||
parent = MockNode('mypkg') | |||
info = modgraph._find_module('__init__', path=[path, os.path.join(path, 'mypkg')] + sys.path, parent=parent) |
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.
Please add a comment to this test case, explaining what is tested here and why.
One of the reasons such a comment is required: Running _find_module()
on a module named __init__
is a flaw, caused be the way modulegraph is implemented. Modulegraph v2 should not have this flaw any more and we will need to adopt this test (and need to know how) :-)
When moving the test to tests/unit/test_modulegraph_more.py
please consider creating the modules on the fly (see other test in this file), allowing for me detailed tests. I suggest additional tests, verifying that "importing" the package works, too.
if sys.platform == 'win32': | ||
ext = '.pyd' | ||
else: | ||
# This is a ly, but is good enough for now |
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.
What does this comment mean?
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 do not understand either.
It's same as https://github.com/pyinstaller/pyinstaller/blob/develop/tests/unit/test_modulegraph/test_modulegraph.py#L286
@htgoebel I don't see the point in merging this - I'm going to be replacing modulegraph this version (already started work on it), so I can just make sure it gets done in the move. |
3a8eccd
to
e20e74c
Compare
It first determines if the module's filetype is a C-extension, which will return the correct metadata.
Fixes #4346 (Fixes part of #4406)
pydantic has a file structure like
pydantic/__init__.pyd
.Before this patch,
ModuleGraph._find_module_path()
had detected it asimp.PKG_DIRECTORY
.After fixed, detected it as
imp.C_EXTENSION
.