From 5c538f5da7e97ead989d55490753f42f0b887634 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 28 Sep 2021 18:15:30 -0600 Subject: [PATCH 01/18] Set __file__ on frozen modules if possible. --- Lib/importlib/_bootstrap.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index 49f08814e95186..6c1e83366ee31e 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -850,13 +850,19 @@ def find_spec(cls, fullname, path=None, target=None): if info is None: return None data, ispkg, origname = info + filename = pkgdir = None spec = spec_from_loader(fullname, cls, origin=cls._ORIGIN, is_package=ispkg) spec.loader_state = type(sys.implementation)( data=data, + filename=filename, origname=origname, ) + if ispkg and filename: + assert filename.endswith('__init__.py'), filename + assert pkgdir, pkgdir + spec.submodule_search_locations.insert(0, pkgdir) return spec @classmethod @@ -873,7 +879,16 @@ def find_module(cls, fullname, path=None): @staticmethod def create_module(spec): - """Use default semantics for module creation.""" + """Set __file__, if able.""" + module = _new_module(spec.name) + try: + filename = spec.loader_state.filename + except AttributeError: + pass + else: + if filename: + module.__file__ = filename + return module @staticmethod def exec_module(module): @@ -905,6 +920,9 @@ def load_module(cls, fullname): # Warning about deprecation implemented in _load_module_shim(). return _load_module_shim(cls, fullname) + # XXX We should also add get_filename(), but when we do we need + # to fix spec_from_loader(). + @classmethod @_requires_frozen def get_code(cls, fullname): From 62e3c1649fbb096999120b685a5af41c9863c4f4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 29 Sep 2021 16:26:48 -0600 Subject: [PATCH 02/18] Set __file__ based on "origname". --- Lib/importlib/_bootstrap.py | 28 ++++++++++++++----- Lib/test/test_importlib/frozen/test_finder.py | 13 ++++++--- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index 6c1e83366ee31e..ea1a762da1c471 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -826,11 +826,9 @@ def module_repr(m): @classmethod def _setup_module(cls, module): - assert not hasattr(module, '__file__'), module.__file__ ispkg = hasattr(module, '__path__') - assert not ispkg or not module.__path__, module.__path__ spec = module.__spec__ - assert not ispkg or not spec.submodule_search_locations + assert not ispkg or spec.submodule_search_locations is not None if spec.loader_state is None: spec.loader_state = type(sys.implementation)( @@ -844,24 +842,40 @@ def _setup_module(cls, module): assert origname, 'see PyImport_ImportFrozenModuleObject()' spec.loader_state.origname = origname + @classmethod + def _resolve_filename(cls, fullname, ispkg): + if not fullname or not getattr(sys, '_stdlib_dir', None): + return None, None + try: + sep = cls._SEP + except AttributeError: + sep = cls._SEP = '\\' if sys.platform == 'win32' else '/' + + relfile = fullname.replace('.', sep) + if ispkg: + pkgdir = f'{sys._stdlib_dir}{sep}{relfile}' + filename = f'{pkgdir}{sep}__init__.py' + else: + pkgdir = None + filename = f'{sys._stdlib_dir}{sep}{relfile}.py' + return filename, pkgdir + @classmethod def find_spec(cls, fullname, path=None, target=None): info = _call_with_frames_removed(_imp.find_frozen, fullname) if info is None: return None data, ispkg, origname = info - filename = pkgdir = None spec = spec_from_loader(fullname, cls, origin=cls._ORIGIN, is_package=ispkg) + filename, pkgdir = cls._resolve_filename(origname, ispkg) spec.loader_state = type(sys.implementation)( data=data, filename=filename, origname=origname, ) - if ispkg and filename: - assert filename.endswith('__init__.py'), filename - assert pkgdir, pkgdir + if pkgdir: spec.submodule_search_locations.insert(0, pkgdir) return spec diff --git a/Lib/test/test_importlib/frozen/test_finder.py b/Lib/test/test_importlib/frozen/test_finder.py index cd5586d524ce2c..8fbf72aa1e7008 100644 --- a/Lib/test/test_importlib/frozen/test_finder.py +++ b/Lib/test/test_importlib/frozen/test_finder.py @@ -44,6 +44,7 @@ def check_loader_state(self, spec, origname=None, filename=None): if not filename: if not origname: origname = spec.name + filename = resolve_stdlib_file(origname) actual = dict(vars(spec.loader_state)) @@ -61,13 +62,17 @@ def check_loader_state(self, spec, origname=None, filename=None): # Check the rest of spec.loader_state. expected = dict( origname=origname, + filename=filename, ) self.assertDictEqual(actual, expected) - def check_search_locations(self, spec): - # Frozen packages do not have any path entries. - # (See https://bugs.python.org/issue21736.) - expected = [] + def check_search_location(self, spec): + missing = object() + filename = getattr(spec.loader_state, 'filename', missing) + if filename is missing: + # We deal with this in check_loader_state(). + return + expected = [os.path.dirname(filename)] if filename else [] self.assertListEqual(spec.submodule_search_locations, expected) def test_module(self): From 6aaba14f120221b82c5323add867c64eeb88e90a Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 1 Oct 2021 15:55:47 -0600 Subject: [PATCH 03/18] Fix it --- Lib/importlib/_bootstrap.py | 11 +++++++++-- Lib/test/test_frozen.py | 3 --- Lib/test/test_importlib/frozen/test_finder.py | 15 +++++++++++---- Lib/test/test_importlib/frozen/test_loader.py | 12 +++++++++++- 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index ea1a762da1c471..c64198df4af0af 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -843,7 +843,7 @@ def _setup_module(cls, module): spec.loader_state.origname = origname @classmethod - def _resolve_filename(cls, fullname, ispkg): + def _resolve_filename(cls, fullname, alias=None, ispkg=False): if not fullname or not getattr(sys, '_stdlib_dir', None): return None, None try: @@ -851,6 +851,13 @@ def _resolve_filename(cls, fullname, ispkg): except AttributeError: sep = cls._SEP = '\\' if sys.platform == 'win32' else '/' + if fullname != alias: + if fullname.startswith('<'): + fullname = fullname[1:] + if not ispkg: + fullname = f'{fullname}.__init__' + else: + ispkg = False relfile = fullname.replace('.', sep) if ispkg: pkgdir = f'{sys._stdlib_dir}{sep}{relfile}' @@ -869,7 +876,7 @@ def find_spec(cls, fullname, path=None, target=None): spec = spec_from_loader(fullname, cls, origin=cls._ORIGIN, is_package=ispkg) - filename, pkgdir = cls._resolve_filename(origname, ispkg) + filename, pkgdir = cls._resolve_filename(origname, fullname, ispkg) spec.loader_state = type(sys.implementation)( data=data, filename=filename, diff --git a/Lib/test/test_frozen.py b/Lib/test/test_frozen.py index 029fd068793c30..0b4a12bcf40948 100644 --- a/Lib/test/test_frozen.py +++ b/Lib/test/test_frozen.py @@ -39,9 +39,6 @@ def test_frozen_submodule_in_unfrozen_package(self): self.assertIs(spam.__spec__.loader, importlib.machinery.FrozenImporter) - # This is not possible until frozen packages have __path__ set properly. - # See https://bugs.python.org/issue21736. - @unittest.expectedFailure def test_unfrozen_submodule_in_frozen_package(self): with import_helper.CleanImport('__phello__', '__phello__.spam'): with import_helper.frozen_modules(enabled=True): diff --git a/Lib/test/test_importlib/frozen/test_finder.py b/Lib/test/test_importlib/frozen/test_finder.py index 8fbf72aa1e7008..654fafcb58a57e 100644 --- a/Lib/test/test_importlib/frozen/test_finder.py +++ b/Lib/test/test_importlib/frozen/test_finder.py @@ -62,17 +62,24 @@ def check_loader_state(self, spec, origname=None, filename=None): # Check the rest of spec.loader_state. expected = dict( origname=origname, - filename=filename, + filename=filename if origname else None, ) self.assertDictEqual(actual, expected) - def check_search_location(self, spec): + def check_search_locations(self, spec): + """This is only called when testing packages.""" missing = object() filename = getattr(spec.loader_state, 'filename', missing) - if filename is missing: + origname = getattr(spec.loader_state, 'origname', None) + if not origname or filename is missing: # We deal with this in check_loader_state(). return - expected = [os.path.dirname(filename)] if filename else [] + if not filename: + expected = [] + elif origname != spec.name and not origname.startswith('<'): + expected = [] + else: + expected = [os.path.dirname(filename)] self.assertListEqual(spec.submodule_search_locations, expected) def test_module(self): diff --git a/Lib/test/test_importlib/frozen/test_loader.py b/Lib/test/test_importlib/frozen/test_loader.py index d6f39fa98a6f1b..2d476612600ba2 100644 --- a/Lib/test/test_importlib/frozen/test_loader.py +++ b/Lib/test/test_importlib/frozen/test_loader.py @@ -3,10 +3,11 @@ machinery = util.import_importlib('importlib.machinery') -from test.support import captured_stdout, import_helper +from test.support import captured_stdout, import_helper, STDLIB_DIR import _imp import contextlib import marshal +import os.path import types import unittest import warnings @@ -30,6 +31,14 @@ def fresh(name, *, oldapi=False): yield +def resolve_stdlib_file(name, ispkg=False): + assert name + if ispkg: + return os.path.join(STDLIB_DIR, *name.split('.'), '__init__.py') + else: + return os.path.join(STDLIB_DIR, *name.split('.')) + '.py' + + class ExecModuleTests(abc.LoaderTests): def exec_module(self, name, origname=None): @@ -44,6 +53,7 @@ def exec_module(self, name, origname=None): loader_state=types.SimpleNamespace( data=marshal.dumps(code), origname=origname or name, + filename=resolve_stdlib_file(origname or name, is_package), ), ) module = types.ModuleType(name) From f4d6db68f9fde403bab22d5e8cc93097fd1897ed Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 29 Sep 2021 18:01:22 -0600 Subject: [PATCH 04/18] Fix up early-imported frozen modules. --- Lib/importlib/_bootstrap.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index c64198df4af0af..9cb50208cbdf98 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -841,6 +841,16 @@ def _setup_module(cls, module): origname = vars(module).pop('__origname__', None) assert origname, 'see PyImport_ImportFrozenModuleObject()' spec.loader_state.origname = origname + if not getattr(spec.loader_state, 'filename', None): + # Note that this happens early in runtime initialization. + # So sys._stdlib_dir isn't set yet... + filename, pkgdir = cls._resolve_filename(origname, ispkg) + if filename: + module.__file__ = filename + if pkgdir: + spec.submodule_search_locations.insert(0, pkgdir) + module.__path__.insert(0, pkgdir) + spec.loader_state.filename = filename or None @classmethod def _resolve_filename(cls, fullname, alias=None, ispkg=False): From d8e19e0a0ddffbcc862c004d0c4875e02e4f8ba1 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 1 Oct 2021 09:21:19 -0600 Subject: [PATCH 05/18] Add a NEWS entry. --- .../2021-10-01-09-21-02.bpo-21736.RI47BU.rst | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2021-10-01-09-21-02.bpo-21736.RI47BU.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2021-10-01-09-21-02.bpo-21736.RI47BU.rst b/Misc/NEWS.d/next/Core and Builtins/2021-10-01-09-21-02.bpo-21736.RI47BU.rst new file mode 100644 index 00000000000000..8396a49c3cf736 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2021-10-01-09-21-02.bpo-21736.RI47BU.rst @@ -0,0 +1,9 @@ +Frozen stdlib modules now have ``__file__`` to the .py file they would +otherwise be loaded from, if possible. For packages, ``__path__`` now has +the correct entry instead of being an empty list, which allows unfrozen +submodules to be imported. These are set only if the stdlib directory is +known when the runtime is initialized. Note that the file at ``__file__`` +is not guaranteed to exist. None of this affects non-stdlib frozen modules +nor, for now, frozen modules imported using +``PyImport_ImportFrozenModule()``. Also, at the moment ``co_filename`` is +not updated for the module. From ac5b7c10c4b04543e3c0396ce2586a3e843755b2 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 5 Oct 2021 13:48:36 -0600 Subject: [PATCH 06/18] Drop an unncessary comment. --- Lib/importlib/_bootstrap.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index 9cb50208cbdf98..0f69a76bfefb22 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -951,9 +951,6 @@ def load_module(cls, fullname): # Warning about deprecation implemented in _load_module_shim(). return _load_module_shim(cls, fullname) - # XXX We should also add get_filename(), but when we do we need - # to fix spec_from_loader(). - @classmethod @_requires_frozen def get_code(cls, fullname): From 79c10353e216b46bfa9bd0407c61a60073806225 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 5 Oct 2021 17:21:58 -0600 Subject: [PATCH 07/18] Make sure frozen modules are fixed up properly. --- Lib/importlib/_bootstrap.py | 104 +++++++++++++----- Lib/test/test_importlib/frozen/test_loader.py | 16 ++- 2 files changed, 84 insertions(+), 36 deletions(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index 0f69a76bfefb22..fbf43b0d7813a9 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -421,7 +421,10 @@ def has_location(self, value): def spec_from_loader(name, loader, *, origin=None, is_package=None): """Return a module spec based on various loader methods.""" - if hasattr(loader, 'get_filename'): + if origin is None: + origin = getattr(loader, '_ORIGIN', None) + + if not origin and hasattr(loader, 'get_filename'): if _bootstrap_external is None: raise NotImplementedError spec_from_file_location = _bootstrap_external.spec_from_file_location @@ -467,12 +470,9 @@ def _spec_from_module(module, loader=None, origin=None): except AttributeError: location = None if origin is None: - if location is None: - try: - origin = loader._ORIGIN - except AttributeError: - origin = None - else: + if loader is not None: + origin = getattr(loader, '_ORIGIN', None) + if not origin and location is not None: origin = location try: cached = module.__cached__ @@ -484,7 +484,7 @@ def _spec_from_module(module, loader=None, origin=None): submodule_search_locations = None spec = ModuleSpec(name, loader, origin=origin) - spec._set_fileattr = False if location is None else True + spec._set_fileattr = origin == location spec.cached = cached spec.submodule_search_locations = submodule_search_locations return spec @@ -825,32 +825,67 @@ def module_repr(m): return ''.format(m.__name__, FrozenImporter._ORIGIN) @classmethod - def _setup_module(cls, module): - ispkg = hasattr(module, '__path__') + def _fix_up_module(cls, module): spec = module.__spec__ - assert not ispkg or spec.submodule_search_locations is not None + state = spec.loader_state + if state is None: + # The module is missing FrozenImporter-specific values. - if spec.loader_state is None: - spec.loader_state = type(sys.implementation)( - data=None, - origname=None, - ) - elif not hasattr(spec.loader_state, 'data'): - spec.loader_state.data = None - if not getattr(spec.loader_state, 'origname', None): + # Fix up the spec attrs. origname = vars(module).pop('__origname__', None) assert origname, 'see PyImport_ImportFrozenModuleObject()' - spec.loader_state.origname = origname - if not getattr(spec.loader_state, 'filename', None): - # Note that this happens early in runtime initialization. - # So sys._stdlib_dir isn't set yet... - filename, pkgdir = cls._resolve_filename(origname, ispkg) - if filename: - module.__file__ = filename + ispkg = hasattr(module, '__path__') + assert _imp.is_frozen_package(module.__name__) == ispkg, ispkg + filename, pkgdir = cls._resolve_filename(origname, spec.name, ispkg) + spec.loader_state = state = type(sys.implementation)( + data=None, + filename=filename, + origname=origname, + ) + __path__ = spec.submodule_search_locations + if ispkg: + assert __path__ == [], __path__ if pkgdir: spec.submodule_search_locations.insert(0, pkgdir) - module.__path__.insert(0, pkgdir) - spec.loader_state.filename = filename or None + else: + assert __path__ is None, __path__ + + # Fix up the module attrs (the bare minimum). + assert not hasattr(module, '__file__'), module.__file__ + if filename: + try: + module.__file__ = filename + except AttributeError: + pass + if ispkg: + if module.__path__ != __path__: + assert not module.__path__, module.__path__ + # XXX _init_module_attrs() should copy like this too. + module.__path__.extend(__path__) + else: + # These checks ensure that _fix_up_module() is only called + # in the right places. + assert state is not None + assert sorted(vars(state)) == ['data', 'filename', 'origname'], state + assert state.data is None, state.data + assert state.origname + __path__ = spec.submodule_search_locations + ispkg = __path__ is not None + (filename, pkgdir, + ) = cls._resolve_filename(state.origname, spec.name, ispkg) + assert state.filename == filename, (state.filename, filename) + if pkgdir: + assert __path__ == [pkgdir], (__path__, pkgdir) + elif ispkg: + assert __path__ == [], __path__ + assert hasattr(module, '__file__') + assert module.__file__ == filename, (module.__file__, filename) + if ispkg: + assert hasattr(module, '__path__') + assert module.__path__ == __path__, (module.__path__, __path__) + else: + assert not hasattr(module, '__path__'), module.__path__ + assert not spec.has_location @classmethod def _resolve_filename(cls, fullname, alias=None, ispkg=False): @@ -949,7 +984,16 @@ def load_module(cls, fullname): """ # Warning about deprecation implemented in _load_module_shim(). - return _load_module_shim(cls, fullname) + module = _load_module_shim(cls, fullname) + info = _imp.find_frozen(fullname) + assert info is not None + _, ispkg, origname = info + module.__origname__ = origname + vars(module).pop('__file__', None) + if ispkg: + module.__path__ = [] + cls._fix_up_module(module) + return module @classmethod @_requires_frozen @@ -1290,7 +1334,7 @@ def _setup(sys_module, _imp_module): spec = _spec_from_module(module, loader) _init_module_attrs(spec, module) if loader is FrozenImporter: - loader._setup_module(module) + loader._fix_up_module(module) # Directly load built-in modules needed during bootstrap. self_module = sys.modules[__name__] diff --git a/Lib/test/test_importlib/frozen/test_loader.py b/Lib/test/test_importlib/frozen/test_loader.py index 2d476612600ba2..1e6eebf6880245 100644 --- a/Lib/test/test_importlib/frozen/test_loader.py +++ b/Lib/test/test_importlib/frozen/test_loader.py @@ -149,36 +149,41 @@ def load_module(self, name): def test_module(self): module, stdout = self.load_module('__hello__') + filename = resolve_stdlib_file('__hello__') check = {'__name__': '__hello__', '__package__': '', '__loader__': self.machinery.FrozenImporter, + '__file__': filename, } for attr, value in check.items(): - self.assertEqual(getattr(module, attr), value) + self.assertEqual(getattr(module, attr, None), value) self.assertEqual(stdout.getvalue(), 'Hello world!\n') - self.assertFalse(hasattr(module, '__file__')) def test_package(self): module, stdout = self.load_module('__phello__') + filename = resolve_stdlib_file('__phello__', ispkg=True) + pkgdir = os.path.dirname(filename) check = {'__name__': '__phello__', '__package__': '__phello__', - '__path__': [], + '__path__': [pkgdir], '__loader__': self.machinery.FrozenImporter, + '__file__': filename, } for attr, value in check.items(): - attr_value = getattr(module, attr) + attr_value = getattr(module, attr, None) self.assertEqual(attr_value, value, "for __phello__.%s, %r != %r" % (attr, attr_value, value)) self.assertEqual(stdout.getvalue(), 'Hello world!\n') - self.assertFalse(hasattr(module, '__file__')) def test_lacking_parent(self): with util.uncache('__phello__'): module, stdout = self.load_module('__phello__.spam') + filename = resolve_stdlib_file('__phello__.spam') check = {'__name__': '__phello__.spam', '__package__': '__phello__', '__loader__': self.machinery.FrozenImporter, + '__file__': filename, } for attr, value in check.items(): attr_value = getattr(module, attr) @@ -186,7 +191,6 @@ def test_lacking_parent(self): "for __phello__.spam.%s, %r != %r" % (attr, attr_value, value)) self.assertEqual(stdout.getvalue(), 'Hello world!\n') - self.assertFalse(hasattr(module, '__file__')) def test_module_reuse(self): with fresh('__hello__', oldapi=True): From 322cfd1676b2f05aade2b1d29b5bf2c5c027cd00 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 12 Oct 2021 17:58:47 -0600 Subject: [PATCH 08/18] Return a memoryview from _imp.find_frozen(). --- Python/import.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/Python/import.c b/Python/import.c index a6170a39c7fdbd..5b68bc6277eb5b 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2079,7 +2079,8 @@ _imp_find_frozen_impl(PyObject *module, PyObject *name) return NULL; } - PyObject *data = PyBytes_FromStringAndSize(info.data, info.size); + PyObject *data = PyMemoryView_FromMemory((char *)info.data, info.size, + PyBUF_READ); if (data == NULL) { return NULL; } @@ -2116,15 +2117,15 @@ _imp_get_frozen_object_impl(PyObject *module, PyObject *name, PyObject *dataobj) /*[clinic end generated code: output=54368a673a35e745 input=034bdb88f6460b7b]*/ { - struct frozen_info info; + struct frozen_info info = {}; if (PyBytes_Check(dataobj)) { - info.nameobj = name; info.data = PyBytes_AS_STRING(dataobj); info.size = PyBytes_Size(dataobj); - if (info.size == 0) { - set_frozen_error(FROZEN_INVALID, name); - return NULL; - } + } + else if (PyMemoryView_Check(dataobj)) { + Py_buffer *buf = PyMemoryView_GET_BUFFER(dataobj); + info.data = (const char *)buf->buf; + info.size = buf->len; } else if (dataobj != Py_None) { _PyArg_BadArgument("get_frozen_object", "argument 2", "bytes", dataobj); @@ -2137,6 +2138,15 @@ _imp_get_frozen_object_impl(PyObject *module, PyObject *name, return NULL; } } + + if (info.nameobj == NULL) { + info.nameobj = name; + } + if (info.size == 0) { + set_frozen_error(FROZEN_INVALID, name); + return NULL; + } + return unmarshal_frozen_code(&info); } From 4f5160ed135e167a44f70fb909081f176f54d658 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 12 Oct 2021 18:03:30 -0600 Subject: [PATCH 09/18] Use PyObject_GetBuffer() in _imp.get_frozen_object(). --- Python/import.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/Python/import.c b/Python/import.c index 5b68bc6277eb5b..52e78057cbeab6 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2118,14 +2118,13 @@ _imp_get_frozen_object_impl(PyObject *module, PyObject *name, /*[clinic end generated code: output=54368a673a35e745 input=034bdb88f6460b7b]*/ { struct frozen_info info = {}; - if (PyBytes_Check(dataobj)) { - info.data = PyBytes_AS_STRING(dataobj); - info.size = PyBytes_Size(dataobj); - } - else if (PyMemoryView_Check(dataobj)) { - Py_buffer *buf = PyMemoryView_GET_BUFFER(dataobj); - info.data = (const char *)buf->buf; - info.size = buf->len; + Py_buffer buf = {}; + if (PyObject_CheckBuffer(dataobj)) { + if (PyObject_GetBuffer(dataobj, &buf, PyBUF_READ) != 0) { + return NULL; + } + info.data = (const char *)buf.buf; + info.size = buf.len; } else if (dataobj != Py_None) { _PyArg_BadArgument("get_frozen_object", "argument 2", "bytes", dataobj); @@ -2147,7 +2146,11 @@ _imp_get_frozen_object_impl(PyObject *module, PyObject *name, return NULL; } - return unmarshal_frozen_code(&info); + PyObject *codeobj = unmarshal_frozen_code(&info); + if (dataobj != Py_None) { + PyBuffer_Release(&buf); + } + return codeobj; } /*[clinic input] From dc152db742d449ad74522dc8515b92cbfe333aca Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 12 Oct 2021 18:23:36 -0600 Subject: [PATCH 10/18] Stop storing the marshaled data in spec.loader_state. --- Lib/importlib/_bootstrap.py | 22 ++--------- Lib/test/test_importlib/frozen/test_finder.py | 11 ------ Lib/test/test_importlib/frozen/test_loader.py | 4 -- Python/clinic/import.c.h | 37 ++++++++++++++----- Python/import.c | 20 ++++++---- 5 files changed, 42 insertions(+), 52 deletions(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index fbf43b0d7813a9..8d2e633aa8e53f 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -838,7 +838,6 @@ def _fix_up_module(cls, module): assert _imp.is_frozen_package(module.__name__) == ispkg, ispkg filename, pkgdir = cls._resolve_filename(origname, spec.name, ispkg) spec.loader_state = state = type(sys.implementation)( - data=None, filename=filename, origname=origname, ) @@ -866,8 +865,7 @@ def _fix_up_module(cls, module): # These checks ensure that _fix_up_module() is only called # in the right places. assert state is not None - assert sorted(vars(state)) == ['data', 'filename', 'origname'], state - assert state.data is None, state.data + assert sorted(vars(state)) == ['filename', 'origname'], state assert state.origname __path__ = spec.submodule_search_locations ispkg = __path__ is not None @@ -917,13 +915,12 @@ def find_spec(cls, fullname, path=None, target=None): info = _call_with_frames_removed(_imp.find_frozen, fullname) if info is None: return None - data, ispkg, origname = info + _, ispkg, origname = info spec = spec_from_loader(fullname, cls, origin=cls._ORIGIN, is_package=ispkg) filename, pkgdir = cls._resolve_filename(origname, fullname, ispkg) spec.loader_state = type(sys.implementation)( - data=data, filename=filename, origname=origname, ) @@ -960,20 +957,7 @@ def create_module(spec): def exec_module(module): spec = module.__spec__ name = spec.name - try: - data = spec.loader_state.data - except AttributeError: - if not _imp.is_frozen(name): - raise ImportError('{!r} is not a frozen module'.format(name), - name=name) - data = None - else: - # We clear the extra data we got from the finder, to save memory. - # Note that if this method is called again (e.g. by - # importlib.reload()) then _imp.get_frozen_object() will notice - # no data was provided and will look it up. - spec.loader_state.data = None - code = _call_with_frames_removed(_imp.get_frozen_object, name, data) + code = _call_with_frames_removed(_imp.get_frozen_object, name) exec(code, module.__dict__) @classmethod diff --git a/Lib/test/test_importlib/frozen/test_finder.py b/Lib/test/test_importlib/frozen/test_finder.py index 654fafcb58a57e..66080b2ade0098 100644 --- a/Lib/test/test_importlib/frozen/test_finder.py +++ b/Lib/test/test_importlib/frozen/test_finder.py @@ -48,17 +48,6 @@ def check_loader_state(self, spec, origname=None, filename=None): actual = dict(vars(spec.loader_state)) - # Check the code object used to import the frozen module. - # We can't compare the marshaled data directly because - # marshal.dumps() would mark "expected" (below) as a ref, - # which slightly changes the output. - # (See https://bugs.python.org/issue34093.) - data = actual.pop('data') - with import_helper.frozen_modules(): - expected = _imp.get_frozen_object(spec.name) - code = marshal.loads(data) - self.assertEqual(code, expected) - # Check the rest of spec.loader_state. expected = dict( origname=origname, diff --git a/Lib/test/test_importlib/frozen/test_loader.py b/Lib/test/test_importlib/frozen/test_loader.py index 1e6eebf6880245..f1ccb8a188aca7 100644 --- a/Lib/test/test_importlib/frozen/test_loader.py +++ b/Lib/test/test_importlib/frozen/test_loader.py @@ -44,14 +44,12 @@ class ExecModuleTests(abc.LoaderTests): def exec_module(self, name, origname=None): with import_helper.frozen_modules(): is_package = self.machinery.FrozenImporter.is_package(name) - code = _imp.get_frozen_object(name) spec = self.machinery.ModuleSpec( name, self.machinery.FrozenImporter, origin='frozen', is_package=is_package, loader_state=types.SimpleNamespace( - data=marshal.dumps(code), origname=origname or name, filename=resolve_stdlib_file(origname or name, is_package), ), @@ -78,7 +76,6 @@ def test_module(self): self.assertEqual(getattr(module, attr), value) self.assertEqual(output, 'Hello world!\n') self.assertTrue(hasattr(module, '__spec__')) - self.assertIsNone(module.__spec__.loader_state.data) self.assertEqual(module.__spec__.loader_state.origname, name) def test_package(self): @@ -92,7 +89,6 @@ def test_package(self): name=name, attr=attr, given=attr_value, expected=value)) self.assertEqual(output, 'Hello world!\n') - self.assertIsNone(module.__spec__.loader_state.data) self.assertEqual(module.__spec__.loader_state.origname, name) def test_lacking_parent(self): diff --git a/Python/clinic/import.c.h b/Python/clinic/import.c.h index dfb59de3b5ce80..6052316cdd8e3e 100644 --- a/Python/clinic/import.c.h +++ b/Python/clinic/import.c.h @@ -170,7 +170,7 @@ _imp_init_frozen(PyObject *module, PyObject *arg) } PyDoc_STRVAR(_imp_find_frozen__doc__, -"find_frozen($module, name, /)\n" +"find_frozen($module, name, /, *, withdata=False)\n" "--\n" "\n" "Return info about the corresponding frozen module (if there is one) or None.\n" @@ -184,26 +184,43 @@ PyDoc_STRVAR(_imp_find_frozen__doc__, " the module\'s current name)"); #define _IMP_FIND_FROZEN_METHODDEF \ - {"find_frozen", (PyCFunction)_imp_find_frozen, METH_O, _imp_find_frozen__doc__}, + {"find_frozen", (PyCFunction)(void(*)(void))_imp_find_frozen, METH_FASTCALL|METH_KEYWORDS, _imp_find_frozen__doc__}, static PyObject * -_imp_find_frozen_impl(PyObject *module, PyObject *name); +_imp_find_frozen_impl(PyObject *module, PyObject *name, int withdata); static PyObject * -_imp_find_frozen(PyObject *module, PyObject *arg) +_imp_find_frozen(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames) { PyObject *return_value = NULL; + static const char * const _keywords[] = {"", "withdata", NULL}; + static _PyArg_Parser _parser = {NULL, _keywords, "find_frozen", 0}; + PyObject *argsbuf[2]; + Py_ssize_t noptargs = nargs + (kwnames ? PyTuple_GET_SIZE(kwnames) : 0) - 1; PyObject *name; + int withdata = 0; - if (!PyUnicode_Check(arg)) { - _PyArg_BadArgument("find_frozen", "argument", "str", arg); + args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, argsbuf); + if (!args) { goto exit; } - if (PyUnicode_READY(arg) == -1) { + if (!PyUnicode_Check(args[0])) { + _PyArg_BadArgument("find_frozen", "argument 1", "str", args[0]); goto exit; } - name = arg; - return_value = _imp_find_frozen_impl(module, name); + if (PyUnicode_READY(args[0]) == -1) { + goto exit; + } + name = args[0]; + if (!noptargs) { + goto skip_optional_kwonly; + } + withdata = PyObject_IsTrue(args[1]); + if (withdata < 0) { + goto exit; + } +skip_optional_kwonly: + return_value = _imp_find_frozen_impl(module, name, withdata); exit: return return_value; @@ -548,4 +565,4 @@ _imp_source_hash(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyOb #ifndef _IMP_EXEC_DYNAMIC_METHODDEF #define _IMP_EXEC_DYNAMIC_METHODDEF #endif /* !defined(_IMP_EXEC_DYNAMIC_METHODDEF) */ -/*[clinic end generated code: output=8c8dd08158f9ac7c input=a9049054013a1b77]*/ +/*[clinic end generated code: output=adcf787969a11353 input=a9049054013a1b77]*/ diff --git a/Python/import.c b/Python/import.c index 52e78057cbeab6..d2d14891907ae8 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2050,6 +2050,8 @@ _imp.find_frozen name: unicode / + * + withdata: bool = False Return info about the corresponding frozen module (if there is one) or None. @@ -2063,8 +2065,8 @@ The returned info (a 2-tuple): [clinic start generated code]*/ static PyObject * -_imp_find_frozen_impl(PyObject *module, PyObject *name) -/*[clinic end generated code: output=3fd17da90d417e4e input=6aa7b9078a89280a]*/ +_imp_find_frozen_impl(PyObject *module, PyObject *name, int withdata) +/*[clinic end generated code: output=8c1c3c7f925397a5 input=22a8847c201542fd]*/ { struct frozen_info info; frozen_status status = find_frozen(name, &info); @@ -2079,10 +2081,12 @@ _imp_find_frozen_impl(PyObject *module, PyObject *name) return NULL; } - PyObject *data = PyMemoryView_FromMemory((char *)info.data, info.size, - PyBUF_READ); - if (data == NULL) { - return NULL; + PyObject *data = NULL; + if (withdata) { + data = PyMemoryView_FromMemory((char *)info.data, info.size, PyBUF_READ); + if (data == NULL) { + return NULL; + } } PyObject *origname = NULL; @@ -2094,11 +2098,11 @@ _imp_find_frozen_impl(PyObject *module, PyObject *name) } } - PyObject *result = PyTuple_Pack(3, data, + PyObject *result = PyTuple_Pack(3, data ? data : Py_None, info.is_package ? Py_True : Py_False, origname ? origname : Py_None); Py_XDECREF(origname); - Py_DECREF(data); + Py_XDECREF(data); return result; } From 968720276720e0e41b3833857c0e26475c89c891 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 13 Oct 2021 08:05:31 -0600 Subject: [PATCH 11/18] Empty initializers are not allowed. --- Python/import.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/import.c b/Python/import.c index d2d14891907ae8..12bd969f91a75a 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2121,8 +2121,8 @@ _imp_get_frozen_object_impl(PyObject *module, PyObject *name, PyObject *dataobj) /*[clinic end generated code: output=54368a673a35e745 input=034bdb88f6460b7b]*/ { - struct frozen_info info = {}; - Py_buffer buf = {}; + struct frozen_info info = {0}; + Py_buffer buf = {0}; if (PyObject_CheckBuffer(dataobj)) { if (PyObject_GetBuffer(dataobj, &buf, PyBUF_READ) != 0) { return NULL; From 3685dd257964e1265fdf76bd31417d65a23bde6e Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 14 Oct 2021 09:19:41 -0600 Subject: [PATCH 12/18] Keep _set_fileattr False if there is no location. --- Lib/importlib/_bootstrap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index 8d2e633aa8e53f..0cdf8585801b41 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -484,7 +484,7 @@ def _spec_from_module(module, loader=None, origin=None): submodule_search_locations = None spec = ModuleSpec(name, loader, origin=origin) - spec._set_fileattr = origin == location + spec._set_fileattr = False if location is None else (origin == location) spec.cached = cached spec.submodule_search_locations = submodule_search_locations return spec From 549d3950b91c97ad8fe281009ba7d2346efdf956 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 14 Oct 2021 09:24:14 -0600 Subject: [PATCH 13/18] Drop some unnecessary uses of "state". --- Lib/importlib/_bootstrap.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index 0cdf8585801b41..d1ea19e5a6ee15 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -837,7 +837,7 @@ def _fix_up_module(cls, module): ispkg = hasattr(module, '__path__') assert _imp.is_frozen_package(module.__name__) == ispkg, ispkg filename, pkgdir = cls._resolve_filename(origname, spec.name, ispkg) - spec.loader_state = state = type(sys.implementation)( + spec.loader_state = type(sys.implementation)( filename=filename, origname=origname, ) @@ -864,7 +864,6 @@ def _fix_up_module(cls, module): else: # These checks ensure that _fix_up_module() is only called # in the right places. - assert state is not None assert sorted(vars(state)) == ['filename', 'origname'], state assert state.origname __path__ = spec.submodule_search_locations From 1decabf5ba998154a9bbc1adbcd3d0c1c21e59bf Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 14 Oct 2021 09:31:42 -0600 Subject: [PATCH 14/18] Make an assertion more precise. --- Lib/importlib/_bootstrap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index d1ea19e5a6ee15..52ca44397a4b04 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -858,7 +858,7 @@ def _fix_up_module(cls, module): pass if ispkg: if module.__path__ != __path__: - assert not module.__path__, module.__path__ + assert module.__path__ == [], module.__path__ # XXX _init_module_attrs() should copy like this too. module.__path__.extend(__path__) else: From 4d47f3009d9a0b8206dec6c695b6e2308aca6e10 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 14 Oct 2021 09:33:42 -0600 Subject: [PATCH 15/18] Move a TODO comment. --- Lib/importlib/_bootstrap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index 52ca44397a4b04..216f97f6c286fb 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -541,6 +541,7 @@ def _init_module_attrs(spec, module, *, override=False): # __path__ if override or getattr(module, '__path__', None) is None: if spec.submodule_search_locations is not None: + # XXX We () should extend __path__ if it's already a list. try: module.__path__ = spec.submodule_search_locations except AttributeError: @@ -859,7 +860,6 @@ def _fix_up_module(cls, module): if ispkg: if module.__path__ != __path__: assert module.__path__ == [], module.__path__ - # XXX _init_module_attrs() should copy like this too. module.__path__.extend(__path__) else: # These checks ensure that _fix_up_module() is only called From d6eea90c04044eddc7472ec714b71eaaccce5f9e Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 14 Oct 2021 09:58:42 -0600 Subject: [PATCH 16/18] All modules without "origname" in _fix_up_module(). --- Lib/importlib/_bootstrap.py | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index 216f97f6c286fb..0f47b3304bab77 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -864,19 +864,29 @@ def _fix_up_module(cls, module): else: # These checks ensure that _fix_up_module() is only called # in the right places. - assert sorted(vars(state)) == ['filename', 'origname'], state - assert state.origname __path__ = spec.submodule_search_locations ispkg = __path__ is not None - (filename, pkgdir, - ) = cls._resolve_filename(state.origname, spec.name, ispkg) - assert state.filename == filename, (state.filename, filename) - if pkgdir: - assert __path__ == [pkgdir], (__path__, pkgdir) - elif ispkg: - assert __path__ == [], __path__ - assert hasattr(module, '__file__') - assert module.__file__ == filename, (module.__file__, filename) + # Check the loader state. + assert sorted(vars(state)) == ['filename', 'origname'], state + if state.origname: + # The only frozen modules with "origname" set are stdlib modules. + (__file__, pkgdir, + ) = cls._resolve_filename(state.origname, spec.name, ispkg) + assert state.filename == __file__, (state.filename, __file__) + if pkgdir: + assert __path__ == [pkgdir], (__path__, pkgdir) + else: + assert __path__ == ([] if ispkg else None), __path__ + else: + __file__ = None + assert state.filename is None, state.filename + assert __path__ == ([] if ispkg else None), __path__ + # Check the file attrs. + if __file__: + assert hasattr(module, '__file__') + assert module.__file__ == __file__, (module.__file__, __file__) + else: + assert not hasattr(module, '__file__'), module.__file__ if ispkg: assert hasattr(module, '__path__') assert module.__path__ == __path__, (module.__path__, __path__) From 866fdadcac074b520912e1456d931f4a1da1a68f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 14 Oct 2021 10:44:55 -0600 Subject: [PATCH 17/18] Add a comment about why we don't preserve the data in find_spec(). --- Lib/importlib/_bootstrap.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index 0f47b3304bab77..c2f09072e99899 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -924,6 +924,18 @@ def find_spec(cls, fullname, path=None, target=None): info = _call_with_frames_removed(_imp.find_frozen, fullname) if info is None: return None + # We get the marshaled data in exec_module() (the loader + # part of the importer), instead of here (the finder part). + # The loader is the usual place to get the data that will + # be loaded into the module. (For example, see _LoaderBasics + # in _bootstra_external.py.) Most importantly, this importer + # is simpler if we wait to get the data. + # However, getting as much data in the finder as possible + # to later load the module is okay, and sometimes important. + # (That's why ModuleSpec.loader_state exists.) This is + # especially true if it avoids throwing away expensive data + # the loader would otherwise duplicate later and can be done + # efficiently. In this case it isn't worth it. _, ispkg, origname = info spec = spec_from_loader(fullname, cls, origin=cls._ORIGIN, From 5cba3d17160cf1047dc1c85974cb12514aea5834 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 14 Oct 2021 12:30:37 -0600 Subject: [PATCH 18/18] Fix a typo in a comment. --- Lib/importlib/_bootstrap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py index c2f09072e99899..889f08f8aeec1f 100644 --- a/Lib/importlib/_bootstrap.py +++ b/Lib/importlib/_bootstrap.py @@ -541,7 +541,7 @@ def _init_module_attrs(spec, module, *, override=False): # __path__ if override or getattr(module, '__path__', None) is None: if spec.submodule_search_locations is not None: - # XXX We () should extend __path__ if it's already a list. + # XXX We should extend __path__ if it's already a list. try: module.__path__ = spec.submodule_search_locations except AttributeError: