-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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.
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
bpo-40350: fix namespace package support in modulefinder #29196
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Filipe Laíns <lains@riseup.net>
|
Signed-off-by: Filipe Laíns <lains@riseup.net>
# See # https://docs.python.org/3/library/importlib.html#importlib.abc.Loader.load_module | ||
# and bpo-32305 | ||
module.__file__ = None | ||
if _bootstrap_external and isinstance(loader, _bootstrap_external.NamespaceLoader): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rescoping this change means it no longer falls under "A backward compatibility hack." Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the if loader is None
block above still needed now that spec.loader
is set in _bootstrap_external
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did do a deep dive into this and was meaning to write a more in-depth explanation. If I understand correctly, the backwards compatibility hack is instantiating the loader, not setting __file__
. This function is meant to fill these missing attributes, but up until now it always expected to be the one instantiating the loader on namespace packages, which now changed with this PR. This should also trip up on 3rd party finders that set the loader for namespace packages, but so far no one has reported it yet.
# this is the best place to ensure this consistency. | ||
# | ||
# See # https://docs.python.org/3/library/importlib.html#importlib.abc.Loader.load_module | ||
# and bpo-32305 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this comment should now also mention bpo-40350.
@@ -17,6 +17,7 @@ | |||
_PKG_DIRECTORY = 5 | |||
_C_BUILTIN = 6 | |||
_PY_FROZEN = 7 | |||
_NAMESPACE = 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change feels wrong based on the comment above ("Old imp constants"), because it introduces a "new old constant". Perhaps the comment just needs to be replaced with something more accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I share the same concern, but these constants are being used here to identify the module type, so maybe we can change the comment to "constants initially imported from imp module" or something similar.
I see tests are failing. I suspect that's because the change modifies Edit: I ran |
I am not home now, so I will finish up looking into the feedback later. |
This PR is stale because it has been open for 30 days with no activity. |
Any progress on this PR? I have run into the same issue and was wondering if this fix was coming anytime soon. I can take a look but I have never contributed so I don't really know how that works with a PR I didn't make myself. |
I will try to rebase the PR and fix the tests in the weekend. |
Signed-off-by: Filipe Laíns lains@riseup.net
https://bugs.python.org/issue40350