gh-92205: set loader for namespace packages PathFinder.find_spec #92279
Conversation
…_namespace_changed Previously, we were blocking the frozen imports and forceing the source version to be used, but we did not fix up sys.meta_path or sys.path_hooks, causing the frozen importers to leak into the source version of the test.
| try: | ||
| module.__loader__ = loader | ||
| module.__loader__ = spec.loader | ||
| except AttributeError: | ||
| pass |
There was a problem hiding this comment.
It's an unlikely case, but spec might not have a loader attribute. So we should probably keep the existing
loader = spec.loader
outside the try block.
| # should also be None for consistency. While a bit of a hack, | ||
| # this is the best place to ensure this consistency. | ||
| # | ||
| # See # https://docs.python.org/3/library/importlib.html#importlib.abc.Loader.load_module |
There was a problem hiding this comment.
While you're here:
| # See # https://docs.python.org/3/library/importlib.html#importlib.abc.Loader.load_module | |
| # See https://docs.python.org/3/library/importlib.html#importlib.abc.Loader.load_module |
| @@ -560,6 +538,19 @@ def _init_module_attrs(spec, module, *, override=False): | |||
| module.__cached__ = spec.cached | |||
| except AttributeError: | |||
| pass | |||
| # A backward compatibility hack. | |||
| if _bootstrap_external and isinstance(spec.loader, _bootstrap_external.NamespaceLoader): | |||
There was a problem hiding this comment.
| if _bootstrap_external and isinstance(spec.loader, _bootstrap_external.NamespaceLoader): | |
| elif _bootstrap_external and isinstance(spec.loader, _bootstrap_external.NamespaceLoader): |
| @@ -560,6 +538,19 @@ def _init_module_attrs(spec, module, *, override=False): | |||
| module.__cached__ = spec.cached | |||
| except AttributeError: | |||
| pass | |||
| # A backward compatibility hack. | |||
There was a problem hiding this comment.
I'm pretty sure you've dropped the backward compatibility part, so we can drop this comment.
| # A backward compatibility hack. |
There was a problem hiding this comment.
The backward compatibility hack this is referring is setting __file__ to None on namespace packages, which we are keeping.
| @@ -1508,6 +1508,7 @@ def find_spec(cls, fullname, path=None, target=None): | |||
| # can create the namespace package. | |||
| spec.origin = None | |||
| spec.submodule_search_locations = _NamespacePath(fullname, namespace_path, cls._get_spec) | |||
| spec.loader = NamespaceLoader(fullname, namespace_path, cls._get_spec) | |||
There was a problem hiding this comment.
(Nothing to do here.)
I just noticed that PathEntryFinder.find_spec() is expected to return a spec with loader set to None, rather than NamespaceLoader (which is mostly fine). See #92896.
|
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 |
There was a problem hiding this comment.
I'm finding it difficult to follow all the changes, but am I reading this right that this will change the __spec__.loader attribute on namespace packages (not namespace package portions) to a NamespaceLoader instance? I think that breaks backward compatibility and wasn't how I read the description.
I thought __spec__.loader for namespace packages is not always getting set. Setting it to None is perfectly reasonable and backward compatible for namespace packages, but it should always be None and not just missing in some cases.
gh-92205
On top of #92275