Skip to content

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Mar 27, 2018

https://bugs.python.org/issue33128

It seems that in 1abcf67 _Py_InitializeEx_Private calls _Py_InitializeCore and _Py_InitializeMainInterpreter. The first one calls at the end initimport that in turns calls importlib._install_external_importers that sets the PathFinder in sys.meta_path. Then, _Py_InitializeMainInterpreter calls initexternalimport that in turns calls again importlib._install_external_importers and therefore we end with two PathFinders in sys.meta_path.

This PR eliminates duplicated calls to importlib._install_external_importers.

@pablogsal
Copy link
Member Author

CC: @ncoghlan @ned-deily @brettcannon

@brettcannon brettcannon requested a review from ncoghlan March 27, 2018 22:41
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to stay - the bug is that initimport is trying to install the external importers that rely on sys.path (et al) already being configured properly.

test.sh Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect you didn't mean to check this in :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Woooops! 🙃

@pablogsal
Copy link
Member Author

@ncoghlan I have removed the call from initimportin 6b5518a. Please, check if this is correct or I am missing something.

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

The change itself looks good to me now. The last item we need is a NEWS entry, as described in https://devguide.python.org/committing/#what-s-new-and-news-entries, which can be created using the blurb tool at https://pypi.org/project/blurb/

Alternatively, just comment below and I can add a suitable blurb entry for you (that may take a bit longer, though)

@pablogsal
Copy link
Member Author

@ncoghlan I have added the News entry. Thanks for reviewing!

@ncoghlan ncoghlan merged commit 0977091 into python:master Apr 25, 2018
@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-6592 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 25, 2018
pythonGH-6273)

External importers were being added in both phases of the import
system initialisation.

They're only supposed to be added in the second phase, after the
import machinery has been appropriately configured.
(cherry picked from commit 0977091)

Co-authored-by: Pablo Galindo <[email protected]>
miss-islington added a commit that referenced this pull request Apr 25, 2018
GH-6273)

External importers were being added in both phases of the import
system initialisation.

They're only supposed to be added in the second phase, after the
import machinery has been appropriately configured.
(cherry picked from commit 0977091)

Co-authored-by: Pablo Galindo <[email protected]>
@pablogsal pablogsal deleted the bpo33128 branch May 19, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants