Use target.id to create the stable classpath for bundle and export-cl…

…asspath

Today, for target a/b:b we create a symlink under a/b/b, current implementation
first delete everything under a/b/b, this is probalematic.

Consider target a/b:b and a/b/b/c:c whose stable classpath share the same
prefix: `a/b/b`

1. a/b/b/c:c first creates a stable symlink a/b/b/c/c/0-z.jar
2. a/b:b calls safe_rmtree('a/b/b') previous symlink will be deleted

The original intent to do `rmtree` my guess is to clear symlinks that already
exist during incremental build, but didn't consider targets may share share
common prefix.

This review
1. delete only non-directories under the current directory, not the entire tree
2. Provide an option to use `target.id` as the safe unique identifier.
   Switch to use `target.id` for `bundle`, but continue to use the old naming
   for `export-classpath` since it requires coordination of Intellij plugin.

Testing Done:
Locally tested on a previously failing bundle

https://travis-ci.org/peiyuwang/pants/builds/95140740 passed

Bugs closed: 2664

Reviewed at https://rbcommons.com/s/twitter/r/3211/

contrib/go/tests/python/pants_test/contrib/go/tasks/BUILD

@@ -112,6 +112,7 @@ python_tests(
     'contrib/go/src/python/pants/contrib/go/subsystems:go_distribution',
     'src/python/pants/util:contextutil',
     'tests/python/pants_test/subsystem:subsystem_utils',
+    'tests/python/pants_test/testutils:file_test_util',
     'tests/python/pants_test:int-test',
   ]
 )

contrib/go/tests/python/pants_test/contrib/go/tasks/test_go_compile_integration.py

@@ -9,6 +9,7 @@
 
 from pants_test.pants_run_integration_test import PantsRunIntegrationTest
 from pants_test.subsystem.subsystem_util import subsystem_instance
+from pants_test.testutils.file_test_util import contains_exact_files
 
 from pants.contrib.go.subsystems.go_distribution import GoDistribution
 
@@ -29,8 +30,8 @@ def test_go_compile_simple(self):
                              'pkg/{goos}_{goarch}/{libname}.a'
                              .format(libname=libname, goos=goos, goarch=goarch)
                              for libname in ('libA', 'libB', 'libC', 'libD', 'libE'))
-        self.assert_contains_exact_files(os.path.join(workdir, 'compile', 'go'),
-                                         expected_files, ignore_links=True)
+        self.assertTrue(contains_exact_files(os.path.join(workdir, 'compile', 'go'),
+                                             expected_files, ignore_links=True))
 
   def test_go_compile_cgo(self):
     args = ['compile', 'contrib/go/examples/src/go/cgo']

contrib/scrooge/tests/python/pants_test/contrib/scrooge/tasks/BUILD

@@ -57,7 +57,7 @@ python_tests(
      'src/python/pants/backend/codegen/targets:java',
      'src/python/pants/build_graph',
      'tests/python/pants_test/tasks:task_test_base',
-     'tests/python/pants_test/testutils',
+     'tests/python/pants_test/testutils:mock_logger',
   ],
 )
 

src/python/pants/backend/jvm/tasks/classpath_util.py

@@ -5,12 +5,13 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+import errno
 import os
 
 from twitter.common.collections import OrderedSet
 
 from pants.util.contextutil import open_zip
-from pants.util.dirutil import fast_relpath, safe_mkdir, safe_open, safe_rmtree, safe_walk
+from pants.util.dirutil import fast_relpath, safe_delete, safe_mkdir, safe_open, safe_walk
 
 
 class ClasspathUtil(object):
@@ -149,7 +150,8 @@ def is_dir(cls, path):
 
   @classmethod
   def create_canonical_classpath(cls, classpath_products, targets, basedir,
-                                 save_classpath_file=False):
+                                 save_classpath_file=False,
+                                 use_target_id=True):
     """Create a stable classpath of symlinks with standardized names.
 
     :param classpath_products: Classpath products.
@@ -162,17 +164,31 @@ def create_canonical_classpath(cls, classpath_products, targets, basedir,
     :rtype: list of strings
     """
     def _stable_output_folder(basedir, target):
+      if use_target_id:
+        return os.path.join(basedir, target.id)
+
       address = target.address
       return os.path.join(
         basedir,
         # target.address.spec is used in export goal to identify targets
         address.spec.replace(':', os.sep) if address.spec_path else address.target_name,
       )
 
+    def safe_delete_current_directory(directory):
+      """Delete only the files or symlinks under the current directory."""
+      try:
+        for name in os.listdir(directory):
+          path = os.path.join(directory, name)
+          if os.path.islink(path) or os.path.isfile(path):
+            safe_delete(path)
+      except OSError as e:
+        if e.errno != errno.ENOENT:
+          raise
+
     canonical_classpath = []
     for target in targets:
       folder_for_target_symlinks = _stable_output_folder(basedir, target)
-      safe_rmtree(folder_for_target_symlinks)
+      safe_delete_current_directory(folder_for_target_symlinks)
 
       classpath_entries_for_target = classpath_products.get_internal_classpath_entries_for_targets(
         [target])

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_classpath_publisher.py

@@ -14,6 +14,14 @@
 class RuntimeClasspathPublisher(Task):
   """Create stable symlinks for runtime classpath entries for JVM targets."""
 
+  @classmethod
+  def register_options(cls, register):
+    super(Task, cls).register_options(register)
+    register('--use-old-naming-style', advanced=True, default=True, action='store_true',
+             deprecated_version='0.0.65',
+             deprecated_hint='Switch to use the safe identifier to construct canonical classpath.',
+             help='Use the old (unsafe) naming style construct canonical classpath.')
+
   @classmethod
   def prepare(cls, options, round_manager):
     round_manager.require_data('runtime_classpath')
@@ -25,7 +33,9 @@ def _output_folder(self):
   def execute(self):
     basedir = os.path.join(self.get_options().pants_distdir, self._output_folder)
     runtime_classpath = self.context.products.get_data('runtime_classpath')
+    use_target_id = not self.get_options().use_old_naming_style
     ClasspathUtil.create_canonical_classpath(runtime_classpath,
                                              self.context.targets(),
                                              basedir,
-                                             save_classpath_file=True)
+                                             save_classpath_file=True,
+                                             use_target_id=use_target_id)

tests/python/pants_test/BUILD

@@ -41,14 +41,17 @@ python_library(
 
 python_library(
   name = 'int-test',
-  sources = ['pants_run_integration_test.py'],
+  sources = [
+    'pants_run_integration_test.py'
+  ],
   dependencies = [
     '3rdparty/python:ansicolors',
     'src/python/pants/base:build_environment',
     'src/python/pants/base:build_file',
     'src/python/pants/fs',
     'src/python/pants/util:contextutil',
     'src/python/pants/util:dirutil',
+    'tests/python/pants_test/testutils:file_test_util',
   ]
 )
 

tests/python/pants_test/backend/jvm/subsystems/test_shader_integration.py

@@ -80,7 +80,7 @@ def test_no_deployjar_run(self):
         library_jars_are_symlinks=False,
         expected_bundle_content=[
           'libs/com.google.code.gson-gson-2.3.1.jar',
-          'libs/testprojects/src/java/org/pantsbuild/testproject/shading/third_lib/0-z.jar',
+          'libs/testprojects.src.java.org.pantsbuild.testproject.shading.third_lib/0-z.jar',
           'third.jar']).strip()))
 
   def test_deployjar_run(self):

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

@@ -171,6 +171,7 @@ python_tests(
     'src/python/pants/backend/jvm/tasks:classpath_util',
     'src/python/pants/goal:products',
     'tests/python/pants_test:base_test',
+    'tests/python/pants_test/testutils:file_test_util',
   ]
 )
 
@@ -278,7 +279,7 @@ python_tests(
     'src/python/pants/util:contextutil',
     'src/python/pants/util:dirutil',
     'tests/python/pants_test/tasks:task_test_base',
-    'tests/python/pants_test/testutils',
+    'tests/python/pants_test/testutils:mock_logger',
   ],
 )
 

tests/python/pants_test/backend/jvm/tasks/test_classpath_util.py

@@ -8,9 +8,13 @@
 import os
 
 from pants.backend.jvm.targets.jvm_target import JvmTarget
+from pants.backend.jvm.tasks.classpath_products import ClasspathProducts
 from pants.backend.jvm.tasks.classpath_util import ClasspathUtil
 from pants.goal.products import UnionProducts
+from pants.util.contextutil import temporary_dir
+from pants.util.dirutil import relativize_paths
 from pants_test.base_test import BaseTest
+from pants_test.testutils.file_test_util import check_file_content, contains_exact_files
 
 
 class ClasspathUtilTest(BaseTest):
@@ -74,3 +78,91 @@ def test_relies_on_product_to_validate_paths_outside_buildroot(self):
                                                 confs=['default'])
 
     self.assertEqual(['/dev/null'], classpath)
+
+  def test_create_canonical_classpath(self):
+    a = self.make_target('a/b', JvmTarget)
+
+    classpath_products = ClasspathProducts(self.pants_workdir)
+
+    classpath_products.add_for_target(a, [('default', self._path('a.jar')),
+                                          ('default', self._path('resources'))])
+
+    self._test_canonical_classpath_helper(classpath_products, [a],
+                                          [
+                                            'a.b.b/0-a.jar',
+                                            'a.b.b/1-resources'
+                                          ],
+                                          {
+                                            'a.b.b/classpath.txt':
+                                              '{}/a.jar:{}/resources\n'.format(self.pants_workdir,
+                                                                               self.pants_workdir)
+                                           },
+                                          True)
+
+  def test_create_canonical_classpath_with_common_prefix(self):
+    """
+    A special case when two targets' canonical classpath share a common prefix.
+
+    Until we use `target.id` for canonical classpath, today's implementation is error-prone.
+    This is such a regression test case added for a bug discovered in
+    https://github.com/pantsbuild/pants/pull/2664
+
+    TODO(peiyu) Remove once we fully migrate to use `target.id`.
+    """
+    # a and c' canonical classpath share a common prefix: a/b/b
+    a = self.make_target('a/b', JvmTarget)
+    c = self.make_target('a/b/b/c', JvmTarget)
+
+    classpath_products = ClasspathProducts(self.pants_workdir)
+
+    classpath_products.add_for_target(a, [('default', self._path('a.jar'))])
+    classpath_products.add_for_target(c, [('default', self._path('c.jar'))])
+
+    # target c first to verify its first created canonical classpath is preserved
+    self._test_canonical_classpath_helper(classpath_products, [c, a],
+                                          [
+                                            'a/b/b/c/c/0-c.jar',
+                                            'a/b/b/0-a.jar',
+                                          ],
+                                          {
+                                            'a/b/b/classpath.txt':
+                                              '{}/a.jar\n'.format(self.pants_workdir),
+                                            'a/b/b/c/c/classpath.txt':
+                                              '{}/c.jar\n'.format(self.pants_workdir),
+                                          },
+                                          False)
+
+  def _test_canonical_classpath_helper(self, classpath_products, targets,
+                                       expected_canonical_classpath,
+                                       expected_classspath_files,
+                                       use_target_id):
+    """
+    Helper method to call `create_canonical_classpath` and verify generated canonical classpath.
+
+    :param ClasspathProducts classpath_products: Classpath products.
+    :param list targets: List of targets to generate canonical classpath from.
+    :param list expected_canonical_classpath: List of canonical classpath relative to a base directory.
+    :param dict expected_classspath_files: A dict of classpath.txt path to its expected content.
+    """
+    with temporary_dir() as base_dir:
+      canonical_classpath = ClasspathUtil.create_canonical_classpath(classpath_products,
+                                                                     targets,
+                                                                     base_dir,
+                                                                     save_classpath_file=True,
+                                                                     use_target_id=use_target_id)
+      # check canonical path returned
+      self.assertEquals(expected_canonical_classpath,
+                        relativize_paths(canonical_classpath, base_dir))
+
+      # check canonical path created contain the exact set of files, no more, no less
+      self.assertTrue(contains_exact_files(base_dir,
+                                           expected_canonical_classpath +
+                                           expected_classspath_files.keys()))
+
+      # check the content of classpath.txt
+      for classpath_file in expected_classspath_files:
+        self.assertTrue(check_file_content(os.path.join(base_dir, classpath_file),
+                                           expected_classspath_files[classpath_file]))
+
+  def _path(self, p):
+    return os.path.join(self.pants_workdir, p)

tests/python/pants_test/cache/BUILD

@@ -44,7 +44,7 @@ python_tests(
     'src/python/pants/util:contextutil',
     'src/python/pants/task',
     'tests/python/pants_test:base_test',
-    'tests/python/pants_test/testutils',
+    'tests/python/pants_test/testutils:mock_logger',
   ]
 )
 

tests/python/pants_test/invalidation/BUILD

@@ -7,7 +7,7 @@ python_tests(
   sources = ['test_cache_manager.py'],
   dependencies = [
     'src/python/pants/invalidation',
-    'tests/python/pants_test/testutils',
+    'tests/python/pants_test/testutils:mock_logger',
     'tests/python/pants_test/tasks:task_test_base',
   ]
 )

tests/python/pants_test/pants_run_integration_test.py

@@ -20,6 +20,7 @@
 from pants.fs.archive import ZIP
 from pants.util.contextutil import temporary_dir
 from pants.util.dirutil import safe_mkdir, safe_open
+from pants_test.testutils.file_test_util import check_symlinks, contains_exact_files
 
 
 PantsResult = namedtuple(
@@ -189,23 +190,23 @@ def bundle_and_run(self, target, bundle_name, bundle_options=None, args=None,
     pants_run = self.run_pants(bundle_options)
     self.assert_success(pants_run)
 
-    self.assert_symlinks('dist/{bundle_name}-bundle/libs'.format(bundle_name=bundle_name),
-                         library_jars_are_symlinks)
+    self.assertTrue(check_symlinks('dist/{bundle_name}-bundle/libs'.format(bundle_name=bundle_name),
+                                   library_jars_are_symlinks))
     # TODO(John Sirois): We need a zip here to suck in external library classpath elements
     # pointed to by symlinks in the run_pants ephemeral tmpdir.  Switch run_pants to be a
     # contextmanager that yields its results while the tmpdir workdir is still active and change
     # this test back to using an un-archived bundle.
     with temporary_dir() as workdir:
       ZIP.extract('dist/{bundle_name}.zip'.format(bundle_name=bundle_name), workdir)
       if expected_bundle_content:
-        self.assert_contains_exact_files(workdir, expected_bundle_content)
+        self.assertTrue(contains_exact_files(workdir, expected_bundle_content))
       if expected_bundle_jar_content:
         with temporary_dir() as check_bundle_jar_dir:
           bundle_jar = os.path.join(workdir, '{bundle_name}.jar')
           ZIP.extract('{workdir}/{bundle_name}.jar'.format(workdir=workdir,
                                                            bundle_name=bundle_name),
                       check_bundle_jar_dir)
-          self.assert_contains_exact_files(check_bundle_jar_dir, expected_bundle_jar_content)
+          self.assertTrue(contains_exact_files(check_bundle_jar_dir, expected_bundle_jar_content))
 
       optional_args = []
       if args:
@@ -248,36 +249,6 @@ def indent(content):
 
     assertion(value, pants_run.returncode, error_msg)
 
-  def assert_contains_exact_files(self, directory, expected_files, ignore_links=False):
-    """Asserts that the only files which directory contains are expected_files.
-
-    :param str directory: Path to directory to search.
-    :param set expected_files: Set of filepaths relative to directory to search for.
-    :param bool ignore_links: Indicates to ignore any file links.
-    """
-    found = []
-    for root, _, files in os.walk(directory):
-      for f in files:
-        p = os.path.join(root, f)
-        if ignore_links and os.path.islink(p):
-          continue
-        found.append(os.path.relpath(p, directory))
-
-    self.assertEqual(sorted(expected_files), sorted(found))
-
-  def assert_symlinks(self, directory, symlinks=True):
-    """Asserts files under directory are symlinks.
-
-    :param str directory: Path to directory to search.
-    :param bool symlinks: If true, verify files are symlinks, if false, verify files are actual files.
-    """
-    for root, _, files in os.walk(directory):
-      for f in files:
-        p = os.path.join(root, f)
-        if symlinks ^ os.path.islink(p):
-          return False
-    return True
-
   def normalize(self, s):
     """Removes escape sequences (e.g. colored output) and all whitespace from string s."""
     return ''.join(strip_color(s).split())

tests/python/pants_test/projects/BUILD

@@ -14,7 +14,7 @@ python_tests(
   sources=['test_examples_integration.py'],
   dependencies=[
     ':base',
-    'tests/python/pants_test/testutils',
+    'tests/python/pants_test/testutils:mock_logger',
   ]
 )
 
@@ -23,7 +23,7 @@ python_tests(
   sources=['test_testprojects_integration.py'],
   dependencies=[
     ':base',
-    'tests/python/pants_test/testutils',
+    'tests/python/pants_test/testutils:mock_logger',
   ]
 )
 
@@ -33,4 +33,4 @@ python_tests(
   dependencies=[
     'tests/python/pants_test:int-test',
   ]
-)
+)

tests/python/pants_test/targets/test_jvm_app_integration.py

@@ -29,7 +29,7 @@ def test_bundle(self):
         expected_bundle_content=[
           'bundle-example.jar',
           'libs/com.google.guava-guava-18.0.jar',
-          'libs/testprojects/src/java/org/pantsbuild/testproject/bundle/bundle-bin/0-z.jar']))
+          'libs/testprojects.src.java.org.pantsbuild.testproject.bundle.bundle-bin/0-z.jar']))
 
   def test_bundle_deployjar(self):
     """bundle with --deployjar.