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

warn or error on matching source globs #5769

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
9e8e3f9
add formatting
cosmicexplorer May 19, 2018
1b8be97
implement the rust side entirely
cosmicexplorer May 19, 2018
237c143
add everything from the previous iteration
cosmicexplorer May 19, 2018
18e6d37
hook it all up
cosmicexplorer May 19, 2018
0545e83
make everything work (but comment out the snapshots)
cosmicexplorer May 19, 2018
5937ab9
everything works -- time to see if perf is affected
cosmicexplorer May 19, 2018
48bc133
fix a test real quick
cosmicexplorer May 19, 2018
a875318
make note about "optimization"
cosmicexplorer May 19, 2018
7c2a91c
add feature flag to GlobMatchErrorBehavior
cosmicexplorer May 19, 2018
9c15ca6
fix imports
cosmicexplorer May 19, 2018
049b528
don't keep path stats in hash map during glob expansion
cosmicexplorer May 21, 2018
44d1628
don't re-eval each time we want a python bool
cosmicexplorer May 21, 2018
d2661fc
format rust
cosmicexplorer May 21, 2018
d25d649
add tests, some fail, will fix
cosmicexplorer May 21, 2018
1e1d03d
add todo
cosmicexplorer May 21, 2018
ec6e318
a little bit of cleanup
cosmicexplorer May 21, 2018
ccec075
add fixme
cosmicexplorer May 21, 2018
9ec9307
implement fmt::Debug for Snapshot by providing Key#to_string()
cosmicexplorer May 21, 2018
473e14f
make the graph integration test into an integration test
cosmicexplorer May 21, 2018
7024613
we should be warning now, but we're not for some reason
cosmicexplorer May 22, 2018
8e47df6
remove eprintln!s
cosmicexplorer May 23, 2018
74f62d2
revert the PathGlob changes you silly
cosmicexplorer May 23, 2018
a1b72d4
yeah there's no "regression"
cosmicexplorer May 23, 2018
1995a30
refactor glob input matching to avoid re-traversing the whole tree at…
cosmicexplorer May 23, 2018
9f0ecc8
traverse from the leaves
cosmicexplorer May 23, 2018
ab05ce1
remove unused imports
cosmicexplorer May 23, 2018
fb985f1
provide a good error message
cosmicexplorer May 23, 2018
c889ecb
make some tests pass, keep some tests failing
cosmicexplorer May 23, 2018
60938e2
skip failing tests
cosmicexplorer May 23, 2018
9f83274
unremove the trait use
cosmicexplorer May 23, 2018
476c985
remove unused From impl
cosmicexplorer May 23, 2018
b3d6480
respond to some review comments
cosmicexplorer May 23, 2018
703e0e8
use singletons for GlobMatchErrorBehavior.create()
cosmicexplorer May 23, 2018
6e7f5ad
revert changes to structs.py
cosmicexplorer May 23, 2018
53dd5ac
revert some more unneeded changes
cosmicexplorer May 23, 2018
4ebb556
learn how to use project_ignoring_type
cosmicexplorer May 23, 2018
1ffa4a4
move things that should go into a later pr into todos
cosmicexplorer May 24, 2018
775ceca
clean up PathGlobs.create() a bit
cosmicexplorer May 24, 2018
0fc52cb
edit todos to point to newly created issue
cosmicexplorer May 24, 2018
3860dc8
clean up a little more
cosmicexplorer May 24, 2018
3b8f8d5
quick cleanup
cosmicexplorer May 24, 2018
af1ee61
document followup work
cosmicexplorer May 24, 2018
ab209b9
skip another test for now
cosmicexplorer May 24, 2018
1c11a56
make ci pass
cosmicexplorer May 24, 2018
72c2f39
remove unused import
cosmicexplorer May 24, 2018
3c1a92c
remove some clone impls
cosmicexplorer May 25, 2018
ec0ee4e
provide more error context in SourcesField#__str__()
cosmicexplorer May 25, 2018
95f22cb
add documentation for the amortized match checking strategy
cosmicexplorer May 26, 2018
5b8d271
refactor testing in response to review comments
cosmicexplorer May 26, 2018
b0fde87
convert todo to just comment
cosmicexplorer May 26, 2018
8135983
remove named TODOs
cosmicexplorer May 26, 2018
7559715
fix lint
cosmicexplorer May 26, 2018
c8b96d8
fix compile errors, adding descriptive comments
cosmicexplorer May 26, 2018
a2e505f
pass lint and rearrange file
cosmicexplorer May 26, 2018
34e4aa9
fix PathGlobs::create() calls and add some testing in engine/test_fs.py
cosmicexplorer May 26, 2018
ba772c4
remove repeated kwarg (an artifact from the rebase)
cosmicexplorer May 31, 2018
93ee21e
add strange change -- watch before trying this again
cosmicexplorer May 31, 2018
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
1 change: 1 addition & 0 deletions src/python/pants/bin/BUILD
Expand Up @@ -27,6 +27,7 @@ python_library(
'src/python/pants/engine:native',
'src/python/pants/engine:parser',
'src/python/pants/engine:scheduler',
'src/python/pants/engine:rules',
'src/python/pants/goal',
'src/python/pants/goal:context',
'src/python/pants/goal:run_tracker',
Expand Down
3 changes: 3 additions & 0 deletions src/python/pants/bin/goal_runner.py
Expand Up @@ -23,6 +23,7 @@
from pants.init.subprocess import Subprocess
from pants.init.target_roots_calculator import TargetRootsCalculator
from pants.java.nailgun_executor import NailgunProcessGroup
from pants.option.global_options import GlobMatchErrorBehavior
from pants.option.ranked_value import RankedValue
from pants.reporting.reporting import Reporting
from pants.scm.subsystems.changed import Changed
Expand Down Expand Up @@ -106,6 +107,8 @@ def _init_graph(self,
pants_ignore_patterns,
workdir,
self._global_options.build_file_imports,
glob_match_error_behavior=GlobMatchErrorBehavior.create(
self._global_options.glob_expansion_failure),
native=native,
build_file_aliases=self._build_config.registered_aliases(),
rules=self._build_config.rules(),
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/engine/BUILD
Expand Up @@ -50,6 +50,7 @@ python_library(
':rules',
':selectors',
'src/python/pants/base:project_tree',
'src/python/pants/option',
'src/python/pants/source',
'src/python/pants/util:meta',
'src/python/pants/util:objects',
Expand Down
36 changes: 30 additions & 6 deletions src/python/pants/engine/fs.py
Expand Up @@ -9,6 +9,7 @@

from pants.base.project_tree import Dir, File
from pants.engine.rules import RootRule
from pants.option.global_options import GlobMatchErrorBehavior
from pants.util.objects import Collection, datatype


Expand All @@ -29,24 +30,47 @@ class Path(datatype(['path', 'stat'])):
"""


class PathGlobs(datatype(['include', 'exclude'])):
class PathGlobs(datatype([
'include',
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 be tempted to add tuple type constraints to include and exclude - having half-typed objects seems weird

'exclude',
('glob_match_error_behavior', GlobMatchErrorBehavior),
])):
"""A wrapper around sets of filespecs to include and exclude.

The syntax supported is roughly git's glob syntax.

NB: this object is interpreted from within Snapshot::lift_path_globs() -- that method will need to
be aware of any changes to this object's definition.
"""

def __new__(cls, include, exclude, glob_match_error_behavior=None):
return super(PathGlobs, cls).__new__(
cls,
tuple(include),
tuple(exclude),
GlobMatchErrorBehavior.create(glob_match_error_behavior))

def with_match_error_behavior(self, glob_match_error_behavior):
return PathGlobs(
include=self.include,
exclude=self.exclude,
glob_match_error_behavior=glob_match_error_behavior)

@staticmethod
def create(relative_to, include, exclude=tuple()):
def create(relative_to, include, exclude=tuple(), glob_match_error_behavior=None):
"""Given various file patterns create a PathGlobs object (without using filesystem operations).

:param relative_to: The path that all patterns are relative to (which will itself be relative
to the buildroot).
:param included: A list of filespecs to include.
:param excluded: A list of filespecs to exclude.
:param include: A list of filespecs to include.
:param exclude: A list of filespecs to exclude.
:param glob_match_error_behavior: The value to pass to GlobMatchErrorBehavior.create()
:rtype: :class:`PathGlobs`
"""
return PathGlobs(tuple(join(relative_to, f) for f in include),
tuple(join(relative_to, f) for f in exclude))
encoded_reldir = relative_to.decode('utf-8')
return PathGlobs(include=tuple(join(encoded_reldir, f.decode('utf-8')) for f in include),
exclude=tuple(join(encoded_reldir, f.decode('utf-8')) for f in exclude),
glob_match_error_behavior=glob_match_error_behavior)


class PathGlobsAndRoot(datatype([('path_globs', PathGlobs), ('root', str)])):
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/engine/legacy/BUILD
Expand Up @@ -61,6 +61,7 @@ python_library(
'src/python/pants/engine:mapper',
'src/python/pants/engine:parser',
'src/python/pants/engine:selectors',
'src/python/pants/option',
'src/python/pants/source',
'src/python/pants/util:dirutil',
'src/python/pants/util:objects',
Expand Down
41 changes: 27 additions & 14 deletions src/python/pants/engine/legacy/graph.py
Expand Up @@ -24,6 +24,7 @@
from pants.engine.legacy.structs import BundleAdaptor, BundlesField, SourcesField, TargetAdaptor
from pants.engine.rules import TaskRule, rule
from pants.engine.selectors import Get, Select
from pants.option.global_options import GlobMatchErrorBehavior
from pants.source.wrapped_globs import EagerFilesetWithSpec, FilesetRelPathWrapper
from pants.util.dirutil import fast_relpath
from pants.util.objects import Collection, datatype
Expand Down Expand Up @@ -364,7 +365,9 @@ def _eager_fileset_with_spec(spec_path, filespec, snapshot, include_dirs=False):
fds = snapshot.path_stats if include_dirs else snapshot.files
files = tuple(fast_relpath(fd.path, spec_path) for fd in fds)

relpath_adjusted_filespec = FilesetRelPathWrapper.to_filespec(filespec['globs'], spec_path)
rel_include_globs = filespec['globs']

relpath_adjusted_filespec = FilesetRelPathWrapper.to_filespec(rel_include_globs, spec_path)
if filespec.has_key('exclude'):
relpath_adjusted_filespec['exclude'] = [FilesetRelPathWrapper.to_filespec(e['globs'], spec_path)
for e in filespec['exclude']]
Expand All @@ -375,33 +378,43 @@ def _eager_fileset_with_spec(spec_path, filespec, snapshot, include_dirs=False):
files_hash=snapshot.directory_digest.fingerprint)


@rule(HydratedField, [Select(SourcesField)])
def hydrate_sources(sources_field):
"""Given a SourcesField, request a Snapshot for its path_globs and create an EagerFilesetWithSpec."""

snapshot = yield Get(Snapshot, PathGlobs, sources_field.path_globs)
fileset_with_spec = _eager_fileset_with_spec(sources_field.address.spec_path,
sources_field.filespecs,
snapshot)
@rule(HydratedField, [Select(SourcesField), Select(GlobMatchErrorBehavior)])
def hydrate_sources(sources_field, glob_match_error_behavior):
"""Given a SourcesField, request a Snapshot for its path_globs and create an EagerFilesetWithSpec.
"""
# TODO(#5864): merge the target's selection of --glob-expansion-failure (which doesn't exist yet)
# with the global default!
path_globs = sources_field.path_globs.with_match_error_behavior(glob_match_error_behavior)
snapshot = yield Get(Snapshot, PathGlobs, path_globs)
fileset_with_spec = _eager_fileset_with_spec(
sources_field.address.spec_path,
sources_field.filespecs,
snapshot)
yield HydratedField(sources_field.arg, fileset_with_spec)


@rule(HydratedField, [Select(BundlesField)])
def hydrate_bundles(bundles_field):
@rule(HydratedField, [Select(BundlesField), Select(GlobMatchErrorBehavior)])
def hydrate_bundles(bundles_field, glob_match_error_behavior):
"""Given a BundlesField, request Snapshots for each of its filesets and create BundleAdaptors."""
snapshot_list = yield [Get(Snapshot, PathGlobs, pg) for pg in bundles_field.path_globs_list]
path_globs_with_match_errors = [
pg.with_match_error_behavior(glob_match_error_behavior)
for pg in bundles_field.path_globs_list
]
snapshot_list = yield [Get(Snapshot, PathGlobs, pg) for pg in path_globs_with_match_errors]

spec_path = bundles_field.address.spec_path

bundles = []
zipped = zip(bundles_field.bundles,
bundles_field.filespecs_list,
snapshot_list)
for bundle, filespecs, snapshot in zipped:
spec_path = bundles_field.address.spec_path
rel_spec_path = getattr(bundle, 'rel_path', spec_path)
kwargs = bundle.kwargs()
# NB: We `include_dirs=True` because bundle filesets frequently specify directories in order
# to trigger a (deprecated) default inclusion of their recursive contents. See the related
# deprecation in `pants.backend.jvm.tasks.bundle_create`.
kwargs['fileset'] = _eager_fileset_with_spec(getattr(bundle, 'rel_path', spec_path),
kwargs['fileset'] = _eager_fileset_with_spec(rel_spec_path,
filespecs,
snapshot,
include_dirs=True)
Expand Down
37 changes: 33 additions & 4 deletions src/python/pants/engine/legacy/structs.py
Expand Up @@ -55,7 +55,7 @@ def field_adaptors(self):
return tuple()
base_globs = BaseGlobs.from_sources_field(sources, self.address.spec_path)
path_globs = base_globs.to_path_globs(self.address.spec_path)
return (SourcesField(self.address, 'sources', base_globs.filespecs, path_globs),)
return (SourcesField(self.address, 'sources', base_globs.filespecs, base_globs, path_globs),)

@property
def default_sources_globs(self):
Expand All @@ -70,7 +70,7 @@ class Field(object):
"""A marker for Target(Adaptor) fields for which the engine might perform extra construction."""


Copy link
Sponsor Member

Choose a reason for hiding this comment

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

None of the changes in this file should be necessary anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! The changes have been reverted.

class SourcesField(datatype(['address', 'arg', 'filespecs', 'path_globs']), Field):
class SourcesField(datatype(['address', 'arg', 'filespecs', 'base_globs', 'path_globs']), Field):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add pydoc explaining what base_globs are? It's not self-evident...

"""Represents the `sources` argument for a particular Target.

Sources are currently eagerly computed in-engine in order to provide the `BuildGraph`
Expand All @@ -92,7 +92,8 @@ def __repr__(self):
return str(self)

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


class JavaLibraryAdaptor(TargetAdaptor):
Expand Down Expand Up @@ -173,6 +174,9 @@ def _construct_bundles_field(self):
rel_root = getattr(bundle, 'rel_path', self.address.spec_path)

base_globs = BaseGlobs.from_sources_field(bundle.fileset, rel_root)
# TODO: we want to have this field set from the global option --glob-expansion-failure, or
# something set on the target. Should we move --glob-expansion-failure to be a bootstrap
# option? See #5864.
path_globs = base_globs.to_path_globs(rel_root)

filespecs_list.append(base_globs.filespecs)
Expand Down Expand Up @@ -205,6 +209,7 @@ def field_adaptors(self):
sources_field = SourcesField(self.address,
'resources',
base_globs.filespecs,
base_globs,
path_globs)
return field_adaptors + (sources_field,)

Expand Down Expand Up @@ -288,6 +293,8 @@ def legacy_globs_class(self):
"""The corresponding `wrapped_globs` class for this BaseGlobs."""

def __init__(self, *patterns, **kwargs):
self._patterns = patterns
self._kwargs = kwargs
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Since this is mutated below, it will not end up containing anything interesting. Can fix in a followup, but IMO, attempting to reproduce the syntax here is probably overkill.

raw_spec_path = kwargs.pop('spec_path')
self._file_globs = self.legacy_globs_class.to_filespec(patterns).get('globs', [])
raw_exclude = kwargs.pop('exclude', [])
Expand Down Expand Up @@ -324,14 +331,36 @@ def _exclude_filespecs(self):
return []

def to_path_globs(self, relpath):
"""Return two PathGlobs representing the included and excluded Files for these patterns."""
"""Return a PathGlobs representing the included and excluded Files for these patterns."""
return PathGlobs.create(relpath, self._file_globs, self._excluded_file_globs)

def _gen_init_args_str(self):
all_arg_strs = []
positional_args = ', '.join([repr(p) for p in self._patterns])
if positional_args:
all_arg_strs.append(positional_args)
keyword_args = ', '.join([
'{}={}'.format(k, repr(v)) for k, v in self._kwargs.items()
])
if keyword_args:
all_arg_strs.append(keyword_args)

return ', '.join(all_arg_strs)

def __repr__(self):
return '{}({})'.format(type(self).__name__, self._gen_init_args_str())

def __str__(self):
return '{}({})'.format(self.path_globs_kwarg, self._gen_init_args_str())


class Files(BaseGlobs):
path_globs_kwarg = 'files'
legacy_globs_class = wrapped_globs.Globs

def __str__(self):
return '[{}]'.format(', '.join(repr(p) for p in self._patterns))


class Globs(BaseGlobs):
path_globs_kwarg = 'globs'
Expand Down
8 changes: 8 additions & 0 deletions src/python/pants/init/engine_initializer.py
Expand Up @@ -24,8 +24,10 @@
from pants.engine.mapper import AddressMapper
from pants.engine.native import Native
from pants.engine.parser import SymbolTable
from pants.engine.rules import SingletonRule
from pants.engine.scheduler import Scheduler
from pants.init.options_initializer import OptionsInitializer
from pants.option.global_options import GlobMatchErrorBehavior
from pants.option.options_bootstrapper import OptionsBootstrapper
from pants.scm.change_calculator import EngineChangeCalculator
from pants.util.objects import datatype
Expand Down Expand Up @@ -131,6 +133,7 @@ def setup_legacy_graph(pants_ignore_patterns,
build_root=None,
native=None,
build_file_aliases=None,
glob_match_error_behavior=None,
rules=None,
build_ignore_patterns=None,
exclude_target_regexps=None,
Expand All @@ -151,6 +154,9 @@ def setup_legacy_graph(pants_ignore_patterns,
:param Native native: An instance of the native-engine subsystem.
:param build_file_aliases: BuildFileAliases to register.
:type build_file_aliases: :class:`pants.build_graph.build_file_aliases.BuildFileAliases`
:param glob_match_error_behavior: How to behave if a glob specified for a target's sources or
bundles does not expand to anything.
:type glob_match_error_behavior: :class:`pants.option.global_options.GlobMatchErrorBehavior`
:param list build_ignore_patterns: A list of paths ignore patterns used when searching for BUILD
files, usually taken from the '--build-ignore' global option.
:param list exclude_target_regexps: A list of regular expressions for excluding targets.
Expand Down Expand Up @@ -194,6 +200,8 @@ def setup_legacy_graph(pants_ignore_patterns,
create_fs_rules() +
create_process_rules() +
create_graph_rules(address_mapper, symbol_table) +
[SingletonRule(GlobMatchErrorBehavior,
GlobMatchErrorBehavior.create(glob_match_error_behavior))] +
rules
)

Expand Down
1 change: 1 addition & 0 deletions src/python/pants/option/BUILD
Expand Up @@ -13,6 +13,7 @@ python_library(
'src/python/pants/util:eval',
'src/python/pants/util:memo',
'src/python/pants/util:meta',
'src/python/pants/util:objects',
'src/python/pants/util:strutil',
]
)
Expand Down
52 changes: 52 additions & 0 deletions src/python/pants/option/global_options.py
Expand Up @@ -16,6 +16,50 @@
from pants.option.optionable import Optionable
from pants.option.scope import ScopeInfo
from pants.subsystem.subsystem_client_mixin import SubsystemClientMixin
from pants.util.memo import memoized_method
from pants.util.objects import datatype


class GlobMatchErrorBehavior(datatype(['failure_behavior'])):
"""Describe the action to perform when matching globs in BUILD files to source 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.

Potentially controversial, but: given that this type will be validated on the rust side, I think removing the python wrapper type and just having it be a checked string would be totally fine. Take it or leave it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think the wrapper type is slightly overkill here :)

(Would maybe consider re-introducing it if it led to an easy way to keep the python and rust enums in sync, but I don't see an obvious one)

NB: this object is interpreted from within Snapshot::lift_path_globs() -- that method will need to
be aware of any changes to this object's definition.
"""

IGNORE = 'ignore'
WARN = 'warn'
ERROR = 'error'

allowed_values = [IGNORE, WARN, ERROR]

default_value = IGNORE

default_option_value = WARN

# FIXME(cosmicexplorer): add helpers in pants.util.memo for class properties and memoized class
# properties!
@classmethod
@memoized_method
def _singletons(cls):
return { behavior: cls(behavior) for behavior in cls.allowed_values }

@classmethod
def create(cls, value=None):
if isinstance(value, cls):
return value
if not value:
value = cls.default_value
return cls._singletons()[value]

def __new__(cls, *args, **kwargs):
this_object = super(GlobMatchErrorBehavior, cls).__new__(cls, *args, **kwargs)

if this_object.failure_behavior not in cls.allowed_values:
raise cls.make_type_error("Value {!r} for failure_behavior must be one of: {!r}."
.format(this_object.failure_behavior, cls.allowed_values))

return this_object


class GlobalOptionsRegistrar(SubsystemClientMixin, Optionable):
Expand Down Expand Up @@ -244,3 +288,11 @@ def register_options(cls, register):
register('--lock', advanced=True, type=bool, default=True,
help='Use a global lock to exclude other versions of pants from running during '
'critical operations.')
# TODO: Make a custom type abstract class (or something) to automate the production of an option
# with specific allowed values from a datatype (ideally using singletons for the allowed
# values).
register('--glob-expansion-failure', type=str,
choices=GlobMatchErrorBehavior.allowed_values,
default=GlobMatchErrorBehavior.default_option_value,
help="Raise an exception if any targets declaring source files "
"fail to match any glob provided in the 'sources' argument.")
1 change: 1 addition & 0 deletions src/python/pants/source/wrapped_globs.py
Expand Up @@ -163,6 +163,7 @@ def create_fileset_with_spec(cls, rel_path, *patterns, **kwargs):
:param rel_path: The relative path to create a FilesetWithSpec for.
:param patterns: glob patterns to apply.
:param exclude: A list of {,r,z}globs objects, strings, or lists of strings to exclude.
NB: this argument is contained within **kwargs!
"""
for pattern in patterns:
if not isinstance(pattern, string_types):
Expand Down