From 84084e918921aec1294ffcb77d8c5d0e69921a38 Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Tue, 5 Jun 2018 17:47:14 -0700 Subject: [PATCH 01/14] Put the ChangeCalculator implementation next to TargetRootsCalculator --- .../pants/init/target_roots_calculator.py | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/src/python/pants/init/target_roots_calculator.py b/src/python/pants/init/target_roots_calculator.py index 099ef4178cc..f7ca1c7c99f 100644 --- a/src/python/pants/init/target_roots_calculator.py +++ b/src/python/pants/init/target_roots_calculator.py @@ -24,6 +24,74 @@ logger = logging.getLogger(__name__) +class _DependentGraph(object): + """A graph for walking dependent addresses of TargetAdaptor objects. + + This avoids/imitates constructing a v1 BuildGraph object, because that codepath results + in many references held in mutable global state (ie, memory leaks). + + The long term goal is to deprecate the `changed` goal in favor of sufficiently good cache + hit rates, such that rather than running: + + ./pants --changed-parent=master test + + ...you would always be able to run: + + ./pants test :: + + ...and have it complete in a similar amount of time by hitting relevant caches. + """ + + @classmethod + def from_iterable(cls, target_types, adaptor_iter): + """Create a new DependentGraph from an iterable of TargetAdaptor subclasses.""" + inst = cls(target_types) + for target_adaptor in adaptor_iter: + inst.inject_target(target_adaptor) + return inst + + def __init__(self, target_types): + self._dependent_address_map = defaultdict(set) + self._target_types = target_types + + def inject_target(self, target_adaptor): + """Inject a target, respecting all sources of dependencies.""" + target_cls = self._target_types[target_adaptor.type_alias] + + declared_deps = target_adaptor.dependencies + implicit_deps = (Address.parse(s) + for s in target_cls.compute_dependency_specs(kwargs=target_adaptor.kwargs())) + + for dep in itertools.chain(declared_deps, implicit_deps): + self._dependent_address_map[dep].add(target_adaptor.address) + + def dependents_of_addresses(self, addresses): + """Given an iterable of addresses, yield all of those addresses dependents.""" + seen = set(addresses) + for address in addresses: + for dependent_address in self._dependent_address_map[address]: + if dependent_address not in seen: + seen.add(dependent_address) + yield dependent_address + + def transitive_dependents_of_addresses(self, addresses): + """Given an iterable of addresses, yield all of those addresses dependents, transitively.""" + addresses_to_visit = set(addresses) + while 1: + dependents = set(self.dependents_of_addresses(addresses)) + # If we've exhausted all dependencies or visited all remaining nodes, break. + if (not dependents) or dependents.issubset(addresses_to_visit): + break + addresses = dependents.difference(addresses_to_visit) + addresses_to_visit.update(dependents) + + transitive_set = itertools.chain( + *(self._dependent_address_map[address] for address in addresses_to_visit) + ) + for dep in transitive_set: + yield dep + + class _DependentGraph(object): """A graph for walking dependent addresses of TargetAdaptor objects. From f3a87932962df93d71a7a802aae3d975da97d299 Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Wed, 6 Jun 2018 10:03:52 -0700 Subject: [PATCH 02/14] Remove changed_calculator targets and dependencies --- src/python/pants/init/engine_initializer.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/python/pants/init/engine_initializer.py b/src/python/pants/init/engine_initializer.py index 9eefeefbfcd..4e5a7a63b55 100644 --- a/src/python/pants/init/engine_initializer.py +++ b/src/python/pants/init/engine_initializer.py @@ -27,6 +27,10 @@ from pants.engine.rules import SingletonRule from pants.engine.scheduler import Scheduler from pants.option.global_options import GlobMatchErrorBehavior +<<<<<<< 84084e918921aec1294ffcb77d8c5d0e69921a38 +======= +from pants.option.options_bootstrapper import OptionsBootstrapper +>>>>>>> Remove changed_calculator targets and dependencies from pants.util.objects import datatype From c788fa28381a5ec8b56d4ce989e0ff992ed2b169 Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Thu, 7 Jun 2018 14:28:35 -0700 Subject: [PATCH 03/14] Implement OwnerCalculator for target address calculation --- .../pants/init/target_roots_calculator.py | 42 ++++++++++++++++++- src/python/pants/option/global_options.py | 2 + 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/python/pants/init/target_roots_calculator.py b/src/python/pants/init/target_roots_calculator.py index f7ca1c7c99f..4684eee2514 100644 --- a/src/python/pants/init/target_roots_calculator.py +++ b/src/python/pants/init/target_roots_calculator.py @@ -195,11 +195,17 @@ def create(cls, options, session, symbol_table, build_root=None): # initialization paths. changed_options = options.for_scope('changed') changed_request = ChangedRequest.from_options(changed_options) - + owned_files = options.for_global_scope().owner_of logger.debug('spec_roots are: %s', spec_roots) logger.debug('changed_request is: %s', changed_request) 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 + logger.debug('owner_files are: %s', owned_files) + + if changed_request.is_actionable() and owned_files: + # We've been provided a change request AND an owner request. Error out. + raise InvalidSpecConstraint('cannot provide owner-of and changed parameters together') if change_calculator and changed_request.is_actionable(): if spec_roots: @@ -212,6 +218,16 @@ def create(cls, options, session, symbol_table, build_root=None): logger.debug('changed addresses: %s', changed_addresses) return TargetRoots(tuple(SingleAddress(a.spec_path, a.target_name) for a in changed_addresses)) + if owner_calculator and owned_files: + if spec_roots: + # We've been provided spec roots (e.g. `./pants list ::`) AND a owner request. Error out. + raise InvalidSpecConstraint('cannot provide owner-of parameters and target specs!') + # 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)) + return TargetRoots(spec_roots) @@ -279,3 +295,27 @@ def iter_changed_target_addresses(self, changed_request): def changed_target_addresses(self, changed_request): return list(self.iter_changed_target_addresses(changed_request)) + +class OwnerCalculator(object): + """A class for owner-of target calculation""" + + def __init__(self, scheduler, symbol_table): + """ + :param scheduler: The `Scheduler` instance to use for computing file to target mapping + :param symbol_table: The symbol table. + """ + self._scheduler = scheduler + self._symbol_table = symbol_table + self._mapper = EngineSourceMapper(self._scheduler) + + def iter_owner_target_addresses(self, owned_files): + """Given an list of owned files, compute and yield all affected target addresses""" + owner_addresses = set(address + for address + in self._mapper.iter_target_addresses_for_sources(owned_files)) + for address in owner_addresses: + yield address + return + + def owner_target_addresses(self, owner_request): + return list(self.iter_owner_target_addresses(owner_request)) \ No newline at end of file diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index f5a4adeafed..0d9588ba20d 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -183,6 +183,8 @@ def register_bootstrap_options(cls, register): register('--subproject-roots', type=list, advanced=True, fromfile=True, default=[], help='Paths that correspond with build roots for any subproject that this ' 'project depends on.') + register('--owner-of', help='Select the targets that own these files', type=list, default=[], + metavar='') # These logging options are registered in the bootstrap phase so that plugins can log during # registration and not so that their values can be interpolated in configs. From f27872d3a1c6d97baff039d6c812097fceee9d61 Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Thu, 7 Jun 2018 15:35:42 -0700 Subject: [PATCH 04/14] Comply with 2 spaces before and after classes --- src/python/pants/init/target_roots_calculator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/python/pants/init/target_roots_calculator.py b/src/python/pants/init/target_roots_calculator.py index 4684eee2514..557aafaace6 100644 --- a/src/python/pants/init/target_roots_calculator.py +++ b/src/python/pants/init/target_roots_calculator.py @@ -296,6 +296,7 @@ def iter_changed_target_addresses(self, changed_request): def changed_target_addresses(self, changed_request): return list(self.iter_changed_target_addresses(changed_request)) + class OwnerCalculator(object): """A class for owner-of target calculation""" From a018691e8be6958783c84a6b24107a8375b1aa9b Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Thu, 7 Jun 2018 16:51:56 -0700 Subject: [PATCH 05/14] Add deprecation warning to list-owners --- src/python/pants/backend/graph_info/tasks/list_owners.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/python/pants/backend/graph_info/tasks/list_owners.py b/src/python/pants/backend/graph_info/tasks/list_owners.py index 96bda02e175..282fa65c6e3 100644 --- a/src/python/pants/backend/graph_info/tasks/list_owners.py +++ b/src/python/pants/backend/graph_info/tasks/list_owners.py @@ -7,6 +7,7 @@ import json +from pants.base.deprecated import deprecated from pants.base.exceptions import TaskError from pants.build_graph.source_mapper import LazySourceMapper from pants.task.console_task import ConsoleTask @@ -51,3 +52,7 @@ def console_output(self, targets): if owner_info.values(): for address_spec in owner_info.values()[0]: yield address_spec + + @deprecated("1.8.0.dev3", "Run './pants --owner-of= list' instead") + def execute(self): + super(ListOwners, self).execute() From a32b40f250c87600915071225346f0c8a1e75d24 Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Thu, 7 Jun 2018 17:10:19 -0700 Subject: [PATCH 06/14] Warn the user about using --changed or specs along with --owner-of --- src/python/pants/init/target_roots_calculator.py | 2 +- src/python/pants/option/global_options.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/python/pants/init/target_roots_calculator.py b/src/python/pants/init/target_roots_calculator.py index 557aafaace6..ee5f149b507 100644 --- a/src/python/pants/init/target_roots_calculator.py +++ b/src/python/pants/init/target_roots_calculator.py @@ -319,4 +319,4 @@ def iter_owner_target_addresses(self, owned_files): return def owner_target_addresses(self, owner_request): - return list(self.iter_owner_target_addresses(owner_request)) \ No newline at end of file + return list(self.iter_owner_target_addresses(owner_request)) diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index 0d9588ba20d..701a22de9b4 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -183,8 +183,11 @@ def register_bootstrap_options(cls, register): register('--subproject-roots', type=list, advanced=True, fromfile=True, default=[], help='Paths that correspond with build roots for any subproject that this ' 'project depends on.') - register('--owner-of', help='Select the targets that own these files', type=list, default=[], - metavar='') + register('--owner-of', type=list, default=[], metavar='', + help='Select the targets that own these files.' + 'This is the third target calculation strategy along with the --changed' + 'options and specifying the targets directly. These three types of target' + 'selection are mutually exclusive.') # These logging options are registered in the bootstrap phase so that plugins can log during # registration and not so that their values can be interpolated in configs. From b555d966397d3038940bbe7ffbbb12623a8d72fb Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Thu, 7 Jun 2018 18:44:22 -0700 Subject: [PATCH 07/14] Remove duplicated class and unused import --- src/python/pants/init/engine_initializer.py | 4 -- .../pants/init/target_roots_calculator.py | 68 ------------------- 2 files changed, 72 deletions(-) diff --git a/src/python/pants/init/engine_initializer.py b/src/python/pants/init/engine_initializer.py index 4e5a7a63b55..9eefeefbfcd 100644 --- a/src/python/pants/init/engine_initializer.py +++ b/src/python/pants/init/engine_initializer.py @@ -27,10 +27,6 @@ from pants.engine.rules import SingletonRule from pants.engine.scheduler import Scheduler from pants.option.global_options import GlobMatchErrorBehavior -<<<<<<< 84084e918921aec1294ffcb77d8c5d0e69921a38 -======= -from pants.option.options_bootstrapper import OptionsBootstrapper ->>>>>>> Remove changed_calculator targets and dependencies from pants.util.objects import datatype diff --git a/src/python/pants/init/target_roots_calculator.py b/src/python/pants/init/target_roots_calculator.py index ee5f149b507..d483e3a9e8a 100644 --- a/src/python/pants/init/target_roots_calculator.py +++ b/src/python/pants/init/target_roots_calculator.py @@ -24,74 +24,6 @@ logger = logging.getLogger(__name__) -class _DependentGraph(object): - """A graph for walking dependent addresses of TargetAdaptor objects. - - This avoids/imitates constructing a v1 BuildGraph object, because that codepath results - in many references held in mutable global state (ie, memory leaks). - - The long term goal is to deprecate the `changed` goal in favor of sufficiently good cache - hit rates, such that rather than running: - - ./pants --changed-parent=master test - - ...you would always be able to run: - - ./pants test :: - - ...and have it complete in a similar amount of time by hitting relevant caches. - """ - - @classmethod - def from_iterable(cls, target_types, adaptor_iter): - """Create a new DependentGraph from an iterable of TargetAdaptor subclasses.""" - inst = cls(target_types) - for target_adaptor in adaptor_iter: - inst.inject_target(target_adaptor) - return inst - - def __init__(self, target_types): - self._dependent_address_map = defaultdict(set) - self._target_types = target_types - - def inject_target(self, target_adaptor): - """Inject a target, respecting all sources of dependencies.""" - target_cls = self._target_types[target_adaptor.type_alias] - - declared_deps = target_adaptor.dependencies - implicit_deps = (Address.parse(s) - for s in target_cls.compute_dependency_specs(kwargs=target_adaptor.kwargs())) - - for dep in itertools.chain(declared_deps, implicit_deps): - self._dependent_address_map[dep].add(target_adaptor.address) - - def dependents_of_addresses(self, addresses): - """Given an iterable of addresses, yield all of those addresses dependents.""" - seen = set(addresses) - for address in addresses: - for dependent_address in self._dependent_address_map[address]: - if dependent_address not in seen: - seen.add(dependent_address) - yield dependent_address - - def transitive_dependents_of_addresses(self, addresses): - """Given an iterable of addresses, yield all of those addresses dependents, transitively.""" - addresses_to_visit = set(addresses) - while 1: - dependents = set(self.dependents_of_addresses(addresses)) - # If we've exhausted all dependencies or visited all remaining nodes, break. - if (not dependents) or dependents.issubset(addresses_to_visit): - break - addresses = dependents.difference(addresses_to_visit) - addresses_to_visit.update(dependents) - - transitive_set = itertools.chain( - *(self._dependent_address_map[address] for address in addresses_to_visit) - ) - for dep in transitive_set: - yield dep - - class _DependentGraph(object): """A graph for walking dependent addresses of TargetAdaptor objects. From 09d3701d70e43be418fa08c5a9de9ba4bfa6791e Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Fri, 8 Jun 2018 09:41:44 -0700 Subject: [PATCH 08/14] Change target version for deprecating list-owners --- src/python/pants/backend/graph_info/tasks/list_owners.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/python/pants/backend/graph_info/tasks/list_owners.py b/src/python/pants/backend/graph_info/tasks/list_owners.py index 282fa65c6e3..1f8dee8957b 100644 --- a/src/python/pants/backend/graph_info/tasks/list_owners.py +++ b/src/python/pants/backend/graph_info/tasks/list_owners.py @@ -53,6 +53,6 @@ def console_output(self, targets): for address_spec in owner_info.values()[0]: yield address_spec - @deprecated("1.8.0.dev3", "Run './pants --owner-of= list' instead") + @deprecated("1.10.0.dev0", "Run './pants --owner-of= list' instead") def execute(self): super(ListOwners, self).execute() From cd41c2f42e8f3714f4ea7a7e7ea53ee74203b75c Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Fri, 8 Jun 2018 09:43:15 -0700 Subject: [PATCH 09/14] Simplify checking of invalid spec constraints --- .../pants/init/target_roots_calculator.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/python/pants/init/target_roots_calculator.py b/src/python/pants/init/target_roots_calculator.py index d483e3a9e8a..937051752d9 100644 --- a/src/python/pants/init/target_roots_calculator.py +++ b/src/python/pants/init/target_roots_calculator.py @@ -134,16 +134,18 @@ def create(cls, options, session, symbol_table, build_root=None): change_calculator = ChangeCalculator(session, symbol_table, scm) if scm else None owner_calculator = OwnerCalculator(session, symbol_table) if owned_files else None logger.debug('owner_files are: %s', owned_files) + targets_specified = [1 for spec + in (changed_request.is_actionable(), owned_files, spec_roots) + if spec] - if changed_request.is_actionable() and owned_files: - # We've been provided a change request AND an owner request. Error out. - raise InvalidSpecConstraint('cannot provide owner-of and changed parameters together') + if len(targets_specified) > 1: + # We've been provided a more than one of: a change request, an owner request, or spec roots. + raise InvalidSpecConstraint( + 'Only one target specification can be provided out of these three options: ' + '--changed-*, --owner-of=, or a specific target' + ) if change_calculator and changed_request.is_actionable(): - if spec_roots: - # We've been provided spec roots (e.g. `./pants list ::`) AND a changed request. Error out. - raise InvalidSpecConstraint('cannot provide changed parameters and target specs!') - # We've been provided no spec roots (e.g. `./pants list`) AND a changed request. Compute # alternate target roots. changed_addresses = change_calculator.changed_target_addresses(changed_request) @@ -151,9 +153,6 @@ def create(cls, options, session, symbol_table, build_root=None): return TargetRoots(tuple(SingleAddress(a.spec_path, a.target_name) for a in changed_addresses)) if owner_calculator and owned_files: - if spec_roots: - # We've been provided spec roots (e.g. `./pants list ::`) AND a owner request. Error out. - raise InvalidSpecConstraint('cannot provide owner-of parameters and target specs!') # 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) From 66ae1a99d56b808eac8b83e95899e950d6f74958 Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Fri, 8 Jun 2018 10:20:22 -0700 Subject: [PATCH 10/14] Add integration tests to owner_of_functionality --- tests/python/pants_test/engine/legacy/BUILD | 9 +++++++ .../engine/legacy/test_owners_integration.py | 25 +++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 tests/python/pants_test/engine/legacy/test_owners_integration.py diff --git a/tests/python/pants_test/engine/legacy/BUILD b/tests/python/pants_test/engine/legacy/BUILD index a2587060431..5c842a6afc1 100644 --- a/tests/python/pants_test/engine/legacy/BUILD +++ b/tests/python/pants_test/engine/legacy/BUILD @@ -53,6 +53,15 @@ python_tests( timeout = 600, ) +python_tests( + name = 'owners_integration', + sources = [ 'test_owners_integration.py' ], + dependencies = [ + 'tests/python/pants_test:int-test', + ], + tags = {'integration'}, +) + python_tests( name = 'dependees_integration', sources = ['test_dependees_integration.py'], diff --git a/tests/python/pants_test/engine/legacy/test_owners_integration.py b/tests/python/pants_test/engine/legacy/test_owners_integration.py new file mode 100644 index 00000000000..c6a3da4a2fa --- /dev/null +++ b/tests/python/pants_test/engine/legacy/test_owners_integration.py @@ -0,0 +1,25 @@ +# coding=utf-8 +# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +from __future__ import (absolute_import, division, generators, nested_scopes, print_function, + unicode_literals, with_statement) + +from pants_test.pants_run_integration_test import PantsRunIntegrationTest + + +class ListOwnersIntegrationTest(PantsRunIntegrationTest): + def test_owner_of_success(self): + pants_run = self.do_command('--owner-of=testprojects/tests/python/pants/dummies/test_pass.py', + 'list', + success=True) + self.assertEquals( + pants_run.stdout_data.strip(), + 'testprojects/tests/python/pants/dummies:passing_target' + ) + + def test_owner_list_not_owned(self): + pants_run = self.do_command('--owner-of=testprojects/tests/python/pants/dummies/test_nonexistent.py', + 'list', + success=True) + self.assertIn('WARNING: No targets were matched in', pants_run.stderr_data) \ No newline at end of file From fdb38d21184fa8856a3666c42aa1be0248931824 Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Fri, 8 Jun 2018 10:27:31 -0700 Subject: [PATCH 11/14] Set an empty owner_of on graph tests\nI dont love this self replicating mocks y'all --- tests/python/pants_test/engine/legacy/test_graph.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/python/pants_test/engine/legacy/test_graph.py b/tests/python/pants_test/engine/legacy/test_graph.py index fde0bf17a81..6d6d3031e7c 100644 --- a/tests/python/pants_test/engine/legacy/test_graph.py +++ b/tests/python/pants_test/engine/legacy/test_graph.py @@ -39,6 +39,7 @@ class GraphTestBase(unittest.TestCase): def _make_setup_args(self, specs): options = mock.Mock(target_specs=specs) options.for_scope.return_value = mock.Mock(diffspec=None, changes_since=None) + options.for_global_scope.return_value = mock.Mock(owner_of=None) return options def _default_build_file_aliases(self): From 0082c629030ca58df0681b3c8a9d7a4513cc1d5b Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Fri, 8 Jun 2018 14:10:29 -0700 Subject: [PATCH 12/14] Code cleaning and test for failing --owner-of calls --- .../pants/init/target_roots_calculator.py | 26 +++++++++++-------- .../engine/legacy/test_owners_integration.py | 24 ++++++++++++++++- 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/src/python/pants/init/target_roots_calculator.py b/src/python/pants/init/target_roots_calculator.py index 937051752d9..375fd8d00c5 100644 --- a/src/python/pants/init/target_roots_calculator.py +++ b/src/python/pants/init/target_roots_calculator.py @@ -1,5 +1,5 @@ # coding=utf-8 -# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md). +# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). from __future__ import (absolute_import, division, generators, nested_scopes, print_function, @@ -127,22 +127,25 @@ def create(cls, options, session, symbol_table, build_root=None): # initialization paths. changed_options = options.for_scope('changed') changed_request = ChangedRequest.from_options(changed_options) + + # Determine the `--owner-of=` arguments provided from the global options owned_files = options.for_global_scope().owner_of + logger.debug('spec_roots are: %s', spec_roots) 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 - logger.debug('owner_files are: %s', owned_files) - targets_specified = [1 for spec + targets_specified = sum(1 for item in (changed_request.is_actionable(), owned_files, spec_roots) - if spec] + if item) - if len(targets_specified) > 1: + if targets_specified > 1: # We've been provided a more than one of: a change request, an owner request, or spec roots. raise InvalidSpecConstraint( - 'Only one target specification can be provided out of these three options: ' - '--changed-*, --owner-of=, or a specific target' + 'Multiple target selection methods provided. Please use only one of ' + '--changed-*, --owner-of, or target specs' ) if change_calculator and changed_request.is_actionable(): @@ -163,7 +166,7 @@ def create(cls, options, session, symbol_table, build_root=None): class ChangeCalculator(object): - """A ChangeCalculator that find the target addresses of changed files based on scm.""" + """A ChangeCalculator that finds the target addresses of changed files based on scm.""" def __init__(self, scheduler, symbol_table, scm, workspace=None, changes_since=None, diffspec=None): @@ -229,7 +232,9 @@ def changed_target_addresses(self, changed_request): class OwnerCalculator(object): - """A class for owner-of target calculation""" + """An OwnerCalculator that finds the target addresses of the files passed down as arguments + to --owner-of + """ def __init__(self, scheduler, symbol_table): """ @@ -239,7 +244,7 @@ def __init__(self, scheduler, symbol_table): self._scheduler = scheduler self._symbol_table = symbol_table self._mapper = EngineSourceMapper(self._scheduler) - + def iter_owner_target_addresses(self, owned_files): """Given an list of owned files, compute and yield all affected target addresses""" owner_addresses = set(address @@ -247,7 +252,6 @@ def iter_owner_target_addresses(self, owned_files): in self._mapper.iter_target_addresses_for_sources(owned_files)) for address in owner_addresses: yield address - return def owner_target_addresses(self, owner_request): return list(self.iter_owner_target_addresses(owner_request)) diff --git a/tests/python/pants_test/engine/legacy/test_owners_integration.py b/tests/python/pants_test/engine/legacy/test_owners_integration.py index c6a3da4a2fa..ec18e2679d4 100644 --- a/tests/python/pants_test/engine/legacy/test_owners_integration.py +++ b/tests/python/pants_test/engine/legacy/test_owners_integration.py @@ -22,4 +22,26 @@ def test_owner_list_not_owned(self): pants_run = self.do_command('--owner-of=testprojects/tests/python/pants/dummies/test_nonexistent.py', 'list', success=True) - self.assertIn('WARNING: No targets were matched in', pants_run.stderr_data) \ No newline at end of file + self.assertIn('WARNING: No targets were matched in', pants_run.stderr_data) + + def test_owner_list_two_target_specs(self): + # Test that any of these combinations fail with the same error message. + expected_error = ('Multiple target selection methods provided. Please use only one of ' + '--changed-*, --owner-of, or target specs') + pants_run_1 = self.do_command('--owner-of=testprojects/tests/python/pants/dummies/test_pass.py', + '--changed-parent=master', + 'list', + success=False) + self.assertIn(expected_error, pants_run_1.stderr_data) + + pants_run_2 = self.do_command('--owner-of=testprojects/tests/python/pants/dummies/test_pass.py', + 'list', + 'testprojects/tests/python/pants/dummies:passing_target', + success=False) + self.assertIn(expected_error, pants_run_2.stderr_data) + + pants_run_3 = self.do_command('--changed-parent=master', + 'list', + 'testprojects/tests/python/pants/dummies:passing_target', + success=False) + self.assertIn(expected_error, pants_run_3.stderr_data) \ No newline at end of file From de6df607e4389490fff7f5b0714046ab7cacdedb Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Fri, 8 Jun 2018 14:26:51 -0700 Subject: [PATCH 13/14] s/2017/2018 --- .../python/pants_test/engine/legacy/test_owners_integration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/python/pants_test/engine/legacy/test_owners_integration.py b/tests/python/pants_test/engine/legacy/test_owners_integration.py index ec18e2679d4..f8a3951fc1b 100644 --- a/tests/python/pants_test/engine/legacy/test_owners_integration.py +++ b/tests/python/pants_test/engine/legacy/test_owners_integration.py @@ -1,5 +1,5 @@ # coding=utf-8 -# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md). +# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). from __future__ import (absolute_import, division, generators, nested_scopes, print_function, From 118031fc54dcafc2e677c90f4c493bc3f433cdea Mon Sep 17 00:00:00 2001 From: Alan Velasco Date: Fri, 8 Jun 2018 15:06:55 -0700 Subject: [PATCH 14/14] Add spaces in between sentences --- src/python/pants/option/global_options.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index 701a22de9b4..a5d9588130a 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -184,9 +184,9 @@ def register_bootstrap_options(cls, register): help='Paths that correspond with build roots for any subproject that this ' 'project depends on.') register('--owner-of', type=list, default=[], metavar='', - help='Select the targets that own these files.' - 'This is the third target calculation strategy along with the --changed' - 'options and specifying the targets directly. These three types of target' + help='Select the targets that own these files. ' + 'This is the third target calculation strategy along with the --changed ' + 'options and specifying the targets directly. These three types of target ' 'selection are mutually exclusive.') # These logging options are registered in the bootstrap phase so that plugins can log during