-
Notifications
You must be signed in to change notification settings - Fork 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
Byte-compile files after installation #8541
Changes from all commits
0a3a558
4cb6f72
2ece73c
42c01ae
452e683
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
import compileall | ||
import contextlib | ||
import csv | ||
import importlib | ||
import logging | ||
import os.path | ||
import re | ||
|
@@ -451,14 +452,6 @@ def install_unpacked_wheel( | |
changed = set() # type: Set[RecordPath] | ||
generated = [] # type: List[str] | ||
|
||
# Compile all of the pyc files that we're going to be installing | ||
if pycompile: | ||
with captured_stdout() as stdout: | ||
with warnings.catch_warnings(): | ||
warnings.filterwarnings('ignore') | ||
compileall.compile_dir(source, force=True, quiet=True) | ||
logger.debug(stdout.getvalue()) | ||
|
||
def record_installed(srcfile, destfile, modified=False): | ||
# type: (text_type, text_type, bool) -> None | ||
"""Map archive RECORD paths to installation RECORD paths.""" | ||
|
@@ -583,6 +576,52 @@ def is_entrypoint_wrapper(name): | |
filter=filter, | ||
) | ||
|
||
def pyc_source_file_paths(): | ||
# type: () -> Iterator[text_type] | ||
# We de-duplicate installation paths, since there can be overlap (e.g. | ||
# file in .data maps to same location as file in wheel root). | ||
# Sorting installation paths makes it easier to reproduce and debug | ||
# issues related to permissions on existing files. | ||
for installed_path in sorted(set(installed.values())): | ||
full_installed_path = os.path.join(lib_dir, installed_path) | ||
if not os.path.isfile(full_installed_path): | ||
continue | ||
if not full_installed_path.endswith('.py'): | ||
continue | ||
yield full_installed_path | ||
|
||
def pyc_output_path(path): | ||
# type: (text_type) -> text_type | ||
"""Return the path the pyc file would have been written to. | ||
""" | ||
if PY2: | ||
if sys.flags.optimize: | ||
return path + 'o' | ||
else: | ||
return path + 'c' | ||
else: | ||
return importlib.util.cache_from_source(path) | ||
|
||
# Compile all of the pyc files for the installed files | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been moved to after some of the other code - is there a reason for this, or was it just for code organisation? I don't think it makes any difference, so I'm not looking for you to change it, I'm just curious as to whether there is a benefit that I missed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Byte-compilation has to occur after we have copied the files to their destination. In the current code that is after the calls to |
||
if pycompile: | ||
with captured_stdout() as stdout: | ||
with warnings.catch_warnings(): | ||
warnings.filterwarnings('ignore') | ||
for path in pyc_source_file_paths(): | ||
# Python 2's `compileall.compile_file` requires a str in | ||
# error cases, so we must convert to the native type. | ||
path_arg = ensure_str( | ||
path, encoding=sys.getfilesystemencoding() | ||
) | ||
success = compileall.compile_file( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. We'd still need to manually get the path on Python 2, unfortunately, but it should be about the same amount of code. I had originally gone that route, but figured the jump from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3 more months. Just 3 more. |
||
path_arg, force=True, quiet=True | ||
) | ||
if success: | ||
pyc_path = pyc_output_path(path) | ||
assert os.path.exists(pyc_path) | ||
record_installed(pyc_path, pyc_path) | ||
logger.debug(stdout.getvalue()) | ||
|
||
maker = PipScriptMaker(None, scheme.scripts) | ||
|
||
# Ensure old scripts are overwritten. | ||
|
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 might be better moved to utils, as it's basically a compatibility wrapper around
importlib.util.cache_from_source
. Once we drop Python 2 support, we should remove this in favour of calling importlib directly.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.
If/when we switch to
py_compile.compile
I expect this to go away (or only be used inside it) since we'd need a compatibility wrapper for that as well.