[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public
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

Trivial cleanups following bpo-31370 #3649

Merged
merged 2 commits into from Sep 18, 2017
Merged

Conversation

Copy link
Member

pitrou commented Sep 18, 2017

Copy link
Member

ericsnowcurrently left a comment

The importlib changes look good to me.

if builtin_name not in sys.modules:
builtin_module = _builtin_from_name(builtin_name)
else:
builtin_module = sys.modules[builtin_name]
setattr(self_module, builtin_name, builtin_module)

# Directly load the _thread module (needed during bootstrap).
Copy link
Member

serhiy-storchaka Sep 18, 2017

Choose a reason for hiding this comment

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

Please look also at _bootstrap_external.py. It contains almost duplicated code.

Copy link
Member Author

pitrou Sep 18, 2017

Choose a reason for hiding this comment

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

Looks like you're right. @ericsnowcurrently, out of curiosity, why the two "bootstrap" modules?

Copy link
Member

ericsnowcurrently Sep 18, 2017

Choose a reason for hiding this comment

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

A while back we split the fundamental import machinery from the file-based parts. The "external" part encapsulates the file-based finders and loaders (e.g. .py, extensions) and related functionality. In addition to maintenance benefits, during runtime startup it helps to have a hard separation between the two. Furthermore, the split also supports some planned work (encapsulating the import state).

Copy link
Contributor

ncoghlan Sep 19, 2017

Choose a reason for hiding this comment

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

In particular, with the multi-phase bootstrap refactoring, the builtin and frozen module support is initialised as one of the last steps in the core runtime initialisation (https://github.com/python/cpython/blob/master/Python/pylifecycle.c#L717), while setting up filesystem imports doesn't happen until the main interpreter is being configured (https://github.com/python/cpython/blob/master/Python/pylifecycle.c#L803)

Copy link
Member Author

pitrou Sep 19, 2017

Choose a reason for hiding this comment

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

Thank you both for the explanation. I think if those files had been named something else (such as _bootstrap_core.py and _bootstrap_importers.py) the situation would have been less confusing to me ("externals" sounded it was something else entirely). Also, you could consider improving the top-level module docstrings and/or comments to describe the split accurately.

pitrou merged commit 88c60c9 into python:master Sep 18, 2017
pitrou deleted the threading_cleanups branch Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants