classification
Title: 'os.path' should not be a frozen module
Type: behavior Stage: needs patch
Components: Versions: Python 3.11
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: eric.snow Nosy List: barry, brett.cannon, corona10, eric.snow, gvanrossum, jaraco, ncoghlan, steve.dower
Priority: normal Keywords:

Created on 2021-09-23 16:24 by steve.dower, last changed 2021-10-06 19:57 by gvanrossum.

Messages (5)
msg402506 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-09-23 16:24
I noticed that Python/frozen.c includes posixpath as 'os.path'.

This is not correct, and shouldn't be necessary anyway, because os.path is just an attribute in "os" and not a concrete module (see Lib/os.py#L95 for the bit that makes it importable, and Lib/os.py#L61 and Lib/os.py#L81 for the imports).
msg402516 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-09-23 17:52
The matter here boils down to the design of _imp.is_frozen() [1].  It only checks to see if the given module name shows up in the list of frozen modules in Python/frozen.c.  This broke things when I froze os and posixpath/ntpath.

The simplest solution was to include os.path in the list of modules in frozen.c.  The better solution would be to have _imp.is_frozen() check the module in sys.modules.


[1] see find_frozen() in Python/import.c
msg403327 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-10-06 17:38
I'm trying to understand the proposed solution, "have _imp.is_frozen() check the module in sys.modules." Does that mean it would do a dict lookup first? Maybe you should do that in the caller instead? importlib/_bootstrap.py calls it a few times, but I'm not convinced that all call sites can be called with "os.path" -- do you know which path was taken when this failed?

I worry about the complexity of the importlib bootstrapping mechanism. Grepping through the source for _bootstrap and _bootstrap_external has not given me any understanding of how this works. Do you know if there are docs for this? Or do we just need to ask Brett?
msg403332 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-10-06 18:49
On Wed, Oct 6, 2021 at 11:38 AM Guido van Rossum <report@bugs.python.org> wrote:
> I'm trying to understand the proposed solution, "have _imp.is_frozen() check the module in sys.modules." Does that mean it would do a dict lookup first?

Correct.  We'd look up the module in sys.modules and, if there, check its loader.

> Maybe you should do that in the caller instead? importlib/_bootstrap.py calls it a few times, but I'm not convinced that all call sites can be called with "os.path" -- do you know which path was taken when this failed?

Good point.  The only place where it matters is the FrozenImporter methods that are wrapped with _requires_frozen().  So the fix can go there instead of _imp.is_frozen().

> I worry about the complexity of the importlib bootstrapping mechanism. Grepping through the source for _bootstrap and _bootstrap_external has not given me any understanding of how this works. Do you know if there are docs for this? Or do we just need to ask Brett?

Are you talking about the use of _imp.is_frozen() or do you mean the code in _bootstrap.py (and _bootstrap_external.py) as a whole?  I don't believe there's any official documentation about the implementation in _bootstrap.py.  At best there have been some PyCon talks about how the import system works (but probably not at the level of the implementation) and Brett may have a blog post or two.  Keep in mind that I'm quite familiar with the importlib code, though Brett is definitely the mastermind behind the overall implementation.
msg403334 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-10-06 19:57
It was a comment about my general lack of understanding of how the importlib bootstrap process works. I should probably start reading the docstrings before complaining more. :-)
History
Date User Action Args
2021-10-06 19:57:06gvanrossumsetmessages: + msg403334
2021-10-06 18:49:55eric.snowsetmessages: + msg403332
2021-10-06 17:38:34gvanrossumsetnosy: + gvanrossum
messages: + msg403327
2021-09-24 15:53:53corona10setnosy: + corona10
2021-09-23 17:52:38eric.snowsetnosy: + barry, brett.cannon, jaraco, ncoghlan
messages: + msg402516
2021-09-23 16:24:55steve.dowercreate