Skip to content

bpo-45272: os.path should not be a frozen module #29329

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

bpo-45272: os.path should not be a frozen module #29329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Lib/importlib/_bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,13 @@ def _requires_builtin_wrapper(self, fullname):
def _requires_frozen(fxn):
"""Decorator to verify the named module is frozen."""
def _requires_frozen_wrapper(self, fullname):
if not _imp.is_frozen(fullname):
if not _imp.is_frozen(fullname) or (
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that _imp.is_frozen() treats any error (including FROZEN_EXCLUDED) as "not frozen", which could lead to weirdness in some corner cases (e.g. _imp.is_frozen() returns True but FrozenImporter.find_spec() returns None).

That's not relevant for this PR, and probably not something to worry about all that much, but I hadn't noticed it before. 🙂

# bpo-45272: os.path is not a concrete module, but an importable
# attribute of 'os'. _imp.is_frozen('os.path') returns
# False but it is frozen, so we check the loader here.
fullname in sys.modules
and sys.modules[fullname].__spec__.loader is FrozenImporter
Comment on lines +267 to +271
Copy link
Member Author

Choose a reason for hiding this comment

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

@ericsnowcurrently, couldn't we do loader.origin == 'frozen' instead? Are we planing to change the value of origin to the actual file or something like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

related bpo: https://bugs.python.org/issue35321

Copy link
Member

Choose a reason for hiding this comment

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

couldn't we do loader.origin == 'frozen' instead?

Let's be explicit about it with the check you have.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the reasoning is there some different frozen importer type might be added, and this code might break. That scenario seemed reasonable to me with the complex bootstrapping architecture, but, of course, I lack the experience to make educated guesses 😛

Copy link
Member

Choose a reason for hiding this comment

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

some different frozen importer type might be added, and this code might break.

We can deal with it if that ever happens. The set of conditions here is so specific, yet highly unlikely. Plus, if the check here somehow breaks in the future, the problem should be fairly obvious. However, I expect it wouldn't ever be a problem.

(Regardless, the point is moot now as I'm pretty sure we can drop the importlib change.)

):
Comment on lines +266 to +272
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm pretty sure we don't need to add the special case here any more. I don't remember why I had to freeze os.path in the first place, other than that a test was failing. It must have been something that was fixed elsewhere, since the test suite passes now. So go ahead and drop this change.

Suggested change
if not _imp.is_frozen(fullname) or (
# bpo-45272: os.path is not a concrete module, but an importable
# attribute of 'os'. _imp.is_frozen('os.path') returns
# False but it is frozen, so we check the loader here.
fullname in sys.modules
and sys.modules[fullname].__spec__.loader is FrozenImporter
):
if not _imp.is_frozen(fullname):

raise ImportError('{!r} is not a frozen module'.format(fullname),
name=fullname)
return fxn(self, fullname)
Expand Down
2 changes: 0 additions & 2 deletions Python/frozen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ static const struct _frozen stdlib_modules[] = {
{"genericpath", _Py_M__genericpath, (int)sizeof(_Py_M__genericpath)},
{"ntpath", _Py_M__ntpath, (int)sizeof(_Py_M__ntpath)},
{"posixpath", _Py_M__posixpath, (int)sizeof(_Py_M__posixpath)},
{"os.path", _Py_M__posixpath, (int)sizeof(_Py_M__posixpath)},
{"os", _Py_M__os, (int)sizeof(_Py_M__os)},
{"site", _Py_M__site, (int)sizeof(_Py_M__site)},
{"stat", _Py_M__stat, (int)sizeof(_Py_M__stat)},
Expand Down Expand Up @@ -114,7 +113,6 @@ const struct _frozen *_PyImport_FrozenTest = test_modules;
static const struct _module_alias aliases[] = {
{"_frozen_importlib", "importlib._bootstrap"},
{"_frozen_importlib_external", "importlib._bootstrap_external"},
{"os.path", "posixpath"},
{"__hello_alias__", "__hello__"},
{"__phello_alias__", "__hello__"},
{"__phello_alias__.spam", "__hello__"},
Expand Down
3 changes: 0 additions & 3 deletions Tools/scripts/freeze_modules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,6 @@ def find_tool():
'genericpath',
'ntpath',
'posixpath',
# We must explicitly mark os.path as a frozen module
# even though it will never be imported.
f'{OS_PATH} : os.path',
'os',
'site',
'stat',
Expand Down