add the target fingerprint to the version of each local dist so that …

…we don't use the first cached one (#6022)

### Problem

I made #5479 because the dist resolved for a `python_dist()` would not change after changing any of the source files, and this persisted even after a `clean-all`. This issue is more thoroughly explained in #5449 (which I literally didn't see before making this PR, but describes almost exactly what this PR does, it seems -- great minds think alike?). We were building the `python_dist()` targets each time they were invalidated, but caching the package locally in `~/.cache/pants/` after we built it the first time and using that instead, because the version string was the same for both the first and subsequent builds of the `python_dist()` target.

### Solution

- Move argv generation for setup.py invocations into `BuildLocalPythonDistributions`.
- Append the `python_dist()` target's fingerprint to the version string using `--tag-build`. This conforms to the "local" version specifier format as per [PEP 440](https://www.python.org/dev/peps/pep-0440/#local-version-identifiers). This tagged version is then used in the synthetic `PythonRequirement` stitched into the build graph for the local dist.
- Add an integration test `test_invalidation()` using `mock_buildroot()` (which fails on master, but not in this PR) where we run a binary depending on a `python_dist()`, rewrite the contents of a source file to change the output, then run pants again to verify that the new dist is resolved.

*Separately:*

- Made `PythonDistribution` subclass `PythonTarget` to avoid duplicated logic. It's not clear what the original reason for not subclassing it was at first, but it seems to be not be in effect anymore.
- Changed the `dir` field name for the `Manager` namedtuple in `mock_buildroot()` to be `new_buildroot` so it doesn't get highlighted as a builtin in certain text editors.

### Result

When we resolve local dists in any python requirements task, we get the dist corresponding to the target we just built. Resolves #5449.

examples/src/python/example/python_distribution/hello/fasthello/hello_package/hello.py

@@ -7,6 +7,7 @@
 import c_greet
 import cpp_greet
 
+
 def hello():
   return '\n'.join([
     c_greet.c_greet(),

examples/src/python/example/python_distribution/hello/setup_requires/hello_package/hello.py

@@ -4,5 +4,6 @@
 
 from __future__ import absolute_import, division, print_function, unicode_literals
 
+
 def hello():
   return 'Hello, world!'

src/python/pants/backend/python/targets/python_distribution.py

@@ -4,18 +4,15 @@
 
 from __future__ import absolute_import, division, print_function, unicode_literals
 
-from builtins import str
-
-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'
@@ -28,7 +25,6 @@ def __init__(self,
                address=None,
                payload=None,
                sources=None,
-               compatibility=None,
                setup_requires=None,
                **kwargs):
     """
@@ -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
+                   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:
+      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):

src/python/pants/backend/python/tasks/build_local_python_distributions.py

@@ -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
@@ -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
@@ -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)
@@ -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. '
@@ -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()):

src/python/pants/backend/python/tasks/setup_py.py

@@ -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
@@ -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)

tests/python/pants_test/backend/python/tasks/BUILD

@@ -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(
@@ -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,
 )
 
@@ -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'],

tests/python/pants_test/backend/python/tasks/test_build_local_python_distributions.py

@@ -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
 
@@ -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)
@@ -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],
@@ -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')
@@ -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')

tests/python/pants_test/backend/python/tasks/test_ctypes_integration.py

@@ -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
@@ -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.