Bug 103537 – [webkitpy] Raise a ScriptError where it can be handled instead of directly using sys.exit

• Component: New Bugs
• Status: NEW

[Zan Dobersek]

[webkitpy] Raise a ScriptError where it can be handled instead of directly using sys.exit

[Zan Dobersek]

Created attachment 176521 [details]
Patch

[Zan Dobersek]

Comment on attachment 176521 [details]
Patch

Removing the cq flag as this patch depends on the patch posted at bug #103532 for correct logging output.

[Dirk Pranke]

Comment on attachment 176521 [details]
Patch


ScriptError is used by Executives to indicate that a called subprocess returned an exit code; you're using this for totally different reasons that have nothing to do with this, making the meaning of ScriptError much less clear. Many of the changes in the commands seems like they could be replaced with a common exception, but ScriptError is the wrong thing to use. Maybe add a generic StepException or CommandException to indicate fatal errors in commands ...

Further, using the same exception for various command-related errors and various internal errors seems confusing. For example, the change in scm.ensure_no_local_commits() doesn't seem like it should be an exception at all (since it seems like an error that could easily happen), and the change in scm.commit_locally_with_message seems like it should be an AssertionError or a NotImplementedError instead.


Lastly, it's not clear what a caller is supposed to when many of these errors are raised; if ScriptError is raised; if there's nothing they can reasonably do other than call sys.exit (which seems to be what happens in abstractsequencedcommand.py), it doesn't seem like this is much of an improvement over calling sys.exit directly.
ScriptError, on the other hand, is almost never used to indicate "I give up, so you should exit"; most ScriptError exceptions are recoverable (frankly, I think ScriptError is mostly used for bad reasons where the caller was being lazy :().

[Zan Dobersek]

Thanks for the input, the patch is clearly wrong in that regard. I'll still try to minimize the number of calls to sys.exit by replacing them with a more proper method. I'll rename this bug once I approach this problem again.

[Eric Seidel (no email)]

We could also add an executive.exit() instead if you like.

sys.exit isn't ideal, but my understanding is the python unittest harness can handle it.  I believe unittest overrides sys.exit to throw an exception instead, or maybe sys.exit always throws an exception.

[Eric Seidel (no email)]

I guess I'm not sure what the goal of this bug is? :)