Skip to content

Commit

Permalink
Fix 'current' platform handling. (#6104)
Browse files Browse the repository at this point in the history
Previously the platform passed for 'current' was partial, leading to
ambiguous resolves. Further, partial platforms in user's BUILD files
also would lead to ambiguous resolves. For example, a multi-platform
`python_binary` with something like
`platforms=['current', 'linux-x86_64', 'macosx_10.11_x86_64']`.

Expand and and then fixup platforms as required by replacing
`get_local_platform` with `expand_and_maybe_adjust_platform`.

Also perform a few `build_local_python_distributions` fixes.
+ Fix an unused `interpreter` parameter - forward to `SetupPyRunner`.
+ Kill declaring `--universal` on behalf of the package author. There
  is not enough info to make the decision if the code being dist'd, if
  pure python, is 2/3 compatible.

To compensate for the removal of `--universal`, give an example of how
to declare your python_dist as `--universal` when you know it is as the
BUILD `python_dist` target author.

Finally two subsidary workarounds:
+ hack around PEX not forwarding custom interpreters to PEXEnvironment.
+ Avoid the troublesome Apple 2.7.10 python for now.

Hacks are tracked by umbrella #5922, but for this PR they are:
+ pex-tool/pex#511
+ pex-tool/pex#522
+ pex-tool/pex#523
  • Loading branch information
jsirois authored Jul 13, 2018
1 parent 5da9146 commit a1696c8
Show file tree
Hide file tree
Showing 14 changed files with 121 additions and 45 deletions.
5 changes: 5 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,11 @@ matrix:
language: generic
env:
- SHARD="Rust + Platform-specific Tests OSX"
# Specifically avoid the OSX provided 2.7.10 under xcode8.3 since it returns a platform
# of `macosx-*-intel` where the `intel` suffix is bogus but pex has not yet been taught to
# deal with this. Can be removed when this issue is resolved:
# https://github.com/pantsbuild/pex/issues/523
- PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython>2.7.10,<3']"
before_install:
- brew tap caskroom/cask && brew update && brew cask install osxfuse
before_script:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ def _run_mypy(self, py3_interpreter, mypy_args, **kwargs):
path = os.path.realpath(os.path.join(self.workdir, str(py3_interpreter.identity), mypy_version))
if not os.path.isdir(path):
self.merge_pexes(path, pex_info, py3_interpreter, [mypy_requirement_pex])
pex = WrappedPEX(PEX(path, py3_interpreter), py3_interpreter)
pex = WrappedPEX(PEX(path, py3_interpreter))
return pex.run(mypy_args, **kwargs)

def execute(self):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ def _compile_target(self, vt):

exec_pex = PEX(exec_pex_path, interpreter)
extra_pex_paths = [pex.path() for pex in filter(None, [reqs_pex, srcs_pex])]
pex = WrappedPEX(exec_pex, interpreter, extra_pex_paths)
pex = WrappedPEX(exec_pex, extra_pex_paths)

with self.context.new_workunit(name='eval',
labels=[WorkUnitLabel.COMPILER, WorkUnitLabel.RUN,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ python_dist(
sources=[
'hello_package/hello.py',
'hello_package/__init__.py',
'setup.cfg',
'setup.py'
],
setup_requires=[
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[bdist_wheel]
universal=1
8 changes: 6 additions & 2 deletions src/python/pants/backend/python/interpreter_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from pex.resolver import resolve
from pex.variables import Variables

from pants.backend.python.pex_util import create_bare_interpreter, get_local_platform
from pants.backend.python.pex_util import create_bare_interpreter, expand_and_maybe_adjust_platform
from pants.backend.python.targets.python_target import PythonTarget
from pants.base.exceptions import TaskError
from pants.process.lock import OwnerPrintingInterProcessFileLock
Expand Down Expand Up @@ -222,7 +222,11 @@ def _resolve_and_link(self, interpreter, requirement, target_link):
distributions = resolve(requirements=[requirement],
fetchers=self._python_repos.get_fetchers(),
interpreter=interpreter,
platform=get_local_platform(),
platform=expand_and_maybe_adjust_platform(
interpreter=interpreter,
# The local interpreter cache is, by definition, composed of
# interpreters for the 'current' platform.
platform='current'),
context=self._python_repos.get_network_context(),
precedence=precedence)
if not distributions:
Expand Down
69 changes: 61 additions & 8 deletions src/python/pants/backend/python/pex_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,15 @@

from __future__ import absolute_import, division, print_function, unicode_literals

import logging

from pex.interpreter import PythonInterpreter
from pex.platforms import Platform


logger = logging.getLogger(__name__)


def create_bare_interpreter(binary_path):
"""Creates an interpreter for python binary at the given path.
Expand All @@ -22,13 +27,61 @@ def create_bare_interpreter(binary_path):
return PythonInterpreter(binary_path, interpreter_with_extras.identity, extras=None)


def get_local_platform():
"""Returns the name of the local platform; eg: 'linux_x86_64' or 'macosx_10_8_x86_64'.
def _interpreter_str(interp):
ident = interp.identity
return ('PythonInterpreter({binary!r}, {identity!r} with extended info: '
'(abbr_impl: {abbr_impl!r}, impl_ver: {impl_ver!r}, abi_tag: {abi_tag!r}))'
.format(binary=interp.binary,
identity=ident,
abbr_impl=ident.abbr_impl,
impl_ver=ident.impl_ver,
abi_tag=ident.abi_tag))


def expand_and_maybe_adjust_platform(interpreter, platform):
"""Adjusts `platform` if it is 'current' and does not match the given `interpreter` platform.
:returns: The local platform name.
:rtype: str
:param interpreter: The target interpreter for the given `platform`.
:type interpreter: :class:`pex.interpreter.PythonInterpreter`
:param platform: The platform name to expand and maybe adjust.
:type platform: text
:returns: The `platform`, potentially adjusted.
:rtype: :class:`pex.platforms.Platform`
"""
# TODO(John Sirois): Kill some or all usages when https://github.com/pantsbuild/pex/issues/511
# is fixed.
current_platform = Platform.current()
return current_platform.platform
# TODO(John Sirois): Kill all usages when https://github.com/pantsbuild/pex/issues/511 is fixed.
cur_plat = Platform.current()

if cur_plat.platform != Platform.create(platform).platform:
# IE: Say we're on OSX and platform was 'linux-x86_64' or 'linux_x86_64-cp-27-cp27mu'.
return Platform.create(platform)

ii = interpreter.identity
if (ii.abbr_impl, ii.impl_ver, ii.abi_tag) == (cur_plat.impl, cur_plat.version, cur_plat.abi):
# IE: Say we're on Linux and platform was 'current' or 'linux-x86_64' or
# 'linux_x86_64-cp-27-cp27mu'and the current extended platform info matches the given
# interpreter exactly.
return cur_plat

# Otherwise we need to adjust the platform to match a local interpreter different from the
# currently executing interpreter.
interpreter_platform = Platform(platform=cur_plat.platform,
impl=ii.abbr_impl,
version=ii.impl_ver,
abi=ii.abi_tag)

logger.debug("""
Modifying given platform of {given_platform!r}:
Using the current platform of {current_platform!r}
Under current interpreter {current_interpreter!r}
To match given interpreter {given_interpreter!r}.
Calculated platform: {calculated_platform!r}""".format(
given_platform=platform,
current_platform=cur_plat,
current_interpreter=_interpreter_str(PythonInterpreter.get()),
given_interpreter=_interpreter_str(interpreter),
calculated_platform=interpreter_platform)
)

return interpreter_platform
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,8 @@ def _prepare_and_create_dist(self, interpreter, shared_libs_product, versioned_t
'Installing setup requirements: {}\n\n'
.format([req.key for req in setup_reqs_to_resolve]))

cur_platforms = ['current'] if is_platform_specific else None

setup_requires_site_dir = ensure_setup_requires_site_dir(
setup_reqs_to_resolve, interpreter, setup_requires_dir, platforms=cur_platforms)
setup_reqs_to_resolve, interpreter, setup_requires_dir, platforms=['current'])
if setup_requires_site_dir:
self.context.log.debug('Setting PYTHONPATH with setup_requires site directory: {}'
.format(setup_requires_site_dir))
Expand All @@ -226,8 +224,9 @@ def _create_dist(self, dist_tgt, dist_target_dir, interpreter,
self._copy_sources(dist_tgt, dist_target_dir)

setup_runner = SetupPyRunner.for_bdist_wheel(
dist_target_dir,
is_platform_specific=is_platform_specific)
source_dir=dist_target_dir,
is_platform_specific=is_platform_specific,
interpreter=interpreter)

with environment_as(**setup_py_execution_environment.as_environment()):
# Build a whl using SetupPyRunner and return its absolute path.
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/backend/python/tasks/pex_build_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from pex.resolver import resolve
from twitter.common.collections import OrderedSet

from pants.backend.python.pex_util import get_local_platform
from pants.backend.python.pex_util import expand_and_maybe_adjust_platform
from pants.backend.python.subsystems.python_repos import PythonRepos
from pants.backend.python.subsystems.python_setup import PythonSetup
from pants.backend.python.targets.python_binary import PythonBinary
Expand Down Expand Up @@ -155,7 +155,7 @@ def resolve_multi(interpreter, requirements, platforms, find_links):
requirements=[req.requirement for req in requirements],
interpreter=interpreter,
fetchers=fetchers,
platform=get_local_platform() if platform == 'current' else platform,
platform=expand_and_maybe_adjust_platform(interpreter=interpreter, platform=platform),
context=python_repos.get_network_context(),
cache=requirements_cache_dir,
cache_ttl=python_setup.resolver_cache_ttl,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,4 @@ def create_pex(self, pex_info=None):
extra_file.add_to(builder)
builder.freeze()

return WrappedPEX(PEX(path, interpreter), interpreter)
return WrappedPEX(PEX(path, interpreter))
6 changes: 2 additions & 4 deletions src/python/pants/backend/python/tasks/setup_py.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from builtins import map, object, str, zip
from collections import OrderedDict, defaultdict

from pex import pep425tags
from pex.compatibility import string, to_bytes
from pex.installer import InstallerBase, Packager
from pex.interpreter import PythonInterpreter
Expand All @@ -21,7 +22,6 @@
from wheel.install import WheelFile

from pants.backend.native.config.environment import CCompiler, CppCompiler, Linker, Platform
from pants.backend.python.pex_util import get_local_platform
from pants.backend.python.targets.python_binary import PythonBinary
from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
from pants.backend.python.targets.python_target import PythonTarget
Expand Down Expand Up @@ -63,9 +63,7 @@ class SetupPyRunner(InstallerBase):
def for_bdist_wheel(cls, source_dir, is_platform_specific, **kw):
cmd = ['bdist_wheel']
if is_platform_specific:
cmd.extend(['--plat-name', get_local_platform()])
else:
cmd.append('--universal')
cmd.extend(['--plat-name', pep425tags.get_platform()])

This comment has been minimized.

Copy link
@jsirois

jsirois Jul 13, 2018

Author Contributor

@cosmicexplorer and @CMLivingston this really deserves review from you two.
The compensation can be seen in:

Let me know if I was missing something here re --universal application in SetupPyRunner.

cmd.extend(['--dist-dir', cls.DIST_DIR])
return cls(source_dir, cmd, **kw)

Expand Down
47 changes: 29 additions & 18 deletions src/python/pants/backend/python/tasks/wrapped_pex.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,57 +4,68 @@

from __future__ import absolute_import, division, print_function, unicode_literals

import logging
from builtins import object
from copy import copy


logger = logging.getLogger(__name__)


class WrappedPEX(object):
"""Wrapper around a PEX that exposes only its run() method.
Allows us to set the PEX_PATH in the environment when running.
"""

_PEX_PATH_ENV_VAR_NAME = 'PEX_PATH'
_PEX_PYTHON_PATH_ENV_VAR_NAME = 'PEX_PYTHON_PATH'

# TODO(benjy): I think we can get rid of the interpreter argument.
# In all cases it appears to be set to pex.interpreter.
def __init__(self, pex, interpreter, extra_pex_paths=None):
def __init__(self, pex, extra_pex_paths=None):
"""
:param pex: The main pex we wrap.
:param interpreter: The interpreter the main pex will run on.
:param extra_pex_paths: Other pexes, to "merge" in via the PEX_PATH mechanism.
"""
self._pex = pex
self._interpreter = interpreter
self._extra_pex_paths = extra_pex_paths

@property
def interpreter(self):
return self._interpreter
return self._pex._interpreter

def path(self):
return self._pex.path()

def cmdline(self, args=()):
cmdline = ' '.join(self._pex.cmdline(args))

def render_env_var(key, value):
return '{key}={value}'.format(key=key, value=value)

env_vars = [(self._PEX_PYTHON_PATH_ENV_VAR_NAME, self._pex._interpreter.binary)]

pex_path = self._pex_path()
if pex_path:
return '{env_var_name}={pex_path} {cmdline}'.format(env_var_name=self._PEX_PATH_ENV_VAR_NAME,
pex_path=pex_path,
cmdline=cmdline)
else:
return cmdline
env_vars.append((self._PEX_PATH_ENV_VAR_NAME, pex_path))

return '{execution_control_env_vars} {cmdline}'.format(
execution_control_env_vars=' '.join(render_env_var(k, v) for k, v in env_vars),
cmdline=cmdline
)

def run(self, *args, **kwargs):
env = copy(kwargs.pop('env', {}))

# Hack around bug in PEX where custom interpreters are not forwarded to PEXEnvironments.
# TODO(John Sirois): Remove when https://github.com/pantsbuild/pex/issues/522 is fixed.
env[self._PEX_PYTHON_PATH_ENV_VAR_NAME] = self._pex._interpreter.binary

pex_path = self._pex_path()
if pex_path:
kwargs_copy = copy(kwargs)
env = copy(kwargs_copy.get('env')) if 'env' in kwargs_copy else {}
env[self._PEX_PATH_ENV_VAR_NAME] = self._pex_path()
kwargs_copy['env'] = env
return self._pex.run(*args, **kwargs_copy)
else:
return self._pex.run(*args, **kwargs)
env[self._PEX_PATH_ENV_VAR_NAME] = pex_path

logger.debug('Executing WrappedPEX using: {}'.format(self.cmdline(args=tuple(*args))))
return self._pex.run(*args, env=env, **kwargs)

def _pex_path(self):
if self._extra_pex_paths:
Expand Down
4 changes: 3 additions & 1 deletion tests/python/pants_test/backend/python/tasks/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,12 @@ python_tests(
sources=globs('*_integration.py', exclude=python_native_code_test_files),
dependencies=[
'3rdparty/python:pex',
'src/python/pants/base:build_environment',
'src/python/pants/util:contextutil',
'src/python/pants/util:process_handler',
'tests/python/pants_test/backend/python:pants_requirement_integration_test_base',
'tests/python/pants_test:int-test',
'tests/python/pants_test/backend/python:pants_requirement_integration_test_base',
'tests/python/pants_test/backend/python/tasks:python_task_test_base',
],
tags={'integration'},
timeout=2400
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
from builtins import map
from textwrap import dedent

from pants.backend.python.pex_util import get_local_platform
from pex import pep425tags

from pants.backend.python.register import build_file_aliases as register_python
from pants.build_graph.address import Address
from pants_test.backend.python.tasks.interpreter_cache_test_mixin import InterpreterCacheTestMixin
Expand All @@ -34,7 +35,7 @@ def name_and_platform(whl):

def check_wheel_platform_matches_host(wheel_dist):
_, _, wheel_platform = name_and_platform(wheel_dist)
return wheel_platform == normalize_platform_tag(get_local_platform())
return wheel_platform == normalize_platform_tag(pep425tags.get_platform())


class PythonTaskTestBase(InterpreterCacheTestMixin, TaskTestBase):
Expand Down

0 comments on commit a1696c8

Please sign in to comment.