[mkgmap-dev] logging improvements

Sun Mar 28 15:43:27 BST 2021

Hi Gerd, 

To assist with examining the patch, most of the source files have very small
changes. Here is a breakdown:

RouteArc - remove commented System.out.println
ExtTypeAttribues - remove commented System.err.println
TypElement - remove commented System.out.println
ImgHeader - - remove commented System.out.println
Locator - - remove commented System.out.println

FileBackedImgFileWriter - replace one System.err.println
MdxFile - replace one System.err.println
MdrFile - replace one System.err.println
NumberStyle - replace one System.err.println
EchoAction - replace one System.err.println
EchoTagsAction - replace one System.err.println
AbstractOp - replace one System.err.println
HousenumberGenerator - replace one System.err.println
OsmXlmHandler - replace one System.err.println
RestrictionHelper - replace one System.err.println
LblFileReader - replace one dubious System.err.println("Map has tide
prediction, please implement! ")
OverviewBuilder - replace System.out.println (2) + remove commented one
CommandArgs - replace System.out.println and System.err.println (2)

TdbBuilder - remove unneeded error message
ImgFS - fix duplicated file name in error message

Utils - write stacktrace to log
FileInfo - write stacktrace to log
HGTReader- write stacktrace to log
CoastlineFileLoader- write stacktrace to log
O5mBinHandler - write stacktrace to log
OsmMapDataSource - write stacktrace to log
SeaGenerator - write stacktrace to log
PrecompSeaGenerator - write stacktrace to log
PrecompSeaSaver - write stacktrace to log
PrecompSeaMerger - write stacktrace to log (2)
HGTList - write stacktrace to log (2)
GmapiBuilder - write stacktrace to log (2)
NsisBuilder - write stacktrace to log (4)
LocatorConfig - write stacktrace to log + replace System.out.println

These last few have slightly more changes:

RoadNetwork - changes for --check-routing-island-len plus fix spelling
mistakes
RoutreNode - changes for displaying the output of roundabout, flare and
similar arc checks
Logger - changes to logging interface
UsefulFormatter - changes to logging interface
GmapsuppBuilder - several message changes
CommandArgsReader - minor change to facilitate not writing date and time if
not processing a file
Main - various improvements to logging
MapMaker - several replacements of System.err.println and log.err
StyledConverter - change to --report-dead-ends default + several
replacements of System.err.println and log.warn
StyleImpl - several replacements of System.err.println and log.err

Cheers,
Mike

-----Original Message-----
From: Gerd Petermann [mailto:gpetermann_muenchen at hotmail.com] 
Sent: 28 March 2021 10:17
To: Development list for mkgmap <mkgmap-dev at lists.mkgmap.org.uk>
Subject: Re: [mkgmap-dev] logging improvements

Hi Mike,

again: Please separate the changes regarding --check-routing-island-len from
those changes reg. logging.
It's a huge patch and I don't see a reason to mix the two things.

Gerd

________________________________________
Von: mkgmap-dev <mkgmap-dev-bounces at lists.mkgmap.org.uk> im Auftrag von Mike
Baggaley <mike at tvage.co.uk>
Gesendet: Donnerstag, 25. März 2021 11:15
An: 'Development list for mkgmap'
Betreff: Re: [mkgmap-dev] logging improvements

Hi Gerd,

Please find attached an updated patch.
Regarding your points:
1. I have been unable to fix the unit test. The patched version writes the
message to stdout when not using a logging.config file, but changing the
test from checkError to checkOutput does not fix the problem. Running the
test under the debugger, it appears that the test harness has nothing in
either the stdout or stderr stream, though the Eclipse console window
clearly shows output. This will need someone who understands the unit test
system better to fix the test.
2. Message fixed
3. Capitalisation fixed
4. Level.OFF is the correct value. The documentation refers to filtering
messages but does not mention use as a message severity.  Level.OFF is
Integer.MAX_VALUE and level.ALL is Integer.MIN_VALUE. Filtering works by
allowing messages of the filter value or higher. Hence using Level.OFF as a
message severity causes it to be always written, and Level.ALL will never be
written unless the filter level is also Level.All. I have added a comment to
the code to clarify this.
5.I have fixed a couple of SonarLint issues in the logging module, however
most of what it comes up with is of no consequence (there are hundreds of
warnings of dubious value).

I have also rethought the way error and warning messages that are unrelated
to input files are logged. Rather than using a write function using
Level.OFF as in the first version, I have added a global logger so that the
standard logging interface can be used. By default this global logger allows
errors and warnings though, whereas the other loggers only allow errors.
This means that program warnings can be displayed without having to display
OSM data warnings as well.

Cheers,
Mike

-----Original Message-----
From: Mike Baggaley [mailto:mike at tvage.co.uk]
Sent: 20 March 2021 12:55
To: 'Development list for mkgmap' <mkgmap-dev at lists.mkgmap.org.uk>
Subject: RE: [mkgmap-dev] logging improvements

HI Gerd, thanks for the reply

The new static method Logger.Write does not output any input file name and
is designed to output the same text as would have been output by
System.err.println. I may have used log.error rather than Logger.Write as a
replacement for System.err.println in a few places where I couldn't see a
reason why it hadn't been used - I'll have a look at these again.

1) Can you let me know how to run the unit tests?
2) I will change the message to correctly display -- report-routing-islands
(I initially started with --check-reporting-islands but decided report was
better, I must have omitted to change this message).
3) I am used to using capital letters for static functions -  I am happy to
change these to lower case.
4) I will look at using Level.ALL instead.
5) I will look into installing Sonarlint. I do get thirty-odd compiler
warnings, but none of these are related to the patch.

Cheers,
Mike

-----Original Message-----
From: Gerd Petermann [mailto:gpetermann_muenchen at hotmail.com]
Sent: 20 March 2021 08:28
To: Development list for mkgmap <mkgmap-dev at lists.mkgmap.org.uk>
Subject: Re: [mkgmap-dev] logging improvements

Hi Mike,

I agree in many points.
One reason to use System.err.println() instead of log.error() is that some
messages are not related to a specific input file and thus should not show
any input file name. Typically this is true in the combiners. I did not try
if your changes reflect this, but I think they don't?
The stacktraces help to indentify possible causes when users report
problems. I see no reason to remove them.

Please review,

1) unit test testWarningOnMismatchedCodePages fails.
2) The use of e.g. check-routing-island-len=1 gives message
"The --check-routing-island-len option is deprecated. Please use
--check-routing-islands and/or max-routing-island-len"
but --check-routing-islands is not a valid option. If possible, please
create a separate patch for the changes regarding options.
3) the new method names like Logger.Write or Logger.WriteStackTrace do not
comply with naming conventions, method names should not start with upper
case characters
4) The use of Level.OFF is confusing. My understanding is that the constant
is meant to disable logging?
5) Sonarlint complains about "Preconditions" and logging arguments should
not require evaluation (java:S2629) but that's not new with your patch.

Gerd

________________________________________
Von: mkgmap-dev <mkgmap-dev-bounces at lists.mkgmap.org.uk> im Auftrag von Mike
Baggaley <mike at tvage.co.uk>
Gesendet: Samstag, 20. März 2021 00:52
An: 'mkgmap development'
Betreff: [mkgmap-dev] logging improvements

Dear all,

Please find attached a first stab at improving the way error and other
messages are written, which at present is an ad hoc mixture of writing to
stderr, stdout and using Java logging, with no apparent logical structure.
Writing to stderr or stdout is not thread safe and causes problems when
max_jobs is greater than 1. When stderr is redirected to a file and threads
simultaneously attempt to write to it, you end up with extra numbered log
files containing a few lines that you would expect to be in the named log
file. As mkgmap supports a configuration file that defines how messages
should be logged, it doesn't make much sense writing a significant number of
messages to stderr which is not under logging control.

There are several diagnostic options that write their output using warning
as the severity, but as this is not enabled by default in the logging, they
appear to do nothing unless a logging configuration file is also used, which
is not intuitive. The --check-routing-island-len option seems somewhat
confused as it attempts to combine both reporting and fixing depending on
the value supplied. It also requires info level of logging which would
include a lot of other not necessarily wanted messages. If you are using it
to fix, you don't necessarily also want any messages.

In my view the way messages should be handled is as follows:

If the message is an informational message about the program itself and is
unrelated to processing any data then write it to stdout - this would apply
to the output of --version and --help.
If the message is an error message about an invalid parameter in the command
line, write it to stderr.
Once the command line has been processed sufficiently that a file is to be
processed then write messages using logging.
If a diagnostic option is enabled, the program should automatically log the
requested diagnostic messages. This would apply to --check-roundabouts,
--check-roundabout-flares, --report-similar-arcs, --check-routing-island-len
and --report-dead-ends.

The attached patch enhances the existing logging interface by providing the
following:
A mechanism for writing messages without any formatting getting added and
that always get written whatever level of logging is enabled.
A mechanism for writing stack trace information to the log file (I don't
believe stack traces really have any place in released code but have not
attempted to replace any of these).
Mostly directs messages as per my above proposal, though there is the
occasional place where the best option is not quite obvious to me, as I
think some of the code is not part of the main mkgmap program (there are
several main functions and some of the code is for testing purposes).
Only logs the start and finish times if files are being processed.
Outputs requested diagnostic messages regardless of enabled logging levels
Changes --report-dead-ends to be off by default, with the value of 1 being
the default if --report-dead-ends is specified without a value.
Filters duplicated --report-similar-arcs messages and ignores synthesised
arcs so that it can be used together with --make-opposite-cycleways.
Deprecates --check-routing-island-len, replacing it with
--report-routing-islands to handle reporting and --max-routing-island-len to
handle fixing.

I expect there will be the odd place where others may disagree with the
logging level for a message, but no doubt we can come to a consensus.
The patch touches quite a few source files, but there are only a few changes
that are anything more than altering the message outputting mechanism. There
is no change to any map outputs.

Cheers,
Mike