-
-
Notifications
You must be signed in to change notification settings - Fork 632
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
Changes from all commits
9e8e3f9
1b8be97
237c143
18e6d37
0545e83
5937ab9
48bc133
a875318
7c2a91c
9c15ca6
049b528
44d1628
d2661fc
d25d649
1e1d03d
ec6e318
ccec075
9ec9307
473e14f
7024613
8e47df6
74f62d2
a1b72d4
1995a30
9f0ecc8
ab05ce1
fb985f1
c889ecb
60938e2
9f83274
476c985
b3d6480
703e0e8
6e7f5ad
53dd5ac
4ebb556
1ffa4a4
775ceca
0fc52cb
3860dc8
3b8f8d5
af1ee61
ab209b9
1c11a56
72c2f39
3c1a92c
ec0ee4e
95f22cb
5b8d271
b0fde87
8135983
7559715
c8b96d8
a2e505f
34e4aa9
ba772c4
93ee21e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
|
@@ -70,7 +70,7 @@ class Field(object): | |
"""A marker for Target(Adaptor) fields for which the engine might perform extra construction.""" | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None of the changes in this file should be necessary anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add pydoc explaining what |
||
"""Represents the `sources` argument for a particular Target. | ||
|
||
Sources are currently eagerly computed in-engine in order to provide the `BuildGraph` | ||
|
@@ -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): | ||
|
@@ -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) | ||
|
@@ -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,) | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', []) | ||
|
@@ -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' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
@@ -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.") |
There was a problem hiding this comment.
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 toinclude
andexclude
- having half-typed objects seems weird