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

Fix 'current' platform handling. #6104

Merged
Merged
Show file tree
Hide file tree
Changes from 4 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
4 changes: 4 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,10 @@ 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: YYY
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

- 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
68 changes: 60 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,60 @@ 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):
Copy link
Member

Choose a reason for hiding this comment

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

a TODO to upstream this into pex might be good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup - this old one pex-tool/pex#511, I'll add a TODO similar to the method above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

"""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
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}:
Resolves as {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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK @CMLivingston and @cosmicexplorer - other issues elsewhere aside - this code never stood a fair chance of working since the selected interpreter was not passed.


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()])
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 XXX is fixed.
Copy link
Member

Choose a reason for hiding this comment

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

s/XXX/a ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup - just finished filing on the pex side. I'll send up a final comment-only diff with issue inks here as soon as this goes green then merge: pex-tool/pex#522

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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