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

[Jiri Vanek]

On 03/14/2012 02:56 PM, Danesh Dadachanji wrote:
>
>
> 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.

It should timeout and dye. If not then it must be run in separated process and kill externally. I have not made deep investigations on this field. But sure is that awt robot to click close is still not presented ...:(

>
>>>> + }
>>>> }
>>>>
>>>
>>> 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