Skip to content
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

Merged
merged 5 commits into from
Jul 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 47 additions & 8 deletions src/pip/_internal/operations/install/wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import compileall
import contextlib
import csv
import importlib
import logging
import os.path
import re
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -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):
Copy link
Member

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.

Copy link
Member Author

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.

# 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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 clobber with the root scheme files and the files in *.data.

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(
Copy link
Member

Choose a reason for hiding this comment

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

Using py_compile.compile would be better, as it returns the path of the compiled file, so we don't have to rely on recalculating it ourselves. The force and quiet arguments don't exist for that function, though, so we would need to change the logic somehow. I'm fine with doing that in a follow-up PR (and it's not a priority).

Copy link
Member Author

Choose a reason for hiding this comment

The 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 compileall.compile_dir to compileall.compile_file was big enough, and would at least help get us closer.

Copy link
Member

Choose a reason for hiding this comment

The 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.
Expand Down
10 changes: 8 additions & 2 deletions tests/unit/test_wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,15 @@ class TestInstallUnpackedWheel(object):
"""

def prep(self, data, tmpdir):
# Since Path implements __add__, os.path.join returns a Path object.
# Passing Path objects to interfaces expecting str (like
# `compileall.compile_file`) can cause failures, so we normalize it
# to a string here.
tmpdir = str(tmpdir)
self.name = 'sample'
self.wheelpath = data.packages.joinpath(
'sample-1.2.0-py2.py3-none-any.whl')
self.wheelpath = os.path.join(
str(data.packages), 'sample-1.2.0-py2.py3-none-any.whl'
)
self.req = Requirement('sample')
self.src = os.path.join(tmpdir, 'src')
self.dest = os.path.join(tmpdir, 'dest')
Expand Down