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

[engine] Selecting for ExecuteProcessResult will Throw on non-zero exit #6000

Merged
merged 2 commits into from Jun 22, 2018

Conversation

Projects
None yet
4 participants
@baroquebobcat
Copy link
Contributor

baroquebobcat commented Jun 21, 2018

This patch renames ExecuteProcessResult to FallibleExecuteProcessResult, and adds a rule to convert that to an ExecuteProcessResult. If the exit code is not zero, it raises ProcessExecutionFailure.

It moves ProcessExecutionFailure into isolated_process.py

Fixes #5719

[engine] Select for ExecuteProcessResult will throw by default
This patch renames ExecuteProcessResult to FallibleExecuteProcessResult, and adds a rule to convert that to an ExecuteProcessResult. If the exit code is not zero, it raises ProcessExecutionFailure

@baroquebobcat baroquebobcat requested a review from illicitonion Jun 21, 2018

@baroquebobcat baroquebobcat self-assigned this Jun 21, 2018

@baroquebobcat baroquebobcat requested a review from dotordogh Jun 21, 2018

"""Converts a FallibleExecuteProcessResult to a ExecuteProcessResult or raises an error."""

if fallible_result.exit_code == 0:
return ExecuteProcessResult(

This comment has been minimized.

@kwlzn

kwlzn Jun 21, 2018

Member

how would you feel about plumbing this raise on exit_code != 0 behavior as a defaulted parameter in an ExecuteProcessRequest with some override capability?

I think there will definitely be cases where we'll hit non-zero exit codes in a process execution but want to continue running.

one example would be: aggregating results of hundreds of pytest executions without e.g. --fast-fail. rather than Throw'ing on the first failure, we may want to aggregate those in a @console_rule and then fail at that level.

This comment has been minimized.

@kwlzn

kwlzn Jun 21, 2018

Member

ah - nm, maybe the idea is now that you'd request a FallibleExecuteProcessResult instead for this mode?

This comment has been minimized.

@baroquebobcat

baroquebobcat Jun 21, 2018

Contributor

Hm. I was going off of the design discussion in #5719.

You either request ExecuteProcessRequest, in which case you get an exception, or you request FallibleExecuteProcessRequest and you get an object you can inspect for failures.

In cases where we want to inspect the exit_code, we'd just request the FallibleExecuteProcessRequest one.

For things like aggregating 100s of results, I feel like it'd be good to have a more flexible API, but I don't know what it should look like precisely.

This comment has been minimized.

@kwlzn

kwlzn Jun 22, 2018

Member

yep, my bad - realized that just after I posted.

@kwlzn

kwlzn approved these changes Jun 22, 2018

Copy link
Member

kwlzn left a comment

lgtm!

For example, exiting with a non-zero code.
"""

MSG_FMT = """process '{desc}' failed with code {code}.

This comment has been minimized.

@kwlzn

kwlzn Jun 22, 2018

Member

with [exit] code?

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -86,11 +87,61 @@ def _verify_env_is_dict(cls, env):


class ExecuteProcessResult(datatype(['stdout', 'stderr', 'exit_code', 'output_directory_digest'])):
pass
"""Result of executing a process. Will raise an exception if the exit code is non-zero."""

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

Might be useful to say "Result of successfully executing a process".

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great, thanks!

Small handful more things to cover:
1: Can we also rename the rust type? I like the symmetry between the two languages, and having types with the same name having different semantics feels a little icky
2. Can you delete https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/graph_info/tasks/cloc.py#L92
3. Can we add a couple of small python unit tests to test_isolated_process.py which run /bin/bash -c "exit 1" requesting each product type, and seeing that the right thing happens?

@@ -86,11 +87,61 @@ def _verify_env_is_dict(cls, env):


class ExecuteProcessResult(datatype(['stdout', 'stderr', 'exit_code', 'output_directory_digest'])):

This comment has been minimized.

@illicitonion

illicitonion Jun 22, 2018

Contributor

ExecuteProcessResult probably doesn't have an exit_code any more, as it's guaranteed to always be 0

@@ -86,11 +87,61 @@ def _verify_env_is_dict(cls, env):


class ExecuteProcessResult(datatype(['stdout', 'stderr', 'exit_code', 'output_directory_digest'])):
pass
"""Result of executing a process. Will raise an exception if the exit code is non-zero."""

This comment has been minimized.

@illicitonion

illicitonion Jun 22, 2018

Contributor

The raising behaviour is a property of the rule which generates it, rather than the generated type, right? Maybe drop this sentence, and just leave the one on the @rule

This comment has been minimized.

@baroquebobcat

baroquebobcat Jun 22, 2018

Contributor

Hm. I'm not sure. Since this is the entry point for using the rule, and the rule isn't directly reference-able I feel like it makes sense for this to be here.

@baroquebobcat baroquebobcat merged commit ae6543c into pantsbuild:master Jun 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment