-
-
Notifications
You must be signed in to change notification settings - Fork 292
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
Fix site-packages identification. #2262
Changes from 2 commits
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 |
---|---|---|
@@ -1,7 +1,7 @@ | ||
# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import absolute_import, print_function | ||
from __future__ import absolute_import | ||
|
||
import ast | ||
import os | ||
|
@@ -17,7 +17,7 @@ | |
from pex.executor import Executor | ||
from pex.finders import get_entry_point_from_console_script, get_script_from_distributions | ||
from pex.inherit_path import InheritPath | ||
from pex.interpreter import PythonInterpreter | ||
from pex.interpreter import PythonIdentity, PythonInterpreter | ||
from pex.layout import Layout | ||
from pex.orderedset import OrderedSet | ||
from pex.pex_info import PexInfo | ||
|
@@ -51,41 +51,48 @@ class IsolatedSysPath(object): | |
@staticmethod | ||
def _expand_paths(*paths): | ||
# type: (*str) -> Iterator[str] | ||
seen = set() | ||
for path in paths: | ||
if path in seen: | ||
continue | ||
seen.add(path) | ||
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. Is this set just an optimisation, or is trying to be a guarantee that all output paths are unique? If it's meant to be a guarantee, as written, it seems possible that this could be violated, e.g.:
Maybe this private function is not ever actually called with problematic args, though? 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. The questions the code raised are enough here to justify simplifying at the expense of slightly more work. Fixed. |
||
yield path | ||
if not os.path.isabs(path): | ||
yield os.path.abspath(path) | ||
yield os.path.realpath(path) | ||
realpath = os.path.realpath(path) | ||
if realpath != path: | ||
yield realpath | ||
|
||
@classmethod | ||
def for_pex( | ||
cls, | ||
interpreter, # type: PythonInterpreter | ||
interpreter, # type: Union[PythonInterpreter, PythonIdentity] | ||
pex, # type: str | ||
pex_pex=None, # type: Optional[str] | ||
): | ||
# type: (...) -> IsolatedSysPath | ||
sys_path = OrderedSet(interpreter.sys_path) | ||
ident = interpreter.identity if isinstance(interpreter, PythonInterpreter) else interpreter | ||
sys_path = OrderedSet(ident.sys_path) | ||
sys_path.add(pex) | ||
sys_path.add(Bootstrap.locate().path) | ||
if pex_pex: | ||
sys_path.add(pex_pex) | ||
|
||
site_packages = OrderedSet() # type: OrderedSet[str] | ||
for site_lib in interpreter.site_packages: | ||
for site_lib in ident.site_packages: | ||
TRACER.log("Discarding site packages path: {site_lib}".format(site_lib=site_lib)) | ||
site_packages.add(site_lib) | ||
|
||
extras_paths = OrderedSet() # type: OrderedSet[str] | ||
for extras_path in interpreter.extras_paths: | ||
for extras_path in ident.extras_paths: | ||
TRACER.log("Discarding site extras path: {extras_path}".format(extras_path=extras_path)) | ||
extras_paths.add(extras_path) | ||
|
||
return cls( | ||
sys_path=sys_path, | ||
site_packages=site_packages, | ||
extras_paths=extras_paths, | ||
is_venv=interpreter.is_venv, | ||
is_venv=ident.is_venv, | ||
) | ||
|
||
def __init__( | ||
|
@@ -339,7 +346,7 @@ def all_distribution_paths(path): | |
# type: (Optional[str]) -> Iterable[str] | ||
if path is None: | ||
return () | ||
locations = set(dist.location for dist in find_distributions(path)) | ||
locations = set(dist.location for dist in find_distributions([path])) | ||
return {path} | locations | set(os.path.realpath(path) for path in locations) | ||
|
||
for path_element in sys.path: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,16 +61,12 @@ def register( | |
), | ||
) | ||
|
||
current_interpreter = PythonInterpreter.get() | ||
program = sys.argv[0] | ||
singe_interpreter_info_cmd = ( | ||
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. Fix the typo to 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. Fixed. |
||
"PEX_TOOLS=1 {current_interpreter} {program} interpreter --verbose --indent 4".format( | ||
current_interpreter=current_interpreter.binary, program=program | ||
"pex3 interpreter inspect --python {current_interpreter} --verbose --indent 4".format( | ||
current_interpreter=sys.executable | ||
) | ||
) | ||
all_interpreters_info_cmd = ( | ||
"PEX_TOOLS=1 {program} interpreter --all --verbose --indent 4".format(program=program) | ||
) | ||
all_interpreters_info_cmd = "pex3 interpreter inspect --all --verbose --indent 4" | ||
|
||
parser.add_argument( | ||
"--interpreter-constraint", | ||
|
@@ -86,22 +82,19 @@ def register( | |
"the constraints. Try `{singe_interpreter_info_cmd}` to find the exact interpreter " | ||
"constraints of {current_interpreter} and `{all_interpreters_info_cmd}` to find out " | ||
"the interpreter constraints of all Python interpreters on the $PATH.".format( | ||
current_interpreter=current_interpreter.binary, | ||
current_interpreter=sys.executable, | ||
singe_interpreter_info_cmd=singe_interpreter_info_cmd, | ||
all_interpreters_info_cmd=all_interpreters_info_cmd, | ||
) | ||
), | ||
) | ||
|
||
if include_platforms: | ||
_register_platform_options( | ||
parser, current_interpreter, singe_interpreter_info_cmd, all_interpreters_info_cmd | ||
) | ||
_register_platform_options(parser, singe_interpreter_info_cmd, all_interpreters_info_cmd) | ||
|
||
|
||
def _register_platform_options( | ||
parser, # type: _ActionsContainer | ||
current_interpreter, # type: PythonInterpreter | ||
singe_interpreter_info_cmd, # type: str | ||
all_interpreters_info_cmd, # type: str | ||
): | ||
|
@@ -120,13 +113,11 @@ def _register_platform_options( | |
"string composed of fields: <platform>-<python impl abbr>-<python version>-<abi>. " | ||
"These fields stem from wheel name conventions as outlined in " | ||
"https://www.python.org/dev/peps/pep-0427#file-name-convention and influenced by " | ||
"https://www.python.org/dev/peps/pep-0425. For the current interpreter at " | ||
"{current_interpreter} the full platform string is {current_platform}. To find out " | ||
"more, try `{all_interpreters_info_cmd}` to print out the platform for all " | ||
"interpreters on the $PATH or `{singe_interpreter_info_cmd}` to inspect the single " | ||
"interpreter {current_interpreter}.".format( | ||
current_interpreter=current_interpreter.binary, | ||
current_platform=current_interpreter.platform, | ||
"https://www.python.org/dev/peps/pep-0425. To find out more, try " | ||
"`{all_interpreters_info_cmd}` to print out the platform for all interpreters on the " | ||
"$PATH or `{singe_interpreter_info_cmd}` to inspect the single interpreter " | ||
"{current_interpreter}.".format( | ||
current_interpreter=sys.executable, | ||
singe_interpreter_info_cmd=singe_interpreter_info_cmd, | ||
all_interpreters_info_cmd=all_interpreters_info_cmd, | ||
) | ||
|
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 was the bug. Here
PythonIdentity.get()
slurps up the ambientsys.path
which may have been mucked with. Totally classic premature optimization. Don't optimize, instead just don't pessimize. Do the simplest thing, you're not smart enough to do more.