Skip to content

Commit

Permalink
[engine] Capture the resources=globs argument for Python targets
Browse files Browse the repository at this point in the history
This fixes [pantsbuild#3506](pantsbuild#3506), which is caused by our conversion of the `globs/rglobs` objects without capturing them later.

- Capture the `resources=globs` argument for Python targets
- Open tickets to design the migration path to typed Sources

Testing Done:
http://jenkins.pantsbuild.org/job/pantsbuild/job/pants/branch/PR-3566/2/

Bugs closed: 3506, 3566

Reviewed at https://rbcommons.com/s/twitter/r/3979/
  • Loading branch information
stuhood committed Jun 7, 2016
1 parent 5d65603 commit 04e4457
Show file tree
Hide file tree
Showing 8 changed files with 66 additions and 10 deletions.
8 changes: 7 additions & 1 deletion src/python/pants/bin/engine_initializer.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from pants.engine.graph import create_graph_tasks
from pants.engine.legacy.graph import LegacyBuildGraph, create_legacy_graph_tasks
from pants.engine.legacy.parser import LegacyPythonCallbacksParser
from pants.engine.legacy.structs import JvmAppAdaptor, TargetAdaptor
from pants.engine.legacy.structs import JvmAppAdaptor, PythonTargetAdaptor, TargetAdaptor
from pants.engine.mapper import AddressMapper
from pants.engine.parser import SymbolTable
from pants.engine.scheduler import LocalScheduler
Expand All @@ -45,8 +45,14 @@ def aliases(cls):
@memoized_method
def table(cls):
def target_type(alias):
# TODO: The alias matching here is to avoid elevating "TargetAdaptors" into the public
# API until until after https://github.com/pantsbuild/pants/issues/3560 has been completed.
# These should likely move onto Target subclasses as the engine gets deeper into beta
# territory.
if alias == 'jvm_app':
return JvmAppAdaptor
elif alias in ('python_library', 'python_tests', 'python_binary'):
return PythonTargetAdaptor
else:
return TargetAdaptor
return {alias: target_type(alias) for alias in cls.aliases().target_types}
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/engine/legacy/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ def hydrate_sources(sources_field, source_files_digest, excluded_source_files):
sources_field.filespecs,
source_files_digest,
excluded_source_files)
return HydratedField('sources', fileset_with_spec)
return HydratedField(sources_field.arg, fileset_with_spec)


def hydrate_bundles(bundles_field, files_digest_list, excluded_files_list):
Expand Down
12 changes: 7 additions & 5 deletions src/python/pants/engine/legacy/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class LegacyPythonCallbacksParser(Parser):
@memoized_method
def _get_symbols(cls, symbol_table_cls):
symbol_table = symbol_table_cls.table()
# TODO: nasty escape hatch
# TODO: Nasty escape hatch: see https://github.com/pantsbuild/pants/issues/3561
aliases = symbol_table_cls.aliases()

class Registrar(BuildFileTargetFactory):
Expand All @@ -58,9 +58,7 @@ def __call__(self, *args, **kwargs):
else:
return self._object_type(*args, **kwargs)

# Compute a single ParseContext for a default path, which we will mutate for each parsed path.
symbols = {}
parse_context = ParseContext(rel_path='', type_aliases=symbols)

for alias, symbol in symbol_table.items():
registrar = Registrar(alias, symbol)
Expand All @@ -71,16 +69,20 @@ def __call__(self, *args, **kwargs):
symbols.update(aliases.objects)

# Compute "per path" symbols (which will all use the same mutable ParseContext).
aliases = symbol_table_cls.aliases()
# TODO: See https://github.com/pantsbuild/pants/issues/3561
parse_context = ParseContext(rel_path='', type_aliases=symbols)
for alias, object_factory in aliases.context_aware_object_factories.items():
symbols[alias] = object_factory(parse_context)

for alias, target_macro_factory in aliases.target_macro_factories.items():
underlying_symbol = symbols.get(alias, TargetAdaptor)
symbols[alias] = target_macro_factory.target_macro(parse_context)
for target_type in target_macro_factory.target_types:
symbols[target_type] = Registrar(alias, TargetAdaptor)
symbols[target_type] = Registrar(alias, underlying_symbol)

# TODO: Replace builtins for paths with objects that will create wrapped PathGlobs objects.
# The strategy for https://github.com/pantsbuild/pants/issues/3560 should account for
# migrating these additional captured arguments to typed Sources.
symbols['globs'] = Globs
symbols['rglobs'] = RGlobs
symbols['zglobs'] = ZGlobs
Expand Down
38 changes: 35 additions & 3 deletions src/python/pants/engine/legacy/structs.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,30 +50,45 @@ def field_adaptors(self):
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),)
return (SourcesField(self.address, 'sources', base_globs.filespecs, path_globs, excluded_path_globs),)


class Field(object):
"""A marker for Target(Adaptor) fields for which the engine might perform extra construction."""


class SourcesField(datatype('SourcesField', ['address', 'filespecs', 'path_globs', 'excluded_path_globs']), Field):
class SourcesField(datatype('SourcesField', ['address', 'arg', 'filespecs', 'path_globs', 'excluded_path_globs']), Field):
"""Represents the `sources` argument for a particular Target.
Sources are currently eagerly computed in-engine in order to provide the `BuildGraph`
API efficiently; once tasks are explicitly requesting particular Products for Targets,
lazy construction will be more natural.
see https://github.com/pantsbuild/pants/issues/3560
:param address: The Address of the TargetAdaptor for which this field is an argument.
:param arg: The name of this argument: usually 'sources', but occasionally also 'resources' in the
case of python resource globs.
:param filespecs: The merged filespecs dict the describes the paths captured by this field.
:param path_globs: A PathGlobs describing included files.
:param excluded_path_globs: A PathGlobs describing files excluded (ie, subtracted) from the
include set.
"""

def __eq__(self, other):
return type(self) == type(other) and self.address == other.address
return type(self) == type(other) and self.address == other.address and self.arg == other.arg

def __ne__(self, other):
return not (self == other)

def __hash__(self):
return hash(self.address)

def __repr__(self):
return str(self)

def __str__(self):
return 'SourcesField(address={}, arg={}, filespecs={!r})'.format(self.address, self.arg, self.filespecs)


class BundlesField(datatype('BundlesField', ['address', 'bundles', 'filespecs_list', 'path_globs_list', 'excluded_path_globs_list']), Field):
"""Represents the `bundles` argument, each of which has a PathGlobs to represent its `fileset`."""
Expand Down Expand Up @@ -144,6 +159,23 @@ def field_adaptors(self):
return field_adaptors + (bundles_field,)


class PythonTargetAdaptor(TargetAdaptor):
@property
def field_adaptors(self):
with exception_logging(logger, 'Exception in `field_adaptors` property'):
field_adaptors = super(PythonTargetAdaptor, self).field_adaptors
if getattr(self, 'resources', None) is None:
return field_adaptors
base_globs = BaseGlobs.from_sources_field(self.resources)
path_globs, excluded_path_globs = base_globs.to_path_globs(self.address.spec_path)
sources_field = SourcesField(self.address,
'resources',
base_globs.filespecs,
path_globs,
excluded_path_globs)
return field_adaptors + (sources_field,)


class BaseGlobs(AbstractClass):
"""An adaptor class to allow BUILD file parsing from ContextAwareObjectFactories."""

Expand Down
4 changes: 4 additions & 0 deletions testprojects/src/python/sources/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
python_library(name='sources',
sources=globs('*.py'),
resources=globs('*.txt'),
)
1 change: 1 addition & 0 deletions testprojects/src/python/sources/sources.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# An empty python file.
1 change: 1 addition & 0 deletions testprojects/src/python/sources/sources.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# An empty test file.
10 changes: 10 additions & 0 deletions tests/python/pants_test/engine/legacy/test_filemap_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ def _extract_exclude_output(self, test_name):
def test_testprojects(self):
self.do_command('filemap', 'testprojects::', success=True, enable_v2_engine=True)

def test_python_sources(self):
run = self.do_command('filemap',
'testprojects/src/python/sources',
success=True,
enable_v2_engine=True)
self.assertIn('testprojects/src/python/sources/sources.py', run.stdout_data)
# FIXME: The synthetic target that is created to expose the `resources=globs` arg does not show
# up in filemap, because it is synthetic. see https://github.com/pantsbuild/pants/issues/3563
#self.assertIn('testprojects/src/python/sources/sources.txt', run.stdout_data)

def test_exclude_invalid_string(self):
build_path = os.path.join(self.PATH_PREFIX, 'BUILD.invalid')
build_content = '''python_library(name='exclude_strings_disallowed',
Expand Down

0 comments on commit 04e4457

Please sign in to comment.