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

add the target fingerprint to the version of each local dist so that we don't use the first cached one #6022

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b4c2eef
fix local dist caching
cosmicexplorer Jun 25, 2018
ad30c54
expand comments
cosmicexplorer Jun 26, 2018
443aa29
add a test that fails on master but passes here
cosmicexplorer Jun 26, 2018
f90c295
update the error message and docstring in the python_dist() ctor
cosmicexplorer Jun 26, 2018
af6d90f
add the swanky new version to all our local dists
cosmicexplorer Jun 26, 2018
36d5174
use the egg_info build tag flags to avoid having to change any setup.…
cosmicexplorer Jun 28, 2018
7efdbfb
fix the version string checking for local dist creation to add the no…
cosmicexplorer Jun 29, 2018
f4b5156
fix up testing to conform to the new snapshot versioning for local dists
cosmicexplorer Jun 29, 2018
a3b9610
.......add a missing import
cosmicexplorer Jul 12, 2018
bd0603d
add the necessary snapshot version to the SetupPyRunner invocation
cosmicexplorer Jul 14, 2018
bb26f29
remove unused import
cosmicexplorer Jul 15, 2018
87b3524
revert unnecessary changes
cosmicexplorer Jul 15, 2018
3a199e5
keep version of ctypes_test testproject at 0.0.1 for now
cosmicexplorer Jul 15, 2018
fd322dd
rename fasthello_install_requires field
cosmicexplorer Jul 15, 2018
d877cf8
use self.mock_buildroot() for test_invalidation()
cosmicexplorer Jul 15, 2018
5bbc660
remove unused methods
cosmicexplorer Jul 15, 2018
e2eda1c
rename mock_buildroot() `dir` field to `new_buildroot`
cosmicexplorer Jul 15, 2018
810f8b8
add docstring for test_invalidation
cosmicexplorer Jul 15, 2018
57093ee
add explanatory comment
cosmicexplorer Jul 15, 2018
356659d
move setup.py argv generation into `BuildLocalPythonDistributions`
cosmicexplorer Jul 15, 2018
4eb92ae
add a couple newlines to pass lint
cosmicexplorer Jul 15, 2018
c9c4c8b
fix ctypes_test wheel version
cosmicexplorer Jul 15, 2018
7041397
reduce travis to a shadow of its former self
cosmicexplorer Jul 16, 2018
5f108db
put travis back together
cosmicexplorer Jul 16, 2018
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import c_greet
import cpp_greet


Copy link
Contributor

Choose a reason for hiding this comment

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

can leave this file untouched.

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 in a7d2f23!

def hello():
return '\n'.join([
c_greet.c_greet(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@

from __future__ import absolute_import, division, print_function, unicode_literals


Copy link
Contributor

Choose a reason for hiding this comment

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

can leave this file untouched.

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 in a7d2f23!

def hello():
return 'Hello, world!'
40 changes: 14 additions & 26 deletions src/python/pants/backend/python/targets/python_distribution.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,15 @@

from __future__ import absolute_import, division, print_function, unicode_literals

from builtins import str
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove this? Is it unused now?

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Jul 16, 2018

Choose a reason for hiding this comment

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

Yes, because the str() call was only in the reimplementation of the PythonTarget constructor that was removed in favor of subclassing PythonTarget -- this line was causing the lint shard to fail. I was actually meaning to ask you about how to handle this in the future -- is there a linter for python 3 compat that would check this? I could probably write one from the tools in contrib/python if that would help to ensure the incorrect str() doesn't creep back in, I did something like that once (then removed it) when I got a bit overeager in a PR a while ago.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, sounds good.

I don't know of any linter that checks for use of the future library. PyLint has a mode for checking Py3 incompatibilities, but it's not checking for use of future, as I understand.

We also unfortunately can't start running futurize on every file committed, because it makes over-optimizations.

The ideal linter would check for any usages of str, map, filter, bytes, zip, range, object and other builtins, and make sure that a corresponding from builtins import x statement is used. That way we always are using Py3 syntax. That could be an easy win.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made into #6157!


from pex.interpreter import PythonIdentity
from twitter.common.collections import maybe_list

from pants.backend.python.targets.python_target import PythonTarget
from pants.base.exceptions import TargetDefinitionException
from pants.base.payload import Payload
from pants.base.payload_field import PrimitiveField
from pants.build_graph.target import Target


class PythonDistribution(Target):
class PythonDistribution(PythonTarget):
"""A Python distribution target that accepts a user-defined setup.py."""

default_sources_globs = '*.py'
Expand All @@ -28,7 +25,6 @@ def __init__(self,
address=None,
payload=None,
sources=None,
compatibility=None,
setup_requires=None,
**kwargs):
"""
Expand All @@ -39,31 +35,23 @@ def __init__(self,
:type payload: :class:`pants.base.payload.Payload`
:param sources: Files to "include". Paths are relative to the
BUILD file's directory.
:type sources: ``Fileset`` or list of strings. Must include setup.py.
:param compatibility: either a string or list of strings that represents
interpreter compatibility for this target, using the Requirement-style
format, e.g. ``'CPython>=3', or just ['>=2.7','<3']`` for requirements
agnostic to interpreter class.
:type sources: :class:`twitter.common.dirutil.Fileset` or list of strings. Must include
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Given that this changes the behaviour of sources, you might want to merge from master and re-test post #5908

setup.py.
:param list setup_requires: A list of python requirements to provide during the invocation of
setup.py.
"""
if not 'setup.py' in sources:
Copy link
Contributor

@CMLivingston CMLivingston Jun 26, 2018

Choose a reason for hiding this comment

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

Why strip these out? Are you planning on adding setup.py checking back or is it happening elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be the exact same as the previous target, just without unnecessarily repeated code, which I think should be a goal. It doesn't seem like too gross of a refactoring because it's limited to a single file. We call the superclass constructor (PythonTarget), which already does all of the checking that was removed from this file.

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Jun 26, 2018

Choose a reason for hiding this comment

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

I think I remember vaguely that we don't subclass PythonTarget due to some method in pex_build_util.py using PythonTarget to mean something that we don't want to include python_dist() in. I don't know if that is still the case with the changes to that file. Will look into and leave a comment if this is ok or revert the change / do something else if not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed on the redundancy, but I think that PythonTarget will not check for setup.py presence as of right now, so it would be good to add that back in to this one.

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Jun 28, 2018

Choose a reason for hiding this comment

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

I don't understand -- we are still checking if setup.py is in sources, just doing it before we initialize anything else. I can move it back to after the super constructor call -- it's not clear which ordering would be desired here, I just usually try to do argument validation first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, weird, I must have read this wrong. This looks good to me regarding setup.py checking.

raise TargetDefinitionException(
self,
'A file named setup.py must be in the same '
'directory as the BUILD file containing this target.')

payload = payload or Payload()
payload.add_fields({
'sources': self.create_sources_field(sources, address.spec_path, key_arg='sources'),
'compatibility': PrimitiveField(maybe_list(compatibility or ())),
'setup_requires': PrimitiveField(maybe_list(setup_requires or ()))
})
super(PythonDistribution, self).__init__(address=address, payload=payload, **kwargs)

if not 'setup.py' in sources:
raise TargetDefinitionException(
self, 'A setup.py in the top-level directory relative to the target definition is required.'
)

# Check that the compatibility requirements are well-formed.
for req in self.payload.compatibility:
try:
PythonIdentity.parse_requirement(req)
except ValueError as e:
raise TargetDefinitionException(self, str(e))
super(PythonDistribution, self).__init__(
address=address, payload=payload, sources=sources, **kwargs)

@property
def has_native_sources(self):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import re
import shutil

from pex import pep425tags
from pex.interpreter import PythonInterpreter

from pants.backend.native.targets.native_library import NativeLibrary
Expand All @@ -34,6 +35,16 @@ class BuildLocalPythonDistributions(Task):

options_scope = 'python-create-distributions'

# NB: these are all the immediate subdirectories of the target's results directory.
# This contains any modules from a setup_requires().
_SETUP_REQUIRES_SITE_SUBDIR = 'setup_requires_site'
# This will contain the sources used to build the python_dist().
_DIST_SOURCE_SUBDIR = 'python_dist_subdir'

# This defines the output directory when building the dist, so we know where the output wheel is
# located. It is a subdirectory of `_DIST_SOURCE_SUBDIR`.
_DIST_OUTPUT_DIR = 'dist'

@classmethod
def product_types(cls):
# Note that we don't actually place the products in the product map. We stitch
Expand Down Expand Up @@ -92,19 +103,13 @@ def _get_setup_requires_to_resolve(self, dist_target):

return reqs_to_resolve

# NB: these are all the immediate subdirectories of the target's results directory.
# This contains any modules from a setup_requires().
setup_requires_site_subdir = 'setup_requires_site'
# This will contain the sources used to build the python_dist().
dist_subdir = 'python_dist_subdir'

@classmethod
def _get_output_dir(cls, results_dir):
return os.path.join(results_dir, cls.dist_subdir)
return os.path.join(results_dir, cls._DIST_SOURCE_SUBDIR)

@classmethod
def _get_dist_dir(cls, results_dir):
return os.path.join(cls._get_output_dir(results_dir), SetupPyRunner.DIST_DIR)
return os.path.join(cls._get_output_dir(results_dir), cls._DIST_OUTPUT_DIR)

def execute(self):
dist_targets = self.context.targets(is_local_python_dist)
Expand Down Expand Up @@ -198,7 +203,7 @@ def _prepare_and_create_dist(self, interpreter, shared_libs_product, versioned_t
# We are including a platform-specific shared lib in this dist, so mark it as such.
is_platform_specific = True

setup_requires_dir = os.path.join(results_dir, self.setup_requires_site_subdir)
setup_requires_dir = os.path.join(results_dir, self._SETUP_REQUIRES_SITE_SUBDIR)
setup_reqs_to_resolve = self._get_setup_requires_to_resolve(dist_target)
if setup_reqs_to_resolve:
self.context.log.debug('python_dist target(s) with setup_requires detected. '
Expand All @@ -215,17 +220,50 @@ def _prepare_and_create_dist(self, interpreter, shared_libs_product, versioned_t
setup_requires_site_dir=setup_requires_site_dir,
setup_py_native_tools=native_tools)

self._create_dist(dist_target, dist_output_dir, interpreter,
setup_py_execution_environment, is_platform_specific)
versioned_target_fingerprint = versioned_target.cache_key.hash

self._create_dist(
dist_target,
dist_output_dir,
interpreter,
setup_py_execution_environment,
versioned_target_fingerprint,
is_platform_specific)

# NB: "snapshot" refers to a "snapshot release", not a Snapshot.
def _generate_snapshot_bdist_wheel_argv(self, snapshot_fingerprint, is_platform_specific):
"""Create a command line to pass to :class:`SetupPyRunner`.

Note that distutils will convert `snapshot_fingerprint` into a string suitable for a version
tag. Currently for versioned target fingerprints, this seems to convert all punctuation into
'.' and downcase all ASCII chars. See https://www.python.org/dev/peps/pep-0440/ for further
information on allowed version names.

NB: adds a '+' before the fingerprint to the build tag!
"""
egg_info_snapshot_tag_args = ['egg_info', '--tag-build=+{}'.format(snapshot_fingerprint)]
bdist_whl_args = ['bdist_wheel']
if is_platform_specific:
platform_args = ['--plat-name', pep425tags.get_platform()]
else:
platform_args = []

dist_dir_args = ['--dist-dir', self._DIST_OUTPUT_DIR]

setup_py_command = egg_info_snapshot_tag_args + bdist_whl_args + platform_args + dist_dir_args
return setup_py_command

def _create_dist(self, dist_tgt, dist_target_dir, interpreter,
setup_py_execution_environment, is_platform_specific):
setup_py_execution_environment, snapshot_fingerprint, is_platform_specific):
"""Create a .whl file for the specified python_distribution target."""
self._copy_sources(dist_tgt, dist_target_dir)

setup_runner = SetupPyRunner.for_bdist_wheel(
setup_py_snapshot_version_argv = self._generate_snapshot_bdist_wheel_argv(
snapshot_fingerprint, is_platform_specific)

setup_runner = SetupPyRunner(
source_dir=dist_target_dir,
is_platform_specific=is_platform_specific,
setup_command=setup_py_snapshot_version_argv,
interpreter=interpreter)

with environment_as(**setup_py_execution_environment.as_environment()):
Expand Down
11 changes: 0 additions & 11 deletions src/python/pants/backend/python/tasks/setup_py.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
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 Down Expand Up @@ -57,16 +56,6 @@
class SetupPyRunner(InstallerBase):
_EXTRAS = ('setuptools', 'wheel')

DIST_DIR = 'dist'

@classmethod
def for_bdist_wheel(cls, source_dir, is_platform_specific, **kw):
cmd = ['bdist_wheel']
if is_platform_specific:
cmd.extend(['--plat-name', pep425tags.get_platform()])
cmd.extend(['--dist-dir', cls.DIST_DIR])
return cls(source_dir, cmd, **kw)

def __init__(self, source_dir, setup_command, **kw):
self.__setup_command = setup_command
super(SetupPyRunner, self).__init__(source_dir, **kw)
Expand Down
19 changes: 2 additions & 17 deletions tests/python/pants_test/backend/python/tasks/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@ python_library(
)

python_native_code_test_files = [
# TODO: This test does not have any platform-specific testing right now, but that will be
# changed when #5815 is merged.
'test_build_local_python_distributions.py',
'test_python_distribution_integration.py',
'test_ctypes_integration.py',
]

python_tests(
Expand All @@ -43,7 +42,7 @@ python_tests(
'tests/python/pants_test:int-test',
'tests/python/pants_test/engine:scheduler_test_base',
],
tags={'platform_specific_behavior'},
tags={'platform_specific_behavior', 'integration'},
timeout=2400,
)

Expand Down Expand Up @@ -99,20 +98,6 @@ python_tests(
timeout=2400
)

python_tests(
name='ctypes_integration',
sources=['test_ctypes_integration.py', 'test_python_distribution_integration.py'],
dependencies=[
'src/python/pants/base:build_environment',
'src/python/pants/util:contextutil',
'src/python/pants/util:process_handler',
'tests/python/pants_test:int-test',
'tests/python/pants_test/backend/python/tasks:python_task_test_base',
],
tags={'integration'},
timeout=2400,
)

python_tests(
name='python_isort_integration',
sources=['test_python_isort_integration.py'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from __future__ import absolute_import, division, print_function, unicode_literals

import re
from builtins import next, str
from textwrap import dedent

Expand All @@ -12,6 +13,7 @@
from pants.backend.python.targets.python_distribution import PythonDistribution
from pants.backend.python.tasks.build_local_python_distributions import \
BuildLocalPythonDistributions
from pants.util.collections import assert_single_element
from pants_test.backend.python.tasks.python_task_test_base import (PythonTaskTestBase,
check_wheel_platform_matches_host,
name_and_platform)
Expand Down Expand Up @@ -115,6 +117,27 @@ def _retrieve_single_product_at_target_base(self, product_mapping, target):
single_product = all_products[0]
return single_product

def _get_dist_snapshot_version(self, task, python_dist_target):
"""Get the target's fingerprint, and guess the resulting version string of the built dist.
Local python_dist() builds are tagged with the versioned target's fingerprint using the
--tag-build option in the egg_info command. This fingerprint string is slightly modified by
distutils to ensure a valid version string, and this method finds what that modified version
string is so we can verify that the produced local dist is being tagged with the correct
snapshot version.
The argument we pass to that option begins with a +, which is unchanged. See
https://www.python.org/dev/peps/pep-0440/ for further information.
"""
with task.invalidated([python_dist_target], invalidate_dependents=True) as invalidation_check:
versioned_dist_target = assert_single_element(invalidation_check.all_vts)

versioned_target_fingerprint = versioned_dist_target.cache_key.hash

# This performs the normalization that distutils performs to the version string passed to the
# --tag-build option.
return re.sub(r'[^a-zA-Z0-9]', '.', versioned_target_fingerprint.lower())

def _create_distribution_synthetic_target(self, python_dist_target):
context = self._scheduling_context(
target_roots=[python_dist_target],
Expand All @@ -126,12 +149,16 @@ def _create_distribution_synthetic_target(self, python_dist_target):
self.assertEquals(1, len(synthetic_tgts))
synthetic_target = next(iter(synthetic_tgts))

return context, synthetic_target
snapshot_version = self._get_dist_snapshot_version(
python_create_distributions_task, python_dist_target)

return context, synthetic_target, snapshot_version

def test_python_create_universal_distribution(self):
universal_dist = self.target_dict['universal']
context, synthetic_target = self._create_distribution_synthetic_target(universal_dist)
self.assertEquals(['universal_dist==0.0.0'],
context, synthetic_target, snapshot_version = self._create_distribution_synthetic_target(
universal_dist)
self.assertEquals(['universal_dist==0.0.0+{}'.format(snapshot_version)],
[str(x.requirement) for x in synthetic_target.requirements.value])

local_wheel_products = context.products.get('local_wheels')
Expand All @@ -141,8 +168,9 @@ def test_python_create_universal_distribution(self):

def test_python_create_platform_specific_distribution(self):
platform_specific_dist = self.target_dict['platform_specific']
context, synthetic_target = self._create_distribution_synthetic_target(platform_specific_dist)
self.assertEquals(['platform_specific_dist==0.0.0'],
context, synthetic_target, snapshot_version = self._create_distribution_synthetic_target(
platform_specific_dist)
self.assertEquals(['platform_specific_dist==0.0.0+{}'.format(snapshot_version)],
[str(x.requirement) for x in synthetic_target.requirements.value])

local_wheel_products = context.products.get('local_wheels')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@

import glob
import os
import re
from zipfile import ZipFile

from pants.backend.native.config.environment import Platform
from pants.option.scope import GLOBAL_SCOPE_CONFIG_SECTION
from pants.util.collections import assert_single_element
from pants.util.contextutil import temporary_dir
from pants.util.dirutil import is_executable
from pants.util.process_handler import subprocess
Expand Down Expand Up @@ -46,23 +48,26 @@ def test_binary(self):
pex = os.path.join(tmp_dir, 'bin.pex')
self.assertTrue(is_executable(pex))

wheel_glob = os.path.join(tmp_dir, 'ctypes_test-0.0.1-*.whl')
globbed_wheel = glob.glob(wheel_glob)
self.assertEqual(len(globbed_wheel), 1)
wheel_dist = globbed_wheel[0]
# The + is because we append the target's fingerprint to the version. We test this version
# string in test_build_local_python_distributions.py.
wheel_glob = os.path.join(tmp_dir, 'ctypes_test-0.0.1+*.whl')
wheel_dist_with_path = assert_single_element(glob.glob(wheel_glob))
wheel_dist = re.sub('^{}{}'.format(re.escape(tmp_dir), os.path.sep), '', wheel_dist_with_path)

_, _, wheel_platform = name_and_platform(wheel_dist)
dist_name, dist_version, wheel_platform = name_and_platform(wheel_dist)
self.assertEqual(dist_name, 'ctypes_test')
contains_current_platform = Platform.create().resolve_platform_specific({
'darwin': lambda: wheel_platform.startswith('macosx'),
'linux': lambda: wheel_platform.startswith('linux'),
})
self.assertTrue(contains_current_platform)

# Verify that the wheel contains our shared libraries.
wheel_files = ZipFile(wheel_dist).namelist()
wheel_files = ZipFile(wheel_dist_with_path).namelist()

dist_versioned_name = '{}-{}.data'.format(dist_name, dist_version)
for shared_lib_filename in ['libasdf-c.so', 'libasdf-cpp.so']:
full_path_in_wheel = os.path.join('ctypes_test-0.0.1.data', 'data', shared_lib_filename)
full_path_in_wheel = os.path.join(dist_versioned_name, 'data', shared_lib_filename)
self.assertIn(full_path_in_wheel, wheel_files)

# Execute the binary and ensure its output is correct.
Expand Down
Loading