From 4d92ff50d3f9be9e4177f423a3e5701a3137c09b Mon Sep 17 00:00:00 2001 From: Ity Kaul Date: Thu, 14 Jun 2018 13:16:05 -0700 Subject: [PATCH] `exclude-patterns` and `tag` should apply only to roots (#5786) ### Problem The `--exclude-patterns` flag currently applies to inner nodes, which causes odd errors. Moreover, tags should also apply only to roots. See #5189. ### Solution - added `tag` & `exclude_patterns` as params to `Specs` - add tests for both - modify changed tests to pass for inner node filtering ### Result Fixes #5189. --- src/python/pants/base/specs.py | 13 ++++-- src/python/pants/bin/goal_runner.py | 20 ++++----- src/python/pants/engine/BUILD | 1 + src/python/pants/engine/build_files.py | 36 ++++++++++------ src/python/pants/engine/legacy/graph.py | 8 ++-- .../pants/engine/legacy/source_mapper.py | 5 ++- src/python/pants/engine/scheduler.py | 2 +- src/python/pants/init/engine_initializer.py | 5 ++- .../pants/init/target_roots_calculator.py | 30 +++++++++---- .../pants/pantsd/service/scheduler_service.py | 9 ++-- .../src/python/python_targets/test_library.py | 2 +- tests/python/pants_test/engine/BUILD | 1 + .../engine/legacy/test_changed_integration.py | 27 +++++++++++- .../legacy/test_dependees_integration.py | 13 +++--- .../pants_test/engine/test_build_files.py | 42 ++++++++++++++++++- 15 files changed, 158 insertions(+), 56 deletions(-) diff --git a/src/python/pants/base/specs.py b/src/python/pants/base/specs.py index db75f6892bf..5ba7410440e 100644 --- a/src/python/pants/base/specs.py +++ b/src/python/pants/base/specs.py @@ -5,10 +5,11 @@ from __future__ import (absolute_import, division, generators, nested_scopes, print_function, unicode_literals, with_statement) +import re from abc import abstractmethod from pants.util.meta import AbstractClass -from pants.util.objects import Collection, datatype +from pants.util.objects import datatype class Spec(AbstractClass): @@ -60,5 +61,11 @@ def to_spec_string(self): return '{}^'.format(self.directory) -class Specs(Collection.of(Spec)): - """A collection of Spec subclasses.""" +class Specs(datatype(['dependencies', 'tags', ('exclude_patterns', tuple)])): + """A collection of Specs representing Spec subclasses, tags and regex filters.""" + + def __new__(cls, dependencies, tags=tuple(), exclude_patterns=tuple()): + return super(Specs, cls).__new__(cls, dependencies, tags, exclude_patterns) + + def exclude_patterns_memo(self): + return [re.compile(pattern) for pattern in set(self.exclude_patterns or [])] diff --git a/src/python/pants/bin/goal_runner.py b/src/python/pants/bin/goal_runner.py index e39d32a623a..dbd54087460 100644 --- a/src/python/pants/bin/goal_runner.py +++ b/src/python/pants/bin/goal_runner.py @@ -22,7 +22,6 @@ from pants.java.nailgun_executor import NailgunProcessGroup from pants.option.ranked_value import RankedValue from pants.task.task import QuietTaskMixin -from pants.util.filtering import create_filters, wrap_filters logger = logging.getLogger(__name__) @@ -70,7 +69,7 @@ def _handle_help(self, help_request): result = help_printer.print_help() self._exiter(result) - def _init_graph(self, target_roots, graph_helper): + def _init_graph(self, target_roots, graph_helper, exclude_target_regexps, tags): """Determine the BuildGraph, AddressMapper and spec_roots for a given run. :param TargetRoots target_roots: The existing `TargetRoots` object, if any. @@ -86,12 +85,13 @@ def _init_graph(self, target_roots, graph_helper): self._global_options, self._build_config) graph_helper = graph_scheduler_helper.new_session() - target_roots = target_roots or TargetRootsCalculator.create( options=self._options, - build_root=self._root_dir, session=graph_helper.scheduler_session, - symbol_table=graph_helper.symbol_table + build_root=self._root_dir, + symbol_table=graph_helper.symbol_table, + exclude_patterns=tuple(exclude_target_regexps), + tags=tuple(tags) ) graph, address_mapper = graph_helper.create_build_graph(target_roots, self._root_dir) @@ -112,16 +112,10 @@ def _determine_goals(self, requested_goals): def _roots_to_targets(self, target_roots): """Populate the BuildGraph and target list from a set of input TargetRoots.""" with self._run_tracker.new_workunit(name='parse', labels=[WorkUnitLabel.SETUP]): - def filter_for_tag(tag): - return lambda target: tag in map(str, target.tags) - - tag_filter = wrap_filters(create_filters(self._tag, filter_for_tag)) def generate_targets(): for address in self._build_graph.inject_roots_closure(target_roots, self._fail_fast): - target = self._build_graph.get_target(address) - if tag_filter(target): - yield target + yield self._build_graph.get_target(address) return list(generate_targets()) @@ -139,6 +133,8 @@ def _setup_context(self): self._build_graph, self._address_mapper, scheduler, target_roots = self._init_graph( self._target_roots, self._daemon_graph_helper, + self._global_options.exclude_target_regexp, + self._global_options.tag ) goals = self._determine_goals(self._requested_goals) diff --git a/src/python/pants/engine/BUILD b/src/python/pants/engine/BUILD index f9f6ca492dd..00dc0eb4bf9 100644 --- a/src/python/pants/engine/BUILD +++ b/src/python/pants/engine/BUILD @@ -71,6 +71,7 @@ python_library( 'src/python/pants/base:project_tree', 'src/python/pants/build_graph', 'src/python/pants/util:dirutil', + 'src/python/pants/util:filtering', 'src/python/pants/util:objects', ] ) diff --git a/src/python/pants/engine/build_files.py b/src/python/pants/engine/build_files.py index 2cfaebcb1f4..ce33581337e 100644 --- a/src/python/pants/engine/build_files.py +++ b/src/python/pants/engine/build_files.py @@ -7,6 +7,7 @@ import collections import functools +import logging from os.path import dirname, join import six @@ -24,9 +25,13 @@ from pants.engine.selectors import Get, Select from pants.engine.struct import Struct from pants.util.dirutil import fast_relpath_optional, recursive_dirname +from pants.util.filtering import create_filters, wrap_filters from pants.util.objects import TypeConstraintError, datatype +logger = logging.getLogger(__name__) + + class ResolvedTypeMismatchError(ResolveError): """Indicates a resolved object was not of the expected type.""" @@ -225,23 +230,28 @@ def by_directory(): def raise_empty_address_family(spec): raise ResolveError('Path "{}" does not contain any BUILD files.'.format(spec.directory)) - def exclude_address(address): - if address_mapper.exclude_patterns: - address_str = address.spec - return any(p.search(address_str) is not None for p in address_mapper.exclude_patterns) + def exclude_address(spec): + if specs.exclude_patterns: + return any(p.search(spec) is not None for p in specs.exclude_patterns_memo()) return False + def filter_for_tag(tag): + return lambda t: tag in map(str, t.kwargs().get("tags", [])) + + include_target = wrap_filters(create_filters(specs.tags if specs.tags else '', filter_for_tag)) + addresses = [] included = set() def include(address_families, predicate=None): matched = False for af in address_families: - for a in af.addressables.keys(): - if not exclude_address(a) and (predicate is None or predicate(a)): - matched = True - if a not in included: - addresses.append(a) - included.add(a) + for (a, t) in af.addressables.items(): + if (predicate is None or predicate(a)): + if include_target(t) and (not exclude_address(a.spec)): + matched = True + if a not in included: + addresses.append(a) + included.add(a) return matched for spec in specs.dependencies: @@ -263,8 +273,11 @@ def include(address_families, predicate=None): address_family = by_directory().get(spec.directory) if not address_family: raise_empty_address_family(spec) + # spec.name here is generally the root node specified on commandline. equality here implies + # a root node i.e. node specified on commandline. if not include([address_family], predicate=lambda a: a.target_name == spec.name): - _raise_did_you_mean(address_family, spec.name) + if len(addresses) == 0: + _raise_did_you_mean(address_family, spec.name) elif type(spec) is AscendantAddresses: include( af @@ -273,7 +286,6 @@ def include(address_families, predicate=None): ) else: raise ValueError('Unrecognized Spec type: {}'.format(spec)) - yield BuildFileAddresses(addresses) diff --git a/src/python/pants/engine/legacy/graph.py b/src/python/pants/engine/legacy/graph.py index 59333d794b4..beeaff2f2e2 100644 --- a/src/python/pants/engine/legacy/graph.py +++ b/src/python/pants/engine/legacy/graph.py @@ -203,7 +203,9 @@ def inject_addresses_closure(self, addresses): addresses = set(addresses) - set(self._target_by_address.keys()) if not addresses: return - for _ in self._inject_specs([SingleAddress(a.spec_path, a.target_name) for a in addresses]): + dependencies = tuple(SingleAddress(a.spec_path, a.target_name) for a in addresses) + specs = [Specs(dependencies=tuple(dependencies))] + for _ in self._inject_specs(specs): pass def inject_roots_closure(self, target_roots, fail_fast=None): @@ -211,6 +213,7 @@ def inject_roots_closure(self, target_roots, fail_fast=None): yield address def inject_specs_closure(self, specs, fail_fast=None): + specs = [Specs(dependencies=tuple(specs))] # Request loading of these specs. for address in self._inject_specs(specs): yield address @@ -258,9 +261,8 @@ def _inject_specs(self, subjects): logger.debug('Injecting specs to %s: %s', self, subjects) with self._resolve_context(): - specs = tuple(subjects) thts, = self._scheduler.product_request(TransitiveHydratedTargets, - [Specs(specs)]) + subjects) self._index(thts.closure) diff --git a/src/python/pants/engine/legacy/source_mapper.py b/src/python/pants/engine/legacy/source_mapper.py index cf11d590f24..31912061b52 100644 --- a/src/python/pants/engine/legacy/source_mapper.py +++ b/src/python/pants/engine/legacy/source_mapper.py @@ -79,11 +79,11 @@ def iter_target_addresses_for_sources(self, sources): """Bulk, iterable form of `target_addresses_for_source`.""" # Walk up the buildroot looking for targets that would conceivably claim changed sources. sources_set = set(sources) - specs = tuple(AscendantAddresses(directory=d) for d in self._unique_dirs_for_sources(sources_set)) + dependencies = tuple(AscendantAddresses(directory=d) for d in self._unique_dirs_for_sources(sources_set)) # Uniqify all transitive hydrated targets. hydrated_target_to_address = {} - hydrated_targets, = self._scheduler.product_request(HydratedTargets, [Specs(specs)]) + hydrated_targets, = self._scheduler.product_request(HydratedTargets, [Specs(dependencies=dependencies)]) for hydrated_target in hydrated_targets.dependencies: if hydrated_target not in hydrated_target_to_address: hydrated_target_to_address[hydrated_target] = hydrated_target.adaptor.address @@ -93,3 +93,4 @@ def iter_target_addresses_for_sources(self, sources): if (LegacyAddressMapper.any_is_declaring_file(legacy_address, sources_set) or self._owns_any_source(sources_set, hydrated_target)): yield legacy_address + diff --git a/src/python/pants/engine/scheduler.py b/src/python/pants/engine/scheduler.py index 2f2fb7c6ac2..474f1db6fff 100644 --- a/src/python/pants/engine/scheduler.py +++ b/src/python/pants/engine/scheduler.py @@ -423,7 +423,7 @@ def execution_request(self, products, subjects): :class:`pants.engine.fs.PathGlobs` objects. :returns: An ExecutionRequest for the given products and subjects. """ - roots = tuple((s, p) for s in subjects for p in products) + roots = (tuple((s, p) for s in subjects for p in products)) native_execution_request = self._scheduler._native.new_execution_request() for subject, product in roots: self._scheduler.add_root_selection(native_execution_request, subject, product) diff --git a/src/python/pants/init/engine_initializer.py b/src/python/pants/init/engine_initializer.py index 40d3d216eee..6ee63c8f3c8 100644 --- a/src/python/pants/init/engine_initializer.py +++ b/src/python/pants/init/engine_initializer.py @@ -9,7 +9,6 @@ from pants.base.build_environment import get_buildroot from pants.base.file_system_project_tree import FileSystemProjectTree -from pants.base.specs import Specs from pants.engine.build_files import create_graph_rules from pants.engine.fs import create_fs_rules from pants.engine.isolated_process import create_process_rules @@ -92,7 +91,9 @@ def warm_product_graph(self, target_roots): :param TargetRoots target_roots: The targets root of the request. """ logger.debug('warming target_roots for: %r', target_roots) - subjects = [Specs(tuple(target_roots.specs))] + subjects = target_roots.specs + if not subjects: + subjects = [] request = self.scheduler_session.execution_request([TransitiveHydratedTargets], subjects) result = self.scheduler_session.execute(request) if result.error: diff --git a/src/python/pants/init/target_roots_calculator.py b/src/python/pants/init/target_roots_calculator.py index 375fd8d00c5..989e2d66669 100644 --- a/src/python/pants/init/target_roots_calculator.py +++ b/src/python/pants/init/target_roots_calculator.py @@ -101,7 +101,7 @@ class TargetRootsCalculator(object): """Determines the target roots for a given pants run.""" @classmethod - def parse_specs(cls, target_specs, build_root=None): + def parse_specs(cls, target_specs, build_root=None, exclude_patterns=None, tags=None): """Parse string specs into unique `Spec` objects. :param iterable target_specs: An iterable of string specs. @@ -110,10 +110,18 @@ def parse_specs(cls, target_specs, build_root=None): """ build_root = build_root or get_buildroot() spec_parser = CmdLineSpecParser(build_root) - return OrderedSet(spec_parser.parse_spec(spec_str) for spec_str in target_specs) + + dependencies = tuple(OrderedSet(spec_parser.parse_spec(spec_str) for spec_str in target_specs)) + if not dependencies: + return None + return [Specs( + dependencies=dependencies, + exclude_patterns=exclude_patterns if exclude_patterns else tuple(), + tags=tags) + ] @classmethod - def create(cls, options, session, symbol_table, build_root=None): + def create(cls, options, session, symbol_table, build_root=None, exclude_patterns=None, tags=None): """ :param Options options: An `Options` instance to use. :param session: The Scheduler session @@ -121,7 +129,11 @@ def create(cls, options, session, symbol_table, build_root=None): :param string build_root: The build root. """ # Determine the literal target roots. - spec_roots = cls.parse_specs(options.target_specs, build_root) + spec_roots = cls.parse_specs( + target_specs=options.target_specs, + build_root=build_root, + exclude_patterns=exclude_patterns, + tags=tags) # Determine `Changed` arguments directly from options to support pre-`Subsystem` # initialization paths. @@ -135,8 +147,8 @@ def create(cls, options, session, symbol_table, build_root=None): logger.debug('changed_request is: %s', changed_request) logger.debug('owned_files are: %s', owned_files) scm = get_scm() - change_calculator = ChangeCalculator(session, symbol_table, scm) if scm else None - owner_calculator = OwnerCalculator(session, symbol_table) if owned_files else None + change_calculator = ChangeCalculator(scheduler=session, symbol_table=symbol_table, scm=scm) if scm else None + owner_calculator = OwnerCalculator(scheduler=session, symbol_table=symbol_table) if owned_files else None targets_specified = sum(1 for item in (changed_request.is_actionable(), owned_files, spec_roots) if item) @@ -153,14 +165,16 @@ def create(cls, options, session, symbol_table, build_root=None): # alternate target roots. changed_addresses = change_calculator.changed_target_addresses(changed_request) logger.debug('changed addresses: %s', changed_addresses) - return TargetRoots(tuple(SingleAddress(a.spec_path, a.target_name) for a in changed_addresses)) + dependencies = tuple(SingleAddress(a.spec_path, a.target_name) for a in changed_addresses) + return TargetRoots([Specs(dependencies=dependencies, exclude_patterns=exclude_patterns, tags=tags)]) if owner_calculator and owned_files: # We've been provided no spec roots (e.g. `./pants list`) AND a owner request. Compute # alternate target roots. owner_addresses = owner_calculator.owner_target_addresses(owned_files) logger.debug('owner addresses: %s', owner_addresses) - return TargetRoots(tuple(SingleAddress(a.spec_path, a.target_name) for a in owner_addresses)) + dependencies = tuple(SingleAddress(a.spec_path, a.target_name) for a in owner_addresses) + return TargetRoots([Specs(dependencies=dependencies, exclude_patterns=exclude_patterns, tags=tags)]) return TargetRoots(spec_roots) diff --git a/src/python/pants/pantsd/service/scheduler_service.py b/src/python/pants/pantsd/service/scheduler_service.py index 614408a7038..b3c14c4ba95 100644 --- a/src/python/pants/pantsd/service/scheduler_service.py +++ b/src/python/pants/pantsd/service/scheduler_service.py @@ -170,12 +170,13 @@ def warm_product_graph(self, options, target_roots_calculator): self._watchman_is_running.wait() session = self._graph_helper.new_session() - with self.fork_lock: target_roots = target_roots_calculator.create( - options, - session.scheduler_session, - session.symbol_table, + options=options, + session=session.scheduler_session, + symbol_table=session.symbol_table, + exclude_patterns=tuple(options.for_global_scope().exclude_target_regexp) if options.for_global_scope().exclude_target_regexp else tuple(), + tags=tuple(options.for_global_scope().tag) if options.for_global_scope().tag else tuple() ) session.warm_product_graph(target_roots) return session, target_roots diff --git a/testprojects/src/python/python_targets/test_library.py b/testprojects/src/python/python_targets/test_library.py index fac4315d7c6..c6380fe182c 100644 --- a/testprojects/src/python/python_targets/test_library.py +++ b/testprojects/src/python/python_targets/test_library.py @@ -1,4 +1,4 @@ -# coding=utf-8 +# coding=utf-8 # Copyright 2016 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). diff --git a/tests/python/pants_test/engine/BUILD b/tests/python/pants_test/engine/BUILD index ba98e52c20b..ba5ffaa6ffe 100644 --- a/tests/python/pants_test/engine/BUILD +++ b/tests/python/pants_test/engine/BUILD @@ -139,6 +139,7 @@ python_tests( ':scheduler_test_base', 'src/python/pants/build_graph', 'src/python/pants/engine:build_files', + 'src/python/pants/engine/legacy:structs', 'src/python/pants/engine:mapper', 'src/python/pants/engine:parser', 'src/python/pants/engine:scheduler', diff --git a/tests/python/pants_test/engine/legacy/test_changed_integration.py b/tests/python/pants_test/engine/legacy/test_changed_integration.py index b2b9cf75826..6d4ac722461 100644 --- a/tests/python/pants_test/engine/legacy/test_changed_integration.py +++ b/tests/python/pants_test/engine/legacy/test_changed_integration.py @@ -285,7 +285,32 @@ def run_list(self, extra_args, success=True): pants_run = self.do_command(*list_args, success=success) return pants_run.stdout_data - def test_test_changed_exclude_target(self): + def test_changed_exclude_root_targets_only(self): + changed_src = 'src/python/python_targets/test_library.py' + exclude_target_regexp = r'_[0-9]' + excluded_set = {'src/python/python_targets:test_library_transitive_dependee_2', + 'src/python/python_targets:test_library_transitive_dependee_3', + 'src/python/python_targets:test_library_transitive_dependee_4'} + expected_set = set(self.TEST_MAPPING[changed_src]['transitive']) - excluded_set + + with create_isolated_git_repo() as worktree: + with mutated_working_copy([os.path.join(worktree, changed_src)]): + pants_run = self.run_pants([ + '-ldebug', # This ensures the changed target names show up in the pants output. + '--exclude-target-regexp={}'.format(exclude_target_regexp), + '--changed-parent=HEAD', + '--changed-include-dependees=transitive', + 'test', + ]) + + self.assert_success(pants_run) + for expected_item in expected_set: + self.assertIn(expected_item, pants_run.stdout_data) + + for excluded_item in excluded_set: + self.assertNotIn(excluded_item, pants_run.stdout_data) + + def test_changed_not_exclude_inner_targets(self): changed_src = 'src/python/python_targets/test_library.py' exclude_target_regexp = r'_[0-9]' excluded_set = {'src/python/python_targets:test_library_transitive_dependee_2', diff --git a/tests/python/pants_test/engine/legacy/test_dependees_integration.py b/tests/python/pants_test/engine/legacy/test_dependees_integration.py index b3edb0146d7..2a2464b2e8f 100644 --- a/tests/python/pants_test/engine/legacy/test_dependees_integration.py +++ b/tests/python/pants_test/engine/legacy/test_dependees_integration.py @@ -24,12 +24,13 @@ def run_dependees(self, *dependees_options): def test_dependees_basic(self): pants_stdout = self.run_dependees() - self.assertEqual( - {'examples/src/scala/org/pantsbuild/example:jvm-run-example-lib', - 'examples/src/scala/org/pantsbuild/example/hello/exe:exe', - 'examples/tests/scala/org/pantsbuild/example/hello/welcome:welcome'}, - set(pants_stdout.split()) - ) + expected = { + 'examples/src/scala/org/pantsbuild/example:jvm-run-example-lib', + 'examples/src/scala/org/pantsbuild/example/hello/exe:exe', + 'examples/tests/scala/org/pantsbuild/example/hello/welcome:welcome' + } + actual = set(pants_stdout.split()) + self.assertEqual(expected, actual) def test_dependees_transitive(self): pants_stdout = self.run_dependees('--dependees-transitive') diff --git a/tests/python/pants_test/engine/test_build_files.py b/tests/python/pants_test/engine/test_build_files.py index 773135ee9aa..b8ca48fd37b 100644 --- a/tests/python/pants_test/engine/test_build_files.py +++ b/tests/python/pants_test/engine/test_build_files.py @@ -9,13 +9,14 @@ import unittest from pants.base.project_tree import Dir, File -from pants.base.specs import SingleAddress, Specs +from pants.base.specs import SiblingAddresses, SingleAddress, Specs from pants.build_graph.address import Address from pants.engine.addressable import addressable, addressable_dict from pants.engine.build_files import (ResolvedTypeMismatchError, addresses_from_address_families, create_graph_rules, parse_address_family) from pants.engine.fs import (DirectoryDigest, FileContent, FilesContent, Path, PathGlobs, Snapshot, create_fs_rules) +from pants.engine.legacy.structs import TargetAdaptor from pants.engine.mapper import AddressFamily, AddressMapper, ResolveError from pants.engine.nodes import Return, Throw from pants.engine.parser import SymbolTable @@ -54,6 +55,45 @@ def test_duplicated(self): self.assertEquals(len(bfas.dependencies), 1) self.assertEquals(bfas.dependencies[0].spec, 'a:a') + def test_tag_filter(self): + """Test that targets are filtered based on `tags`.""" + spec = SiblingAddresses('root') + address_mapper = AddressMapper(JsonParser(TestTable())) + snapshot = Snapshot(DirectoryDigest(str('xx'), 2), (Path('root/BUILD', File('root/BUILD')),)) + address_family = AddressFamily('root', + {'a': ('root/BUILD', TargetAdaptor()), + 'b': ('root/BUILD', TargetAdaptor(tags={'integration'})), + 'c': ('root/BUILD', TargetAdaptor(tags={'not_integration'})) + } + ) + + targets = run_rule( + addresses_from_address_families, address_mapper, Specs([spec], tags=['+integration']), { + (Snapshot, PathGlobs): lambda _: snapshot, + (AddressFamily, Dir): lambda _: address_family, + }) + + self.assertEquals(len(targets.dependencies), 1) + self.assertEquals(targets.dependencies[0].spec, 'root:b') + + def test_exclude_pattern(self): + """Test that targets are filtered based on exclude patterns.""" + spec = SiblingAddresses('root') + address_mapper = AddressMapper(JsonParser(TestTable())) + snapshot = Snapshot(DirectoryDigest(str('xx'), 2), (Path('root/BUILD', File('root/BUILD')),)) + address_family = AddressFamily('root', + {'exclude_me': ('root/BUILD', TargetAdaptor()), + 'not_me': ('root/BUILD', TargetAdaptor()), + } + ) + targets = run_rule( + addresses_from_address_families, address_mapper, Specs([spec], exclude_patterns=tuple(['.exclude*'])),{ + (Snapshot, PathGlobs): lambda _: snapshot, + (AddressFamily, Dir): lambda _: address_family, + }) + self.assertEquals(len(targets.dependencies), 1) + self.assertEquals(targets.dependencies[0].spec, 'root:not_me') + class ApacheThriftConfiguration(StructWithDeps): # An example of a mixed-mode object - can be directly embedded without a name or else referenced