Make JvmAppAdaptor compatible with bare bundle() form.

Given a BUILD file like:

```
jvm_app(
  name='bundle',
  basename='xyz',
  binary='x/y/z:bin',
  bundles=[
    bundle()
  ]
)
```

running `./pants --enable-v2-engine list <target>` currently throws an `AttributeError: JvmAppAdaptor(address=x/y:bundle) does not have attribute 'field_adaptors'`. this was an in-property exception (itself causing an `AttributeError` for failed resolution of the property) which actually turned out to be a second `AttributeError` for an invalid access of an undefined `bundle.fileset` attribute.

this review adds handling of this case with logging - as well as blanket exception logging for the `field_adaptors` properties to help reveal otherwise hidden exceptions in the future.

Testing Done:
reproduced locally in our monorepo against one of ~250 targets of this form. verified the exception logging prior to the change and the warning logging after the change.

https://travis-ci.org/pantsbuild/pants/builds/134654915

Bugs closed: 3540

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

src/python/pants/backend/jvm/targets/jvm_app.py

@@ -14,6 +14,7 @@
 
 from pants.backend.jvm.targets.jvm_binary import JvmBinary
 from pants.base.build_environment import get_buildroot
+from pants.base.deprecated import deprecated_conditional
 from pants.base.exceptions import TargetDefinitionException
 from pants.base.payload import Payload
 from pants.base.payload_field import PayloadField, PrimitiveField, combine_hashes
@@ -128,6 +129,11 @@ def __call__(self, rel_path=None, mapper=None, relative_to=None, fileset=None):
       filenames, or a Fileset object (e.g. globs()).
       E.g., ``relative_to='common'`` removes that prefix from all files in the application bundle.
     """
+    deprecated_conditional(lambda: fileset is None,
+                           '1.2.0',
+                           'bare bundle() without `fileset=` param',
+                           "Pass the `fileset=` parameter: `bundle(fileset=globs('*.config')`")
+
     if mapper and relative_to:
       raise ValueError("Must specify exactly one of 'mapper' or 'relative_to'")
 

src/python/pants/engine/legacy/BUILD

@@ -17,11 +17,13 @@ python_library(
   name='structs',
   sources=['structs.py'],
   dependencies=[
+    'src/python/pants/base:deprecated',
     'src/python/pants/build_graph',
     'src/python/pants/engine:fs',
     'src/python/pants/engine:nodes',
     'src/python/pants/engine:struct',
     'src/python/pants/source',
+    'src/python/pants/util:contextutil',
     'src/python/pants/util:meta',
     'src/python/pants/util:objects',
   ],

src/python/pants/engine/legacy/structs.py

@@ -10,12 +10,14 @@
 
 from six import string_types
 
+from pants.base.deprecated import deprecated_conditional
 from pants.build_graph.address import Addresses
 from pants.engine.addressable import Exactly, addressable_list
 from pants.engine.fs import Files as FSFiles
 from pants.engine.fs import PathGlobs
 from pants.engine.struct import Struct, StructWithDeps
 from pants.source import wrapped_globs
+from pants.util.contextutil import exception_logging
 from pants.util.meta import AbstractClass
 from pants.util.objects import datatype
 
@@ -43,11 +45,12 @@ def has_concrete_sources(self):
   @property
   def field_adaptors(self):
     """Returns a tuple of Fields for captured fields which need additional treatment."""
-    if not self.has_concrete_sources:
-      return tuple()
-    base_globs = BaseGlobs.from_sources_field(self.sources)
-    path_globs, excluded_path_globs = base_globs.to_path_globs(self.address.spec_path)
-    return (SourcesField(self.address, base_globs.filespecs, path_globs, excluded_path_globs),)
+    with exception_logging(logger, 'Exception in `field_adaptors` property'):
+      if not self.has_concrete_sources:
+        return tuple()
+      base_globs = BaseGlobs.from_sources_field(self.sources)
+      path_globs, excluded_path_globs = base_globs.to_path_globs(self.address.spec_path)
+      return (SourcesField(self.address, base_globs.filespecs, path_globs, excluded_path_globs),)
 
 
 class Field(object):
@@ -110,25 +113,35 @@ def bundles(self):
 
   @property
   def field_adaptors(self):
-    field_adaptors = super(JvmAppAdaptor, self).field_adaptors
-    if getattr(self, 'bundles', None) is None:
-      return field_adaptors
-    # Construct a field for the `bundles` argument.
-    filespecs_list = []
-    path_globs_list = []
-    excluded_path_globs_list = []
-    for bundle in self.bundles:
-      base_globs = BaseGlobs.from_sources_field(bundle.fileset)
-      filespecs_list.append(base_globs.filespecs)
-      path_globs, excluded_path_globs = base_globs.to_path_globs(self.address.spec_path)
-      path_globs_list.append(path_globs)
-      excluded_path_globs_list.append(excluded_path_globs)
-    bundles_field = BundlesField(self.address,
-                                 self.bundles,
-                                 filespecs_list,
-                                 path_globs_list,
-                                 excluded_path_globs_list)
-    return field_adaptors + (bundles_field,)
+    with exception_logging(logger, 'Exception in `field_adaptors` property'):
+      field_adaptors = super(JvmAppAdaptor, self).field_adaptors
+      if getattr(self, 'bundles', None) is None:
+        return field_adaptors
+      # Construct a field for the `bundles` argument.
+      filespecs_list = []
+      path_globs_list = []
+      excluded_path_globs_list = []
+      for bundle in self.bundles:
+        # Short circuit in the case of `bundles=[..., bundle(), ...]`.
+        if not hasattr(bundle, 'fileset'):
+          # N.B. This notice is duplicated in jvm_app.py::Bundle.__call__() for the old engine.
+          deprecated_conditional(lambda: True,
+                                 '1.2.0',
+                                 'bare bundle() without `fileset=` param',
+                                 "Pass a `fileset=` parameter: `bundle(fileset=globs('*.config')`")
+          logger.warn('Ignoring `bundle()` without `fileset` parameter.')
+          continue
+        base_globs = BaseGlobs.from_sources_field(bundle.fileset)
+        filespecs_list.append(base_globs.filespecs)
+        path_globs, excluded_path_globs = base_globs.to_path_globs(self.address.spec_path)
+        path_globs_list.append(path_globs)
+        excluded_path_globs_list.append(excluded_path_globs)
+      bundles_field = BundlesField(self.address,
+                                   self.bundles,
+                                   filespecs_list,
+                                   path_globs_list,
+                                   excluded_path_globs_list)
+      return field_adaptors + (bundles_field,)
 
 
 class BaseGlobs(AbstractClass):

src/python/pants/util/contextutil.py

@@ -221,3 +221,17 @@ def elapsed(self):
 
   def __exit__(self, typ, val, traceback):
     self.finish = self._clock.time()
+
+
+@contextmanager
+def exception_logging(logger, msg):
+  """Provides exception logging via `logger.exception` for a given block of code.
+
+  :param logging.Logger logger: The `Logger` instance to use for logging.
+  :param string msg: The message to emit before `logger.exception` emits the traceback.
+  """
+  try:
+    yield
+  except Exception:
+    logger.exception(msg)
+    raise

tests/python/pants_test/util/BUILD

@@ -15,6 +15,7 @@ python_tests(
   sources = ['test_contextutil.py'],
   coverage = ['pants.util.contextutil'],
   dependencies = [
+    '3rdparty/python:mock',
     'src/python/pants/util:contextutil',
   ]
 )

tests/python/pants_test/util/test_contextutil.py

@@ -11,8 +11,10 @@
 import sys
 import unittest
 
-from pants.util.contextutil import (Timer, environment_as, open_zip, pushd, stdio_as, temporary_dir,
-                                    temporary_file)
+import mock
+
+from pants.util.contextutil import (Timer, environment_as, exception_logging, open_zip, pushd,
+                                    stdio_as, temporary_dir, temporary_file)
 
 
 class ContextutilTest(unittest.TestCase):
@@ -182,3 +184,12 @@ def test_permissions(self):
 
     with temporary_dir(permissions=0644) as path:
       self.assertEquals(0644, os.stat(path)[0] & 0777)
+
+  def test_exception_logging(self):
+    fake_logger = mock.Mock()
+
+    with self.assertRaises(AssertionError):
+      with exception_logging(fake_logger, 'error!'):
+        assert True is False
+
+    fake_logger.exception.assert_called_once_with('error!')