Bug 91884

Summary: nrwt swallows early exceptions
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: Tools / TestsAssignee: Balazs Kelemen <kbalazs>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Balazs Kelemen
Reported 2012-07-20 11:08:16 PDT
Well, it's annoying if you made an error implement a new command line switch for example and make a mistake. nrwt just fails to run but does not show the error.
Attachments
Patch (1.57 KB, patch)
2012-07-20 11:13 PDT, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
Patch (2.12 KB, patch)
2012-07-23 05:08 PDT, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
Patch (3.60 KB, patch)
2012-07-23 10:25 PDT, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Balazs Kelemen
Comment 1 2012-07-20 11:13:54 PDT
Created attachment 153546 [details] Patch
Dirk Pranke
Comment 2 2012-07-20 12:21:26 PDT
Comment on attachment 153546 [details] Patch argh. some day I'll get everything covered, and thanks for looking into this. However, this will end up printing the exception twice in most cases, so I don't think this is the right fix. I think we should move the stuff before the try block in main() into the try block instead.
Balazs Kelemen
Comment 3 2012-07-23 05:08:03 PDT
Created attachment 153777 [details] Patch
Balazs Kelemen
Comment 4 2012-07-23 06:53:42 PDT
Comment on attachment 153777 [details] Patch This does not work, we have to change the catch in main to catch BaseException's as well
Dirk Pranke
Comment 5 2012-07-23 09:52:01 PDT
(In reply to comment #4) > (From update of attachment 153777 [details]) > This does not work, we have to change the catch in main to catch BaseException's as well What exception are you trying to catch?
Balazs Kelemen
Comment 6 2012-07-23 10:24:25 PDT
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 153777 [details] [details]) > > This does not work, we have to change the catch in main to catch BaseException's as well > > What exception are you trying to catch? The issue was that _set_up_derived_options was still not guarded by a try.
Balazs Kelemen
Comment 7 2012-07-23 10:25:19 PDT
Created attachment 153815 [details] Patch
Dirk Pranke
Comment 8 2012-07-23 11:34:46 PDT
Comment on attachment 153815 [details] Patch Bah. Clearly I need to rework this whole area of code, as it seems if you have to be this careful about wrapping exceptions something must be wrong. But I don't want to hold this particular fix up any longer; thanks for working on it!
Balazs Kelemen
Comment 9 2012-07-23 14:21:41 PDT
Comment on attachment 153815 [details] Patch Clearing flags on attachment: 153815 Committed r123382: <http://trac.webkit.org/changeset/123382>
Balazs Kelemen
Comment 10 2012-07-23 14:21:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.