-
-
Notifications
You must be signed in to change notification settings - Fork 638
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
Incremental strategy for running/skipping targets via tag #6947
Changes from 5 commits
07b2ce0
8dc895c
e402adc
1911841
2742ca2
a6e03ba
57506f2
3e0f335
26bf023
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,6 +55,10 @@ class HasSkipOptionMixin(object): | |
def skip_execution(self): | ||
return self.get_options().skip | ||
|
||
@property | ||
def supports_skipping_target_by_tag(self): | ||
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. I'd call this |
||
return True | ||
|
||
|
||
class SkipOptionRegistrar(object): | ||
"""Registrar of --skip.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -210,6 +210,15 @@ def skip_execution(self): | |
""" | ||
return False | ||
|
||
@property | ||
def supports_skipping_target_by_tag(self): | ||
""" | ||
Whether this task supports forced skip/non-skip on targets based on tags. | ||
|
||
:API: public | ||
""" | ||
return False | ||
|
||
@property | ||
def act_transitively(self): | ||
"""Whether this task should act on the transitive closure of the target roots. | ||
|
@@ -237,8 +246,30 @@ def get_targets(self, predicate=None): | |
|
||
:API: public | ||
""" | ||
return (self.context.targets(predicate) if self.act_transitively | ||
else list(filter(predicate, self.context.target_roots))) | ||
relevant_targets = self.context.targets(predicate) if self.act_transitively else list( | ||
filter(predicate, self.context.target_roots)) | ||
if not self.supports_skipping_target_by_tag: | ||
return relevant_targets | ||
|
||
force_run_tag = 'no-{}.skip'.format(self.options_scope) | ||
force_run_targets = set(t for t in relevant_targets if force_run_tag in t.tags) | ||
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 use a set comprehension with {}. |
||
|
||
if not force_run_targets and self.skip_execution: | ||
self.context.log.info('Skipping by returning empty targets ' | ||
'because task level is skipped and there is no forced run targets') | ||
return [] | ||
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.
|
||
|
||
force_skip_tag = '{}.skip'.format(self.options_scope) | ||
force_skip_targets = set(t for t in relevant_targets if force_skip_tag in t.tags) | ||
|
||
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. Ditto on set comprehension |
||
if force_skip_targets: | ||
self.context.log.debug("Force skipping targets by tag: {}\n{}".format(force_skip_tag, force_skip_targets)) | ||
if force_run_targets: | ||
self.context.log.debug("Force running targets by tag: {}\n{}".format(force_run_tag, force_run_targets)) | ||
|
||
final_targets = list(set(relevant_targets).union(force_run_targets).difference(force_skip_targets)) | ||
|
||
return final_targets | ||
|
||
@memoized_property | ||
def workdir(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,8 +19,10 @@ class EchoTaskBase(HasSkipAndTransitiveGoalOptionsMixin, Task): | |
prefix = None | ||
|
||
def execute(self): | ||
with open(os.path.join(self.workdir, 'output'), 'w') as fp: | ||
fp.write('\n'.join(t.address.spec for t in self.get_targets())) | ||
relevant_targets = self.get_targets() | ||
if relevant_targets: | ||
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. Do we not want to write an empty file if relevant_targets is empty? Seems more consistent. |
||
with open(os.path.join(self.workdir, 'output'), 'w') as fp: | ||
fp.write('\n'.join(t.address.spec for t in relevant_targets)) | ||
|
||
|
||
class EchoOne(EchoTaskBase): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,21 +43,21 @@ def get_echo(which): | |
return None | ||
else: | ||
with open(path, 'r') as fp: | ||
return [os.path.basename(x.strip()) for x in fp.readlines()] | ||
return {os.path.basename(x.strip()) for x in fp.readlines()} | ||
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. changed these to sets since the target list ordering isn't consistent. |
||
self.assertEqual(expected_one, get_echo('one')) | ||
self.assertEqual(expected_two, get_echo('two')) | ||
|
||
def test_defaults(self): | ||
self._do_test_goal_options([], ['foo:a', 'foo:b'], ['foo:a', 'foo:b']) | ||
self._do_test_goal_options([], {'foo:a', 'foo:b'}, {'foo:a', 'foo:b'}) | ||
|
||
def test_set_at_goal_level(self): | ||
self._do_test_goal_options(['--skip'], None, None) | ||
|
||
def test_set_at_task_level(self): | ||
self._do_test_goal_options(['--echo-one-skip'], None, ['foo:a', 'foo:b']) | ||
self._do_test_goal_options(['--no-echo-two-transitive'], ['foo:a', 'foo:b'], ['foo:a']) | ||
self._do_test_goal_options(['--echo-one-skip'], None, {'foo:a', 'foo:b'}) | ||
self._do_test_goal_options(['--no-echo-two-transitive'], {'foo:a', 'foo:b'}, {'foo:a'}) | ||
|
||
def test_set_at_goal_and_task_level(self): | ||
self._do_test_goal_options(['--skip', '--no-echo-one-skip'], ['foo:a', 'foo:b'], None) | ||
self._do_test_goal_options(['--skip', '--no-echo-one-skip'], {'foo:a', 'foo:b'}, None) | ||
self._do_test_goal_options(['--no-transitive', '--echo-two-transitive'], | ||
['foo:a'], ['foo:a', 'foo:b']) | ||
{'foo:a'}, {'foo:a', 'foo:b'}) |
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.
Ref 1
Need to remove this because if a target says
no-...-skip
in the tag, we can only find out at the task level by filtering targets.