From e95b15c5d039f7ba04f02dd69d9e8557d8266238 Mon Sep 17 00:00:00 2001 From: Kevin Newton Date: Mon, 20 Jul 2026 13:08:05 -0400 Subject: [PATCH] gh-157230: test_importlib restore builtins Our CI (https://github.com/facebookincubator/cinder/actions/runs/29382592631/job/87249270535) was failing on test_pickle and test_pickletool with: ``` PicklingError: Can't pickle : it's not the same object as importlib._bootstrap.BuiltinImporter ``` I believe this is happening because we run multiple test modules in the same interpreter, so the following sequence happens: * `test_importlib.util.import_importlib()` imports a source copy of `importlib` while blocking `_frozen_importlib`. During this import, `importlib._bootstrap._setup()` initializes import metadata on existing built-in modules. If `builtins.__loader__` or `builtins.__spec__` was originally absent, the source copy installs its own `BuiltinImporter`. * Although `import_fresh_module()` restores `sys.modules`, it does not restore attributes mutated on existing module objects. Consequently, `builtins.__loader__` continues to reference the temporary source `BuiltinImporter`, while `importlib._bootstrap.BuiltinImporter` resolves to the restored frozen class. * Pickle serializes classes by module and qualified name and verifies that the resolved global is the same object. The two `BuiltinImporter` class objects therefore cause the identity check to fail. To fix this we instead snapshot `__loader__` and `__spec__` before importing `importlib` and then restore them to their original values after. --- Lib/test/test_import/__init__.py | 10 +++++-- Lib/test/test_importlib/test_util.py | 16 +++++++++++ Lib/test/test_importlib/util.py | 40 ++++++++++++++++++++++++++-- 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 7e92028cb955cb2..78a536d72012b60 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -1269,8 +1269,14 @@ class Spec: pass spec = Spec() - spec.name = "sys" - self.assertIs(_imp.create_builtin(spec), sys) + # Reloading a core module copies back the module dict snapshot taken + # when it was first initialized, in which __spec__ and __loader__ are + # still None. Swap them back so the rest of the suite sees a sys that + # carries its import metadata. + with (swap_attr(sys, '__spec__', sys.__spec__), + swap_attr(sys, '__loader__', sys.__loader__)): + spec.name = "sys" + self.assertIs(_imp.create_builtin(spec), sys) spec.name = None with self.assertRaisesRegex(TypeError, 'name must be string, not NoneType'): diff --git a/Lib/test/test_importlib/test_util.py b/Lib/test/test_importlib/test_util.py index 8a911c1b751a4b4..e4d4bbad4e806d8 100644 --- a/Lib/test/test_importlib/test_util.py +++ b/Lib/test/test_importlib/test_util.py @@ -33,6 +33,22 @@ _interpreters = None +class ImportImportlibTests(unittest.TestCase): + + def test_restores_missing_import_metadata(self): + # importlib._bootstrap._setup() installs the loader classes of the + # source copy on any module that has no __spec__ of its own. + for name in ('builtins', 'sys'): + module = sys.modules[name] + with self.subTest(module=name): + with (support.swap_attr(module, '__spec__', None), + support.swap_attr(module, '__loader__', None)): + util.import_importlib('importlib') + + self.assertIsNone(module.__spec__) + self.assertIsNone(module.__loader__) + + class DecodeSourceBytesTests: source = "string ='ΓΌ'" diff --git a/Lib/test/test_importlib/util.py b/Lib/test/test_importlib/util.py index 6399f952f9e912b..53a5186268de4e6 100644 --- a/Lib/test/test_importlib/util.py +++ b/Lib/test/test_importlib/util.py @@ -64,12 +64,48 @@ def _extension_details(): _extension_details() +_MISSING = object() + + +@contextlib.contextmanager +def _restore_import_metadata(): + """Keep the source copy of importlib out of other modules' metadata. + + Importing importlib with _frozen_importlib blocked runs + importlib._bootstrap._setup(), which fills in __spec__ and __loader__ on + every builtin and frozen module that lacks them. After the import, this + context restores the original values of those attributes on all modules that + were modified so those modifications don't leak into other tests. + """ + incomplete = {} + for name, module in list(sys.modules.items()): + if isinstance(module, types.ModuleType): + for attr in ('__spec__', '__loader__'): + value = getattr(module, attr, _MISSING) + if value is None or value is _MISSING: + incomplete[name, attr] = value + try: + yield + finally: + for (name, attr), value in incomplete.items(): + module = sys.modules.get(name) + if module is not None and getattr(module, attr, _MISSING) is not value: + if value is _MISSING: + delattr(module, attr) + else: + setattr(module, attr, value) + + def import_importlib(module_name): """Import a module from importlib both w/ and w/o _frozen_importlib.""" fresh = ('importlib',) if '.' in module_name else () frozen = import_helper.import_fresh_module(module_name) - source = import_helper.import_fresh_module(module_name, fresh=fresh, - blocked=('_frozen_importlib', '_frozen_importlib_external')) + with _restore_import_metadata(): + source = import_helper.import_fresh_module( + module_name, + fresh=fresh, + blocked=('_frozen_importlib', '_frozen_importlib_external'), + ) return {'Frozen': frozen, 'Source': source}