-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
console_rules can exit with exit codes #6654
console_rules can exit with exit codes #6654
Conversation
Depends on #6653 |
This feels like a thing that we might have existing concepts for... |
@@ -536,6 +536,10 @@ def products_request(self, products, subjects): | |||
throw_root_states = tuple(state for root, state in result.root_products if type(state) is Throw) | |||
if throw_root_states: | |||
unique_exceptions = tuple({t.exc for t in throw_root_states}) | |||
|
|||
if len(unique_exceptions) == 1 and isinstance(unique_exceptions[0], GracefulTerminationException): |
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.
Mmmm... this bit of magic is unpleasant.
It almost makes me wonder whether we shouldn't have a separate/explicit Scheduler entrypoint for @console_rule
? Would likely be useful for @wisechengyi's UX PR.
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.
Added a TODO for now
@@ -15,3 +16,15 @@ def fast_list(console, addresses): | |||
"""A fast variant of `./pants list` with a reduced feature set.""" | |||
for address in addresses.dependencies: | |||
console.print_stdout(address.spec) | |||
|
|||
|
|||
# This should really be registered somehow as an in-repo plugin when testing, rather than living |
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.
Making this an explicit TODO would be good.
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.
Doesn't TestBase support this via:
pants/tests/python/pants_test/test_base.py
Line 275 in 17ddd39
def rules(cls): |
Or do console rules need special setup that TestBase doesn't handle? A naive scan through the relevant code looks like it should all work.
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.
Moved into pants-plugins/src/python/internal_backend/rules_for_testing
9b3d18a
to
4164b9b
Compare
Rebased. |
@@ -15,3 +16,15 @@ def fast_list(console, addresses): | |||
"""A fast variant of `./pants list` with a reduced feature set.""" | |||
for address in addresses.dependencies: | |||
console.print_stdout(address.spec) | |||
|
|||
|
|||
# This should really be registered somehow as an in-repo plugin when testing, rather than living |
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.
Doesn't TestBase support this via:
pants/tests/python/pants_test/test_base.py
Line 275 in 17ddd39
def rules(cls): |
Or do console rules need special setup that TestBase doesn't handle? A naive scan through the relevant code looks like it should all work.
src/python/pants/base/exceptions.py
Outdated
@@ -62,3 +62,28 @@ class BackendConfigurationError(BuildConfigurationError): | |||
|
|||
class IncompatiblePlatformsError(Exception): | |||
"""Indicates that target platforms are incompatible with a target that contains native code.""" | |||
|
|||
|
|||
class GracefulTerminationException(Exception): |
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.
It's not clear why TaskError above with exit_code nonzero doesn't fit the bill. Granted there is the name and the fact it also carries failed_targets. Perhaps exceptions bound for the v2 rule world should live in a different file / package to help make it clear we're moving to new names? It really seems like this should live in pants.engine.rules.
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.
Moved into src/python/pants/rules/core/exceptions.py - happy to re-use TaskError
if that feels better to folks, but I quite like single-purpose exceptions for this kind of context
(And which will still be run)
This will eagerly halt execution.
4164b9b
to
802e6eb
Compare
This will eagerly halt execution.