Add check if jars exists before registering products

Commits:
Add check if jar exists before registering products and also move the check out of with context

Pants, registers a empty jar product when there were no products generated by the target.

I saw this happen for sure when a target contained an empty scala file.
compile task did not produce any classes for that target and hence a jar with zero length got created.
(Obviously the fix was the remove the empty scala file, but pants needs to be more robust to handle such cases)

The reason for this was in commit 570498e Benjy added a change to add empty products for a target. Hence, the condition https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jar_task.py#L342 always returned true.
I added the method __nonzero__ for python 2.x and __bool__ for python 3.x in  MultipleRootedProducts and  RootedProducts to return true only if there were any products registered.
Added test case for this scenario.

We also saw this happen in idea --intransitive phase. The jar phase registered products for synthetic java_thrift_library target created from twitter internal task. We create 2 synthetic targets (scala and java) for one thrift_library and attach same sources.
At runtime, however only one jar product was created but two got registered.
We saw this error
{code}
IOError: [Errno 2] No such file or directory: u'/Users/bzhang/workspace/source/birdcage/.pants.d/jar/jar/.pants.d.gen.scrooge.java-finagle..pants.d.gen.idl-extract.3rdparty.jvm.com.twitter.ibis.deprecated-ibis-thrift-service-java.3rdparty.jvm.com.twitter.ibis.deprecated-ibis-thrift-service-java.jar'

IOError: [Errno 2] No such file or directory: u'/Users/tdesai/projects/source2/birdcage/.pants.d/jar/jar/ibis.ibis-executor.src.test.java.java.jar'
{code}

I am happy to hear any other potential fixes.

Testing Done:
yes:
https://travis-ci.org/pantsbuild/pants/builds/51559910

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

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

@@ -71,7 +71,7 @@ def execute(self):
         jar_name = jarname(target)
         jar_path = os.path.join(self.workdir, jar_name)
         with self.create_jar(target, jar_path) as jarfile:
-          if self._jar_builder.add_target(jarfile, target):
+          if target in self._jar_builder.add_target(jarfile, target):
             self.context.products.get('jars').add(target, self.workdir).append(jar_name)
 
   @contextmanager

src/python/pants/goal/products.py

@@ -71,6 +71,11 @@ def abs_paths(self):
     for relpath in self._rel_paths:
       yield os.path.join(self._root, relpath)
 
+  def __bool__(self):
+    return self._rel_paths
+
+  __nonzero__ = __bool__
+
 
 class MultipleRootedProducts(object):
   """A product consisting of multiple roots, with associated file products."""
@@ -94,6 +99,15 @@ def abs_paths(self):
   def _get_products_for_root(self, root):
     return self._rooted_products_by_root.setdefault(root, RootedProducts(root))
 
+  def __bool__(self):
+    """Return True if any of the roots contains products"""
+    for root, products in self.rel_paths():
+      if products:
+        return True
+    return False
+
+  __nonzero__ = __bool__
+
 
 class Products(object):
   """An out-of-band 'dropbox' where tasks can place build product information for later tasks to use.
@@ -188,6 +202,11 @@ def __repr__(self):
                               for target, outputs_by_basedir in self.by_target.items()
                               for basedir, outputs in outputs_by_basedir.items()))
 
+    def __bool__(self):
+      return not self.empty()
+
+    __nonzero__ = __bool__
+
   def __init__(self):
     self.products = {}  # type -> ProductMapping instance.
     self.predicates_for_type = defaultdict(list)

tests/python/pants_test/BUILD

@@ -19,6 +19,7 @@ python_library(
     'src/python/pants/base:exceptions',
     'src/python/pants/base:target',
     'src/python/pants/goal:goal',
+    'src/python/pants/goal:products',
     'src/python/pants/util:contextutil',
     'src/python/pants/util:dirutil',
     'tests/python/pants_test/base:context_utils',

tests/python/pants_test/base_test.py

@@ -27,7 +27,7 @@
 from pants.base.source_root import SourceRoot
 from pants.base.target import Target
 from pants.goal.goal import Goal
-from pants.goal.products import UnionProducts
+from pants.goal.products import MultipleRootedProducts, UnionProducts
 from pants.util.contextutil import pushd, temporary_dir, temporary_file
 from pants.util.dirutil import safe_mkdir, safe_open, safe_rmtree, touch
 from pants_test.base.context_utils import create_context
@@ -276,3 +276,27 @@ def populate_compile_classpath(self, context, classpath=None):
     """
     compile_classpaths = context.products.get_data('compile_classpath', lambda: UnionProducts())
     compile_classpaths.add_for_targets(context.targets(), [('default', entry) for entry in classpath or ['none']])
+
+  @contextmanager
+  def add_data(self, context_products, data_type, target, *products):
+    make_products = lambda: defaultdict(MultipleRootedProducts)
+    data_by_target = context_products.get_data(data_type, make_products)
+    with temporary_dir() as outdir:
+      def create_product(product):
+        abspath = os.path.join(outdir, product)
+        with safe_open(abspath, mode='w') as fp:
+          fp.write(product)
+        return abspath
+      data_by_target[target].add_abs_paths(outdir, map(create_product, products))
+      yield temporary_dir
+
+  @contextmanager
+  def add_products(self, context_products, product_type, target, *products):
+    product_mapping = context_products.get(product_type)
+    with temporary_dir() as outdir:
+      def create_product(product):
+        with safe_open(os.path.join(outdir, product), mode='w') as fp:
+          fp.write(product)
+        return product
+      product_mapping.add(target, outdir, map(create_product, products))
+      yield temporary_dir

tests/python/pants_test/goal/test_products.py

@@ -5,13 +5,13 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
-import unittest
-
 from pants.goal.products import Products
+from pants_test.base_test import BaseTest
 
 
-class ProductsTest(unittest.TestCase):
+class ProductsTest(BaseTest):
   def setUp(self):
+    super(ProductsTest, self).setUp()
     self.products = Products()
 
   def test_require(self):
@@ -87,3 +87,23 @@ def test_get_data_does_not_require_data(self):
     self.assertFalse(self.products.is_required_data('foo'))
     self.products.require_data('foo')
     self.assertTrue(self.products.is_required_data('foo'))
+
+  def test_empty_products(self):
+    foo_product_mapping = self.products.get('foo')
+    self.assertFalse(foo_product_mapping)
+
+  def test_non_empty_products(self):
+    target = self.make_target('c')
+    with self.add_products(self.products, 'foo', target, 'a.class'):
+      foo_product_mapping = self.products.get('foo')
+      self.assertTrue(foo_product_mapping)
+
+  def test_empty_data(self):
+    foo_product_mapping = self.products.get_data('foo')
+    self.assertFalse(foo_product_mapping)
+
+  def test_non_empty_data(self):
+    target = self.make_target('c')
+    with self.add_data(self.products, 'foo', target, 'a.class'):
+      foo_product_mapping = self.products.get_data('foo')
+      self.assertTrue(foo_product_mapping)

tests/python/pants_test/goal/test_union_products.py

@@ -27,3 +27,12 @@ def test_get_for_target(self):
     self.assertEquals(self.products.get_for_target(a), OrderedSet([1, 2, 3]))
     self.assertEquals(self.products.get_for_target(b), OrderedSet([2, 3]))
     self.assertEquals(self.products.get_for_target(c), OrderedSet([3]))
+
+  def test_empty_products(self):
+    c = self.make_target('c')
+    self.assertFalse(self.products.get_for_target(c))
+
+  def test_non_empty_products(self):
+    c = self.make_target('c')
+    self.products.add_for_target(c, [3])
+    self.assertTrue(self.products.get_for_target(c))

tests/python/pants_test/tasks/BUILD

@@ -416,7 +416,6 @@ python_tests(
     'src/python/pants/base:build_environment',
     'src/python/pants/base:build_file_aliases',
     'src/python/pants/base:source_root',
-    'src/python/pants/goal:products',
     'src/python/pants/java:jar',
     'src/python/pants/util:contextutil',
     'src/python/pants/util:dirutil',

tests/python/pants_test/tasks/test_jar_create.py

@@ -6,8 +6,7 @@
                         unicode_literals, with_statement)
 
 import os
-from collections import defaultdict
-from contextlib import closing, contextmanager
+from contextlib import closing
 from textwrap import dedent
 
 from pants.backend.codegen.targets.java_thrift_library import JavaThriftLibrary
@@ -17,9 +16,7 @@
 from pants.backend.jvm.targets.scala_library import ScalaLibrary
 from pants.backend.jvm.tasks.jar_create import JarCreate, is_jvm_library
 from pants.base.source_root import SourceRoot
-from pants.goal.products import MultipleRootedProducts
 from pants.util.contextutil import open_zip, temporary_dir
-from pants.util.dirutil import safe_open
 from pants_test.jvm.jar_task_test_base import JarTaskTestBase
 
 
@@ -93,35 +90,13 @@ def get_source_root_fs_path(path):
                                         java_sources=['src/java/com/twitter/foo:java_foo'])
     self.binary = self.jvm_binary('src/java/com/twitter/baz', 'baz', source='b.java',
                                   resources='src/resources/com/twitter:spam')
+    self.empty_sl = self.scala_library('src/scala/com/foo', 'foo', ['dupe.scala'])
 
   def context(self, **kwargs):
     return super(JarCreateExecuteTest, self).context(
-      target_roots=[self.jl, self.sl, self.binary, self.jtl, self.scala_lib],
+      target_roots=[self.jl, self.sl, self.binary, self.jtl, self.scala_lib, self.empty_sl],
       **kwargs)
 
-  @contextmanager
-  def add_products(self, context, product_type, target, *products):
-    product_mapping = context.products.get(product_type)
-    with temporary_dir() as outdir:
-      def create_product(product):
-        with safe_open(os.path.join(outdir, product), mode='w') as fp:
-          fp.write(product)
-        return product
-      product_mapping.add(target, outdir, map(create_product, products))
-      yield temporary_dir
-
-  @contextmanager
-  def add_data(self, context, data_type, target, *products):
-    make_products = lambda: defaultdict(MultipleRootedProducts)
-    data_by_target = context.products.get_data(data_type, make_products)
-    with temporary_dir() as outdir:
-      def create_product(product):
-        abspath = os.path.join(outdir, product)
-        with safe_open(abspath, mode='w') as fp:
-          fp.write(product)
-        return abspath
-      data_by_target[target].add_abs_paths(outdir, map(create_product, products))
-      yield temporary_dir
 
   def assert_jar_contents(self, context, product_type, target, *contents):
     jar_mapping = context.products.get(product_type).get(target)
@@ -137,11 +112,11 @@ def assert_jar_contents(self, context, product_type, target, *contents):
 
   def test_classfile_jar_contents(self):
     context = self.context()
-    with self.add_data(context, 'classes_by_target', self.jl, 'a.class', 'b.class'):
-      with self.add_data(context, 'classes_by_target', self.sl, 'c.class'):
-        with self.add_data(context, 'classes_by_target', self.binary, 'b.class'):
-          with self.add_data(context, 'resources_by_target', self.res, 'r.txt.transformed'):
-            with self.add_data(context, 'classes_by_target', self.scala_lib, 'scala_foo.class',
+    with self.add_data(context.products, 'classes_by_target', self.jl, 'a.class', 'b.class'):
+      with self.add_data(context.products, 'classes_by_target', self.sl, 'c.class'):
+        with self.add_data(context.products, 'classes_by_target', self.binary, 'b.class'):
+          with self.add_data(context.products, 'resources_by_target', self.res, 'r.txt.transformed'):
+            with self.add_data(context.products, 'classes_by_target', self.scala_lib, 'scala_foo.class',
                                'java_foo.class'):
               self.execute(context)
 
@@ -152,3 +127,10 @@ def test_classfile_jar_contents(self):
                                         'b.class', 'r.txt.transformed')
               self.assert_jar_contents(context, 'jars', self.scala_lib, 'scala_foo.class',
                                         'java_foo.class')
+
+  def test_empty_scala_files(self):
+    context = self.context()
+    with self.add_data(context.products, 'classes_by_target', self.empty_sl):
+      with self.add_data(context.products, 'resources_by_target', self.res, 'r.txt.transformed'):
+        self.execute(context)
+        self.assertFalse(context.products.get('jars').has(self.empty_sl))