modulegraph: Use loaders to get module content. #5157
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
htgoebel
force-pushed
the
issue-4158-pydantic
branch
from
a655150
to
a59e45b
Compare
|
Tried running this against a simple script containing just one line: Command and output |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@agronholm Please give the new code a try, which should now fix the issue. |
htgoebel
force-pushed
the
issue-4158-pydantic
branch
2 times, most recently
from
61344fb
to
1adbe4f
Compare
|
Seems to work. I don't get any errors anymore. Was the hook a necessary part of this fix somehow? |
Legorooj
reviewed
Sep 13, 2020
|
@agronholm The hook is not part of the fix, but the hook is required for freezing pydantic. |
htgoebel
force-pushed
the
issue-4158-pydantic
branch
from
1adbe4f
to
498aa35
Compare
While working on the following commits, I stepped over add_to_suffix__extension() returning wrong results. Obviously unit-tests have been missing.
These tests check for "recursion too deep" caused by the `__init__` module being an extension module. See pyinstaller#5131, pyinstaller#4346.
htgoebel
force-pushed
the
issue-4158-pydantic
branch
from
498aa35
to
b38c5d9
Compare
modulegraph's heritage includes code still based on Python 2.x capabilities on how to find a module and get its source or code. It also contained a anomaly regarding packages and their ``__init__``-file: If a package was detected, it's ``__init__`` file was loaded as a module. This, while being ugly, worked in most cases, but failed if the ``__init__`` module is an extension module (see pyinstaller#5131, pyinstaller#4346), ending in an infinite loop. This was caused by modulegraph distinguishing between the package and its ``__init__`` module. The solution is to switch to "modern" loaders, both being a loader for a specific type of modules (source, extension, etc.) and having a package characteristic (property ``is_package()``) This commit does the following - In ``_find_module_path()`` no longer return "metadata" but a loader. This also removed huge part of this function, making it much easier to understand. As a side effect, this asymmetric closing of a file in a completely other part of the code (``_safe_import_module``) could be removed. - Change ``_load_module`` to use the loaders. - Merge ``_load_package`` into `__load_module``, getting rid of the anomaly described above. - Adjust the test-cases to the new behavior (esp. loader instead of metadata-tuple and filehandle) Please note: Since we plan to change to modulegraph2 soon anyway, I did not spend too much time on creating a clean solution. See pyinstaller#4406, closes pyinstaller#5131, pyinstaller#4346.
While the latest changes fixed modulegraph to not go into an endless recursion if a package's `__init__` is an extension module, there still was an issue: The returned node was of type ``Package``, leading to ``PYZ()`` trying to load and compile the source. Since the file is an extension module, compiling fails, of course. This change adds a test-case for this.
Introduce a new node type ExtensionPackage, which signals that a package's `__init__` is an extension module. While the latest changes fixed modulegraph to not go into an endless recursion if a package's `__init__` is an extension module, there still was an issue: The returned node was of type ``Package``, leading to ``PYZ()`` trying to load and compile the source (to get "missing" the code-object). Since the file is an extension module, compiling failed, of course.
This maps the modulegraph nodes of type ExtensionPackage to simple extensions for the sake of the TOC.
Add modulegraph's new node type ExtensionPackage.
Converting modulegraph Nodes into TOC entries was done twice. The code was the same, except that one implementation did not filter node types and had less comments.
When converting the nodes to TOC entries, also change the filename for ExtensionPackage: Actually convert the packages name (``mypkg``) into the module name (``mypkg.__init__``).
htgoebel
force-pushed
the
issue-4158-pydantic
branch
from
b38c5d9
to
49002f3
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
modulegraph's heritage includes code still based on Python 2.x capabilities on
how to find a module and get its source or code. It also contained a anomaly
regarding packages and their
__init__-file: If a package was detected,it's
__init__file was loaded as a module. This, while being ugly, workedin most cases, but failed if the
__init__module is an extensionmodule (see #5131, #4346), ending in an infinite loop. This was caused by
modulegraph distinguishing between the package and its
__init__module.The solution is to switch to "modern" loaders, both being a loader for a
specific type of modules (source, extension, etc.) and having a package
characteristic (property
is_package())This commit does the following
In
_find_module_path()no longer return "metadata" but a loader. Thisalso removed huge part of this function, making it much easier to understand.
As a side effect, this asymmetric closing of a file in a completely other
part of the code (
_safe_import_module) could be removed.Change
_load_moduleto use the loaders.Merge
_load_packageinto `__load_module``, getting rid of the anomalydescribed above.
Adjust the test-cases to the new behavior (esp. loader instead of
metadata-tuple and filehandle)
Please note: Since we plan to change to modulegraph2 soon anyway, I did not
spend too much time on creating a clean solution.
See #4406, closes #5131, #4346.