Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow pants to select targets by file(s) #5930

Merged
merged 14 commits into from Jun 9, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/python/pants/backend/graph_info/tasks/list_owners.py
Expand Up @@ -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
Expand Down Expand Up @@ -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=<file> list' instead")
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the docs aren't clear here... based on https://www.pantsbuild.org/deprecation_policy.html , let's target this to 1.10.0.dev0.

def execute(self):
super(ListOwners, self).execute()
43 changes: 42 additions & 1 deletion src/python/pants/init/target_roots_calculator.py
Expand Up @@ -127,11 +127,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a comment here, following the pattern for spec_roots and changed_options, and adding the blank line before and after.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/owner_files are/owned_files are/ ?


if changed_request.is_actionable() and owned_files:
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to unify checking of "exactly one of these three types of options" into one check here, with one large and helpful error message (not a paragraph probably, but a few sentences certainly explaining the three choices that exist). You could then remove the if spec_roots checks from the codepaths below.

# 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:
Expand All @@ -144,6 +150,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)


Expand Down Expand Up @@ -211,3 +227,28 @@ 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"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring could describe what the class does better.


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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xx the return, I think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't need this explicit return


def owner_target_addresses(self, owner_request):
return list(self.iter_owner_target_addresses(owner_request))
5 changes: 5 additions & 0 deletions src/python/pants/option/global_options.py
Expand Up @@ -183,6 +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', type=list, default=[], metavar='<path>',
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.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add spaces to the end of the lines so that when they're concatenated, they'll have separations.


# 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.
Expand Down