-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
Add first-class support for multi-interpreter and multi-platform pex construction. #394
Changes from 1 commit
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,6 +1,7 @@ | ||
*~ | ||
*.pyc | ||
*.egg-info | ||
/.cache | ||
/venv | ||
/build/* | ||
/dist/* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ | |
from pex.platforms import Platform | ||
from pex.requirements import requirements_from_file | ||
from pex.resolvable import Resolvable | ||
from pex.resolver import CachingResolver, Resolver, Unsatisfiable | ||
from pex.resolver import Unsatisfiable, resolve_multi | ||
from pex.resolver_options import ResolverOptionsBuilder | ||
from pex.tracer import TRACER | ||
from pex.variables import ENV, Variables | ||
|
@@ -274,9 +274,12 @@ def configure_clp_pex_environment(parser): | |
group.add_option( | ||
'--python', | ||
dest='python', | ||
default=None, | ||
default=[], | ||
type='str', | ||
action='append', | ||
help='The Python interpreter to use to build the pex. Either specify an explicit ' | ||
'path to an interpreter, or specify a binary accessible on $PATH. ' | ||
'path to an interpreter, or specify a binary accessible on $PATH. This option ' | ||
'can be passed multiple times to create a multi-interpreter compatible pex. ' | ||
'Default: Use current interpreter.') | ||
|
||
group.add_option( | ||
|
@@ -290,8 +293,11 @@ def configure_clp_pex_environment(parser): | |
group.add_option( | ||
'--platform', | ||
dest='platform', | ||
default=Platform.current(), | ||
help='The platform for which to build the PEX. Default: %default') | ||
default=[], | ||
type=str, | ||
action='append', | ||
help='The platform for which to build the PEX. This option can be passed multiple times ' | ||
'to create a multi-platform compatible pex. Default: %default') | ||
|
||
group.add_option( | ||
'--interpreter-cache-dir', | ||
|
@@ -460,39 +466,54 @@ def installer_provider(sdist): | |
return interpreter.with_extra(egg.name, egg.raw_version, egg.path) | ||
|
||
|
||
def interpreter_from_options(options): | ||
def get_interpreter(python_interpreter, interpreter_cache_dir, repos, use_wheel): | ||
interpreter = None | ||
|
||
if options.python: | ||
if os.path.exists(options.python): | ||
interpreter = PythonInterpreter.from_binary(options.python) | ||
if python_interpreter: | ||
if os.path.exists(python_interpreter): | ||
interpreter = PythonInterpreter.from_binary(python_interpreter) | ||
else: | ||
interpreter = PythonInterpreter.from_env(options.python) | ||
interpreter = PythonInterpreter.from_env(python_interpreter) | ||
if interpreter is None: | ||
die('Failed to find interpreter: %s' % options.python) | ||
die('Failed to find interpreter: %s' % python_interpreter) | ||
else: | ||
interpreter = PythonInterpreter.get() | ||
|
||
with TRACER.timed('Setting up interpreter %s' % interpreter.binary, V=2): | ||
resolve = functools.partial(resolve_interpreter, options.interpreter_cache_dir, options.repos) | ||
resolve = functools.partial(resolve_interpreter, interpreter_cache_dir, repos) | ||
|
||
# resolve setuptools | ||
interpreter = resolve(interpreter, SETUPTOOLS_REQUIREMENT) | ||
|
||
# possibly resolve wheel | ||
if interpreter and options.use_wheel: | ||
if interpreter and use_wheel: | ||
interpreter = resolve(interpreter, WHEEL_REQUIREMENT) | ||
|
||
return interpreter | ||
|
||
|
||
def build_pex(args, options, resolver_option_builder): | ||
with TRACER.timed('Resolving interpreter', V=2): | ||
interpreter = interpreter_from_options(options) | ||
def _lowest_version_interpreter(interpreters): | ||
"""Given an iterable of interpreters, return the one with the lowest version.""" | ||
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. More than an iterable. Just say list? 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. |
||
lowest = interpreters[0] | ||
for i in interpreters[1:]: | ||
lowest = lowest if lowest < i else i | ||
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 implies the interpreter objects implement 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. I considered that and attempted it, but given that I tried to add 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. Gotcha, makes sense. |
||
return lowest | ||
|
||
if interpreter is None: | ||
|
||
def build_pex(args, options, resolver_option_builder): | ||
with TRACER.timed('Resolving interpreters', V=2): | ||
interpreters = [ | ||
get_interpreter(interpreter, | ||
options.interpreter_cache_dir, | ||
options.repos, | ||
options.use_wheel) | ||
for interpreter in options.python or [None] | ||
] | ||
|
||
if not interpreters: | ||
die('Could not find compatible interpreter', CANNOT_SETUP_INTERPRETER) | ||
|
||
interpreter = _lowest_version_interpreter(interpreters) | ||
pex_builder = PEXBuilder(path=safe_mkdtemp(), interpreter=interpreter) | ||
|
||
pex_info = pex_builder.info | ||
|
@@ -515,16 +536,13 @@ def build_pex(args, options, resolver_option_builder): | |
constraints.append(r) | ||
resolvables.extend(constraints) | ||
|
||
resolver_kwargs = dict(interpreter=interpreter, platform=options.platform) | ||
|
||
if options.cache_dir: | ||
resolver = CachingResolver(options.cache_dir, options.cache_ttl, **resolver_kwargs) | ||
else: | ||
resolver = Resolver(**resolver_kwargs) | ||
|
||
with TRACER.timed('Resolving distributions'): | ||
try: | ||
resolveds = resolver.resolve(resolvables) | ||
resolveds = resolve_multi(resolvables, | ||
interpreters=interpreters, | ||
platforms=options.platform, | ||
cache=options.cache_dir, | ||
cache_ttl=options.cache_ttl) | ||
except Unsatisfiable as e: | ||
die(e) | ||
|
||
|
@@ -585,8 +603,8 @@ def main(args=None): | |
os.rename(tmp_name, options.pex_name) | ||
return 0 | ||
|
||
if options.platform != Platform.current(): | ||
log('WARNING: attempting to run PEX with differing platform!') | ||
if options.platform and Platform.current() not in options.platform: | ||
log('WARNING: attempting to run PEX with incompatible platforms!') | ||
|
||
pex_builder.freeze() | ||
|
||
|
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.
How about
[Platform.current()]
here to make the help clearer (%default
)? I'd be game for the duplicate[Platform.current()]
defaulting code inresolve_multi
if extracting adefault_platforms
function to some reasonable location for both to use seems onerous / more trouble than it's worth.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.
ah, how would you feel about just replacing the
%default
here with a literal description?the problem with defaulting to
[Platform.current()]
here is that theaction=append
mode is additive only, so without implementing a custom callback action for this option you'd end up resolving/building for the local platform every time whether or not you actually intended to.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.
Makes sense, sgtm
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.
fixed.