From cb1c39fe5836dc65db6b9bef14dc205ad18909d2 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Thu, 2 Jun 2016 13:17:51 -0700 Subject: [PATCH] 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 ` 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/ --- .../pants/backend/jvm/targets/jvm_app.py | 6 ++ src/python/pants/engine/legacy/BUILD | 2 + src/python/pants/engine/legacy/structs.py | 61 +++++++++++-------- src/python/pants/util/contextutil.py | 14 +++++ tests/python/pants_test/util/BUILD | 1 + .../pants_test/util/test_contextutil.py | 15 ++++- 6 files changed, 73 insertions(+), 26 deletions(-) diff --git a/src/python/pants/backend/jvm/targets/jvm_app.py b/src/python/pants/backend/jvm/targets/jvm_app.py index 05be455869f..c0055891427 100644 --- a/src/python/pants/backend/jvm/targets/jvm_app.py +++ b/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'") diff --git a/src/python/pants/engine/legacy/BUILD b/src/python/pants/engine/legacy/BUILD index 1779a23c487..32a0730cfe5 100644 --- a/src/python/pants/engine/legacy/BUILD +++ b/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', ], diff --git a/src/python/pants/engine/legacy/structs.py b/src/python/pants/engine/legacy/structs.py index 98a81572a33..9021e84479b 100644 --- a/src/python/pants/engine/legacy/structs.py +++ b/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): diff --git a/src/python/pants/util/contextutil.py b/src/python/pants/util/contextutil.py index 44a6bcb88d6..544f1e04ac8 100644 --- a/src/python/pants/util/contextutil.py +++ b/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 diff --git a/tests/python/pants_test/util/BUILD b/tests/python/pants_test/util/BUILD index a8cd3384e7c..6f1f916b8cc 100644 --- a/tests/python/pants_test/util/BUILD +++ b/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', ] ) diff --git a/tests/python/pants_test/util/test_contextutil.py b/tests/python/pants_test/util/test_contextutil.py index 35304a6379e..4d75ac0da28 100644 --- a/tests/python/pants_test/util/test_contextutil.py +++ b/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!')