Skip to content
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

bpo-37473: Don't import importlib ASAP in tests #14661

Merged
merged 1 commit into from
Jul 14, 2019
Merged

bpo-37473: Don't import importlib ASAP in tests #14661

merged 1 commit into from
Jul 14, 2019

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jul 9, 2019

bpo-15386, bpo-37473: test_import, regrtest and libregrtest no longer
import importlib as soon as possible, as the first import, "to test
bpo-15386".

Sort test_import imports.

https://bugs.python.org/issue37473

bpo-15386, bpo-37473: test_import, regrtest and libregrtest no longer
import importlib as soon as possible, as the first import, "to test
bpo-15386".

Sort test_import imports.
@vstinner
Copy link
Member Author

vstinner commented Jul 9, 2019

@brettcannon, @ncoghlan: Are you ok to remove these hacks?

It seems like https://bugs.python.org/issue15386 is already tested by test_import.test_there_can_be_only_one(). "import imp" was replaced with "import _imp" incommit be7e49f:

commit be7e49fd820318509cd8b4dbde479c552f74ef62
Author: Nick Coghlan <ncoghlan@gmail.com>
Date:   Fri Jul 20 23:40:09 2012 +1000

    Close #15386: There was a loophole that meant importlib.machinery and imp would sometimes reference an uninitialised copy of importlib._bootstrap

At least, I can say that importlib breaks quickly with the following change:

diff --git a/Lib/importlib/__init__.py b/Lib/importlib/__init__.py
index 0c73c505f9..130bc19537 100644
--- a/Lib/importlib/__init__.py
+++ b/Lib/importlib/__init__.py
@@ -9,7 +9,7 @@ __all__ = ['__import__', 'import_module', 'invalidate_caches', 'reload']
 # modules would get an uninitialised copy of the source version, instead
 # of a fully initialised version (either the frozen one or the one
 # initialised below if the frozen one is not available).
-import _imp  # Just the builtin component, NOT the full Python module
+import imp  # Just the builtin component, NOT the full Python module
 import sys
 
 try:

@brettcannon
Copy link
Member

As long as there's another test then I'm fine with the change.

@brettcannon brettcannon added the tests Tests in the Lib/test dir label Jul 9, 2019
Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading through the issues and doing some testing on the latest version, I checked to see if the issue is still occurring:

I haven't worked out how yet, but importlib.machinery is managing to bypass the replacement of importlib._bootstrap with _frozen_importlib:

Since FileFinder was moved from _bootstrap to _bootstrap_external I replaced the reference when I ran through the process in the interpreter:

image

Based upon the outputs, it looks like the issue was solved at some point by having importlib._bootstrap_external.FileFinder and importlib.machinery.FileFinder refer to the same class, _frozen_importlib_external.FileFinder. To ensure this is happening, an assertion such as this could be used:

# ...
self.assertEqual(importlib.machinery.FileFinder, 
importlib._bootstrap_external.FileFinder, 
msg = "Both locations should refer to the same class.")

It might be a bit bandage solution still since it does not explain why the override was occurring in the first place, but at least it no longer happens with FileFinder.

@vstinner vstinner merged commit 8b7db5a into python:master Jul 14, 2019
@vstinner vstinner deleted the remove_importlib_hack branch July 14, 2019 17:31
@vstinner
Copy link
Member Author

I merged your PR. If someone considers than test_there_can_be_only_one() is not enough, please add a new test.

@ncoghlan
Copy link
Contributor

importlib initialisation changed quite a bit when the external import bootstrapping was split out to a separate step, so it's quite plausible that the previous odd behaviour was a result of FileFinder being defined in the core import system bootstrap module.

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
bpo-15386, bpo-37473: test_import, regrtest and libregrtest no longer
import importlib as soon as possible, as the first import, "to test
bpo-15386".

It is tested by test_import.test_there_can_be_only_one().

Sort test_import imports.
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
bpo-15386, bpo-37473: test_import, regrtest and libregrtest no longer
import importlib as soon as possible, as the first import, "to test
bpo-15386".

It is tested by test_import.test_there_can_be_only_one().

Sort test_import imports.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants