-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
add PipelineError to capture exit code #26722
Conversation
Alternative to #26719 |
error("Downloading files requires Windows Management Framework 3.0 or later.") | ||
end | ||
rethrow(e) | ||
end |
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.
This should probably be a separate commit.
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.
Same PR okay?
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.
Sure.
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.
On the triage call it was noted that we might not want to actually use this style of error handling --- we can use wait=false
, call wait
on the process object, and then look at the exit code.
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.
In that case, should we export pipeline_error
(or some sort of "throw error based on exit code" function?)
ccf8e52
to
6efcfdd
Compare
@@ -707,29 +707,38 @@ with a code of 0). An exception is raised if the process cannot be started. | |||
""" | |||
success(cmd::AbstractCmd) = success(_spawn(cmd)) | |||
|
|||
struct PipelineError{P} <: 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.
Maybe call it ProcessError
since it has to do with process failure, and there is not necessarily a pipeline (might be 1 process)?
Some bikeshedding: this could be called struct ProcessError <: Exception
process::Process
pipeline::Union{Nothing,Pipeline}
end Where the |
Triage is in favor of the better error type; @vtjnash doesn't want to use it for this particular use case, favoring a |
Can't you have multiple failures in the one pipeline? |
Sure, that's why it has the pipeline field as well. |
Okay, but which one do we report in the |
The first one that failed, presumably. |
This can still be done but doesn't block 1.0. |
Looks like it was fixed by #27900 |
At the moment if a process fails and throws and error, there is no easy way to capture the exit code. The only other option is to run asynchronously and manually check (but then there is no easy way to throw an error...)