looks good.
/Andy
On 7/9/2020 12:02 AM, alexander.matv...@oracle.com wrote:
Hi Alexey,
http://cr.openjdk.java.net/~almatvee/8248261/webrev.01/
- Added fatalError() to log fatal errors without timestamp.
- Added missing timestamp to Log.verbose(Throwable t).
Thanks,
Alexander
On 7/8/20 9:34 AM, Alexey Semenyuk wrote:
I still think it would be good to create dedicated method for final
error reporting in Main and Arguments classes without timestamps.
- Alexey
On 7/8/2020 9:59 AM, Andy Herrick wrote:
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
