[RFC][icedtea-web] Launch errors are not being printed to terminal.

[Danesh Dadachanji]

On 13/03/12 06:00 PM, Omair Majid wrote:
> On 03/13/2012 05:44 PM, Danesh Dadachanji wrote:
>> On 13/03/12 05:21 PM, Omair Majid wrote:
>>> On 03/13/2012 03:57 PM, Danesh Dadachanji wrote:
>>>>>   On 12/03/12 08:55 PM, Omair Majid wrote:
>>>>>>>   On 03/12/2012 10:59 AM, Danesh Dadachanji wrote:
>>>>>>>>>   I thought so too until I checked where printMessage() was
>>>>>> called from.
>>>>>>>>>   See launchWarning in GuiLaunchHandler[1], it explicitly calls
>>>>>>>>>   DefaultLaunchHandler.printMessage(). I didn't look into why it
>>>>>> was doing
>>>>>>>>>   that so if you think that's a bug, I can look into it further.
>>>>>>>>>
>>>>>>>
>>>>>>>   Assuming that we want to print the stack trace only in -verbose
>>>>> mode,
>>>>>>>   how does the attached patch look? (I did get a little side
>>>>> tracked and
>>>>>>>   fixed problems in printing the exceptions too).
>>>>>>>
>>>>>
>>>>>   This is way better than my patch! One question, why did you choose to
>>>>>   print the entire exception as opposed to just the cause? I realize
>>>>>   icedtea-web does it that way and it may even be better but I found
>>>> that
>>>>>   when I did that to test, the cause was repeated.
>>>>>
>>> I am not entirely clear on this. Do you mean the stack trace was
>>> repeated? As in it had duplicate "Caused by" lines?
>>>
>>>>>>>   I can rework this if others still think printing the entire
>>>>> stack trace
>>>>>>>   in -headless mode is a good idea.
>>>>>
>>>>>   No I think you have the right idea. I completely forgot about the
>>>> "Show
>>>>>   Details" button which can be seen as somewhat similar to supplying
>>>>>   -verbose. I'm fine with the one-liner in output.
>>>>>
>>>>>
>>>>>   Another thing, did you intentionally leave out the part that
>>>> prints the
>>>>>   exception to stderr with -verbose in GuiLaunchHandler?
>>> Doh. Sorry; it was a mistake.
>>>
>>>>>   If not, can you
>>>>>   include it in this patch?
>>> Done. How does the attached patch look?
>>>
>>
>> Great stuff, one more nitpick below and then it's fine with me.
>>
>>> diff --git a/netx/net/sourceforge/jnlp/GuiLaunchHandler.java
>>> b/netx/net/sourceforge/jnlp/GuiLaunchHandler.java
>>> --- a/netx/net/sourceforge/jnlp/GuiLaunchHandler.java
>>> +++ b/netx/net/sourceforge/jnlp/GuiLaunchHandler.java
>>
>> [snip]
>>
>>> @@ -71,6 +77,9 @@
>>>                    BasicExceptionDialog.show(exception);
>>>                }
>>>            });
>>> +        if (JNLPRuntime.isDebug()) {
>>> +            exception.printStackTrace(outputStream);
>>
>> Could you change that to printMessage(exception); instead? I think
>> consistency here between what is printed to terminal between GUI and
>> headless runs is better. We might base future tests off misinterpretations!
>
> Sorry, I was a little confused (the original patch had an 'if
> JNLPRuntime.isDebug()). I prefer printMessage here too because of the
> consistency. I should probably add unit tests for the GUI too, except
> it's a little coupled with a bunch of other things.
>

For something like this, GUI shouldn't be too troublesome, all you need 
to do is pass in the -verbose option to the javaws call. Jiri has passed 
in params in some other jnlp_tests, refer to those. The catch is _not_ 
running it in headless, I'm just not sure how you'd go about killing it 
and getting rid of that error dialog.

>>> +        }
>>>        }
>>>
>>
>> Also, you forgot to include AbstractLaunchHandler. =)
>>
>
> Whoops. The attached patch should address both the problems.
>
> Thanks for the multiple reviews!
>

My pleasure! Thanks for cleaning this all up. This looks good to me, go 
ahead and push.

Cheers,
Danesh