From 18133f7c0a0c41c94e027ef96a729473637b805b Mon Sep 17 00:00:00 2001 From: John Sirois Date: Mon, 1 Oct 2018 13:19:20 -0400 Subject: [PATCH 1/4] Fix PEXEnvironment platform determination. Have this use pex intrinsics instead of pkg_resources which is known to report bad values on Apple-shipped inerpreters. Also fix resolver extended platform determination to support the test for the fix. Fixes #511 Fixes #523 --- pex/environment.py | 7 ++-- pex/interpreter.py | 6 ++- pex/resolver.py | 44 +++++++++++++++++++++- tests/test_environment.py | 77 ++++++++++++++++++++++++++++++++++++--- 4 files changed, 123 insertions(+), 11 deletions(-) diff --git a/pex/environment.py b/pex/environment.py index f04f4899b..492e99edb 100644 --- a/pex/environment.py +++ b/pex/environment.py @@ -120,13 +120,14 @@ def __init__(self, pex, pex_info, interpreter=None, **kw): self._interpreter = interpreter or PythonInterpreter.get() self._inherit_path = pex_info.inherit_path self._supported_tags = [] + + platform = Platform.current() super(PEXEnvironment, self).__init__( search_path=[] if pex_info.inherit_path == 'false' else sys.path, + platform=platform.platform, **kw ) - self._supported_tags.extend( - Platform.create(self.platform).supported_tags(self._interpreter) - ) + self._supported_tags.extend(platform.supported_tags(self._interpreter)) TRACER.log( 'E: tags for %r x %r -> %s' % (self.platform, self._interpreter, self._supported_tags), V=9 diff --git a/pex/interpreter.py b/pex/interpreter.py index 6da98e8bc..4b3226b57 100644 --- a/pex/interpreter.py +++ b/pex/interpreter.py @@ -261,8 +261,10 @@ def __str__(self): ) def __repr__(self): - return 'PythonIdentity(%r, %s, %s, %s)' % ( - self._interpreter, + return 'PythonIdentity(%r, %r, %r, %r, %r, %r)' % ( + self.abbr_impl, + self.abi_tag, + self.impl_ver, self._version[0], self._version[1], self._version[2] diff --git a/pex/resolver.py b/pex/resolver.py index 03c9ad868..c2ac8aee4 100644 --- a/pex/resolver.py +++ b/pex/resolver.py @@ -164,10 +164,52 @@ class Resolver(object): class Error(Exception): pass + @staticmethod + def _expand_and_maybe_adjust_platform(interpreter, platform=None): + # Adjusts `platform` if it is 'current' and does not match the given `interpreter` platform. + cur_plat = Platform.current() + + given_platform = Platform.create(platform or 'current') + if cur_plat.platform != given_platform.platform: + # IE: Say we're on OSX and platform was 'linux-x86_64' or 'linux_x86_64-cp-27-cp27mu'. + return given_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. + adjusted_platform = Platform(platform=cur_plat.platform, + impl=ii.abbr_impl, + version=ii.impl_ver, + abi=ii.abi_tag) + + TRACER.log(""" + 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=given_platform, + current_platform=cur_plat, + current_interpreter=PythonInterpreter.get(), + given_interpreter=interpreter, + calculated_platform=adjusted_platform), + V=9 + ) + + return adjusted_platform + def __init__(self, allow_prereleases=None, interpreter=None, platform=None, pkg_blacklist=None, use_manylinux=None): self._interpreter = interpreter or PythonInterpreter.get() - self._platform = Platform.create(platform) if platform else Platform.current() + self._platform = self._expand_and_maybe_adjust_platform(self._interpreter, platform) self._allow_prereleases = allow_prereleases self._blacklist = pkg_blacklist.copy() if pkg_blacklist else {} self._supported_tags = self._platform.supported_tags( diff --git a/tests/test_environment.py b/tests/test_environment.py index adf06e4b1..24da1f5a0 100644 --- a/tests/test_environment.py +++ b/tests/test_environment.py @@ -1,23 +1,36 @@ # Copyright 2014 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). - import os +import platform +import subprocess from contextlib import contextmanager +import pytest from twitter.common.contextutil import temporary_dir +from pex import resolver from pex.compatibility import nested from pex.environment import PEXEnvironment +from pex.installer import EggInstaller, WheelInstaller +from pex.interpreter import PythonInterpreter +from pex.package import EggPackage, SourcePackage, WheelPackage +from pex.pex import PEX from pex.pex_builder import PEXBuilder from pex.pex_info import PexInfo from pex.testing import make_bdist, temporary_filename +from pex.version import SETUPTOOLS_REQUIREMENT, WHEEL_REQUIREMENT @contextmanager -def yield_pex_builder(zip_safe=True): - with nested(temporary_dir(), make_bdist('p1', zipped=True, zip_safe=zip_safe)) as (td, p1): - pb = PEXBuilder(path=td) - pb.add_egg(p1.location) +def yield_pex_builder(zip_safe=True, installer_impl=EggInstaller, interpreter=None): + with nested(temporary_dir(), + make_bdist('p1', + zipped=True, + zip_safe=zip_safe, + installer_impl=installer_impl, + interpreter=interpreter)) as (td, p1): + pb = PEXBuilder(path=td, interpreter=interpreter) + pb.add_dist_location(p1.location) yield pb @@ -95,3 +108,57 @@ def test_load_internal_cache_unzipped(): assert len(dists) == 1 assert normalize(dists[0].location).startswith( normalize(os.path.join(pb.path(), pb.info.internal_cache))) + + +_KNOWN_BAD_APPLE_INTERPRETER = ('/System/Library/Frameworks/Python.framework/Versions/' + '2.7/Resources/Python.app/Contents/MacOS/Python') + + +@pytest.mark.skipif(not os.path.exists(_KNOWN_BAD_APPLE_INTERPRETER), + reason='Test requires known bad Apple interpreter {}' + .format(_KNOWN_BAD_APPLE_INTERPRETER)) +def test_osx_platform_intel_issue_523(): + def bad_interpreter(include_site_extras=True): + return PythonInterpreter.from_binary(_KNOWN_BAD_APPLE_INTERPRETER, + include_site_extras=include_site_extras) + + interpreter = bad_interpreter(include_site_extras=False) + with temporary_dir() as cache: + # We need to run the bad interpreter with a modern, non-Apple-Extras setuptools in order to + # successfully install psutil. + for requirement in (SETUPTOOLS_REQUIREMENT, WHEEL_REQUIREMENT): + for dist in resolver.resolve([requirement], + cache=cache, + # We can't use wheels since we're bootstrapping them. + precedence=(SourcePackage, EggPackage), + interpreter=interpreter): + interpreter = interpreter.with_extra(dist.key, dist.version, dist.location) + + with nested(yield_pex_builder(installer_impl=WheelInstaller, interpreter=interpreter), + temporary_filename()) as (pb, pex_file): + for dist in resolver.resolve(['psutil==5.4.3'], + cache=cache, + precedence=(SourcePackage, WheelPackage), + interpreter=interpreter): + pb.add_dist_location(dist.location) + pb.build(pex_file) + + # NB: We want PEX to find the bare bad interpreter at runtime. + pex = PEX(pex_file, interpreter=bad_interpreter()) + args = ['-c', 'import pkg_resources; print(pkg_resources.get_supported_platform())'] + env = os.environ.copy() + env['PEX_VERBOSE'] = '1' + process = pex.run(args=args, + env=env, + blocking=False, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE) + stdout, stderr = process.communicate() + assert 0 == process.returncode, ( + 'Process failed with exit code {} and stderr:\n{}'.format(process.returncode, stderr) + ) + + # Verify this all worked under the previously problematic pkg_resources-reported platform. + release, _, _ = platform.mac_ver() + major_minor = '.'.join(release.split('.')[:2]) + assert 'macosx-{}-intel'.format(major_minor) == stdout.strip() From cf1d0b1114aab035d81db28ec0086ac77b5bfc29 Mon Sep 17 00:00:00 2001 From: John Sirois Date: Wed, 3 Oct 2018 11:50:11 -0400 Subject: [PATCH 2/4] Fix test assertion for py3 compat. --- tests/test_environment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_environment.py b/tests/test_environment.py index 24da1f5a0..bdc855918 100644 --- a/tests/test_environment.py +++ b/tests/test_environment.py @@ -161,4 +161,4 @@ def bad_interpreter(include_site_extras=True): # Verify this all worked under the previously problematic pkg_resources-reported platform. release, _, _ = platform.mac_ver() major_minor = '.'.join(release.split('.')[:2]) - assert 'macosx-{}-intel'.format(major_minor) == stdout.strip() + assert b'macosx-{}-intel'.format(major_minor) == stdout.strip() From c9f06187d263219ec92186dded88e15b6b9c6bc8 Mon Sep 17 00:00:00 2001 From: John Sirois Date: Wed, 3 Oct 2018 11:57:16 -0400 Subject: [PATCH 3/4] Use more explicit variable names. --- pex/environment.py | 5 ++++- pex/resolver.py | 11 ++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/pex/environment.py b/pex/environment.py index 492e99edb..9f9c056c0 100644 --- a/pex/environment.py +++ b/pex/environment.py @@ -122,9 +122,12 @@ def __init__(self, pex, pex_info, interpreter=None, **kw): self._supported_tags = [] platform = Platform.current() + platform_name = platform.platform super(PEXEnvironment, self).__init__( search_path=[] if pex_info.inherit_path == 'false' else sys.path, - platform=platform.platform, + # NB: Our pkg_resources.Environment base-class wants the platform name string and not the + # pex.platform.Platform object. + platform=platform_name, **kw ) self._supported_tags.extend(platform.supported_tags(self._interpreter)) diff --git a/pex/resolver.py b/pex/resolver.py index c2ac8aee4..68955807f 100644 --- a/pex/resolver.py +++ b/pex/resolver.py @@ -174,8 +174,9 @@ def _expand_and_maybe_adjust_platform(interpreter, platform=None): # IE: Say we're on OSX and platform was 'linux-x86_64' or 'linux_x86_64-cp-27-cp27mu'. return given_platform - ii = interpreter.identity - if (ii.abbr_impl, ii.impl_ver, ii.abi_tag) == (cur_plat.impl, cur_plat.version, cur_plat.abi): + if (interpreter.identity.abbr_impl, + interpreter.identity.impl_ver, + interpreter.identity.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. @@ -184,9 +185,9 @@ def _expand_and_maybe_adjust_platform(interpreter, platform=None): # Otherwise we need to adjust the platform to match a local interpreter different from the # currently executing interpreter. adjusted_platform = Platform(platform=cur_plat.platform, - impl=ii.abbr_impl, - version=ii.impl_ver, - abi=ii.abi_tag) + impl=interpreter.identity.abbr_impl, + version=interpreter.identity.impl_ver, + abi=interpreter.identity.abi_tag) TRACER.log(""" Modifying given platform of {given_platform!r}: From f72cc11ab86d7508338515bbe1ea6b797b774daa Mon Sep 17 00:00:00 2001 From: John Sirois Date: Wed, 3 Oct 2018 13:30:16 -0400 Subject: [PATCH 4/4] Simplify and clarify resolver platform expansion. --- pex/resolver.py | 87 +++++++++++++++++++++------------------ tests/test_environment.py | 4 +- 2 files changed, 48 insertions(+), 43 deletions(-) diff --git a/pex/resolver.py b/pex/resolver.py index 68955807f..c86759208 100644 --- a/pex/resolver.py +++ b/pex/resolver.py @@ -165,52 +165,57 @@ class Resolver(object): class Error(Exception): pass @staticmethod - def _expand_and_maybe_adjust_platform(interpreter, platform=None): - # Adjusts `platform` if it is 'current' and does not match the given `interpreter` platform. - cur_plat = Platform.current() - - given_platform = Platform.create(platform or 'current') - if cur_plat.platform != given_platform.platform: - # IE: Say we're on OSX and platform was 'linux-x86_64' or 'linux_x86_64-cp-27-cp27mu'. - return given_platform - - if (interpreter.identity.abbr_impl, - interpreter.identity.impl_ver, - interpreter.identity.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. - adjusted_platform = Platform(platform=cur_plat.platform, - impl=interpreter.identity.abbr_impl, - version=interpreter.identity.impl_ver, - abi=interpreter.identity.abi_tag) - - TRACER.log(""" - 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=given_platform, - current_platform=cur_plat, - current_interpreter=PythonInterpreter.get(), - given_interpreter=interpreter, - calculated_platform=adjusted_platform), - V=9 - ) + def _maybe_expand_platform(interpreter, platform=None): + # Expands `platform` if it is 'current' and abbreviated. + # + # IE: If we're on linux and handed a platform of `None`, 'current', or 'linux_x86_64', we expand + # the platform to an extended platform matching the given interpreter's abi info, eg: + # 'linux_x86_64-cp-27-cp27mu'. - return adjusted_platform + cur_plat = Platform.current() + def expand_platform(): + expanded_platform = Platform(platform=cur_plat.platform, + impl=interpreter.identity.abbr_impl, + version=interpreter.identity.impl_ver, + abi=interpreter.identity.abi_tag) + TRACER.log(""" +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=PythonInterpreter.get(), + given_interpreter=interpreter, + calculated_platform=expanded_platform), + V=9 + ) + return expanded_platform + + if platform in (None, 'current'): + # Always expand the default local (abbreviated) platform to the given interpreter. + return expand_platform() + else: + given_platform = Platform.create(platform) + if given_platform.is_extended: + # Always respect an explicit extended platform. + return given_platform + elif given_platform.platform != cur_plat.platform: + # IE: Say we're on OSX and platform was 'linux-x86_64'; we can't expand a non-local + # platform so we leave as-is. + return given_platform + else: + # IE: Say we're on 64 bit linux and platform was 'linux-x86_64'; ie: the abbreviated local + # platform. + return expand_platform() def __init__(self, allow_prereleases=None, interpreter=None, platform=None, pkg_blacklist=None, use_manylinux=None): self._interpreter = interpreter or PythonInterpreter.get() - self._platform = self._expand_and_maybe_adjust_platform(self._interpreter, platform) + self._platform = self._maybe_expand_platform(self._interpreter, platform) self._allow_prereleases = allow_prereleases self._blacklist = pkg_blacklist.copy() if pkg_blacklist else {} self._supported_tags = self._platform.supported_tags( diff --git a/tests/test_environment.py b/tests/test_environment.py index bdc855918..00479eda7 100644 --- a/tests/test_environment.py +++ b/tests/test_environment.py @@ -9,7 +9,7 @@ from twitter.common.contextutil import temporary_dir from pex import resolver -from pex.compatibility import nested +from pex.compatibility import nested, to_bytes from pex.environment import PEXEnvironment from pex.installer import EggInstaller, WheelInstaller from pex.interpreter import PythonInterpreter @@ -161,4 +161,4 @@ def bad_interpreter(include_site_extras=True): # Verify this all worked under the previously problematic pkg_resources-reported platform. release, _, _ = platform.mac_ver() major_minor = '.'.join(release.split('.')[:2]) - assert b'macosx-{}-intel'.format(major_minor) == stdout.strip() + assert to_bytes('macosx-{}-intel'.format(major_minor)) == stdout.strip()