bpo-45272: os.path should not be a frozen module#29329
bpo-45272: os.path should not be a frozen module#29329FFY00 wants to merge 1 commit intopython:mainfrom
Conversation
Signed-off-by: Filipe Laíns <lains@riseup.net>
| # 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 |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
related bpo: https://bugs.python.org/issue35321
There was a problem hiding this comment.
couldn't we do loader.origin == 'frozen' instead?
Let's be explicit about it with the check you have.
There was a problem hiding this comment.
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 😛
There was a problem hiding this comment.
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.)
|
This PR is stale because it has been open for 30 days with no activity. |
|
FYI, I'm trying to track down why I'd frozen os.path in the first place. It doesn't seem like it should be necessary. I remember it was in response to a failing test and it related to using the finder of the parent module and maybe something about runpy. I'm running the test suite with os.path not frozen to see. |
ericsnowcurrently
left a comment
ericsnowcurrently
left a comment
There was a problem hiding this comment.
LGTM
(with the importlib changes dropped)
| """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 ( |
There was a problem hiding this comment.
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. 🙂
| 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 | ||
| ): |
There was a problem hiding this comment.
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.
| 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): |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
ericsnowcurrently
removed
the
stale
|
Closing this in favor of a new PR to avoid either overwriting the commit history or using merge commit and pinging everyone. |
5 participants
Signed-off-by: Filipe Laíns lains@riseup.net
https://bugs.python.org/issue45272