After further discussion, I approve this change as is.
The main fatal error in Arguments.processArguments() only calls
Log.error if not verbose , so guarding it from having a timestamp when
verbose is not an issue.
/Andy
On 7/7/2020 7:20 PM, Andy Herrick wrote:
I agree - but I would rather then that Log.error() never show timestamp.
It would be better still if we could differentiate fatal error
messages from warning messages - right now we are using Log.error for
both.
/Andy
On 7/7/2020 6:51 PM, alexander.matv...@oracle.com wrote:
Hi Andy,
Timestamps for error message without verbose output are meaningless
in my opinion. This is why I did not add them. Also, in some cases
output does not look right. For example when timestamp is added to
error message always:
jpackage --someoption
WARNING: Using incubator modules: jdk.incubator.jpackage
[15:49:23.798] Error: Invalid Option: [--someoption]
In example above I do not see any point to have timestamp.
Thanks,
Alexander
On 7/7/20 6:18 AM, Andy Herrick wrote:
All looks good, except maybe Log.error().
I think Log.error() should have the same output whether verbose or
not, adding timestamp to the message only if verbose does not look
right.
Problem is that Log.error() is used for two fundamentally different
purposes:
1.) by Main and Arguments to display the final error message when
jpackage is failing.
2.) by everyone else to display a serious warning message when
jpackage is continuing (sometimes failing further on, and sometimes
not).
I wouldn't mind adding timestamps only when verbose in case (2), but
I don't think that's appropriate for case (1).
/ANdy
On 7/6/2020 8:27 PM, alexander.matv...@oracle.com wrote:
Please review the jpackage fix for bug [1] at [2].
Added timestamp to verbose and test output in form of [HH:mm:ss.SSS].
[1] https://bugs.openjdk.java.net/browse/JDK-8248261
[2] http://cr.openjdk.java.net/~almatvee/8248261/webrev.00/
Thanks,
Alexander
