[Gnucash-changes] r12999 - gnucash/trunk/lib/libqof/qof - Re-enable logging for GnuCash modules that haven't explicitly set their

[Neil Williams]

On Saturday 28 January 2006 9:53 pm, Chris Shoemaker wrote:
> >    Set the logging level for all known log_modules.
> > Note: Unless a log_module has been registered using qof_log_set_level, it
> > will be unaffected by this change.
>
> Think about this.  _When_ is this function supposed to be called?

To modify the log levels _after_ initial (hard-coded) setup. The debug option 
simply moderates the level of debugging of the modules identified as being 
logged. This is exactly as it was before - global never did initialise the 
modules for logging, that was done by the assumption in the lists. Global 
simply moderated the AMOUNT of logging by tweaking the loglevel of each 
module in the assumed list. It still does that, without the assumption.

1. Developers enter debug macros, ENTER(" "), LEAVE (" ");
2. Developers call qof_log_set_level wherever it is needed to initialise this 
module for logging later.
3. Users set the amount to log by specifying --debug and/or specifying a debug 
integer level. (Developers are free to add a default OUTSIDE lib/libqof)
4. If it's a dynamic module, put the  qof_log_set_level call in the init 
function so that you don't need to include the #define from the header 
anywhere else, you get logging if it's loaded and it's ignored if it is not.

> > What that all means is that the API was not broken when it moved to
> > libqof, just clarified.
>
> The docs are clear that the API is broken. 

Nope. If the docs are unclear, I'll fix them but the API was fine until you 
broke it.

> Setting a global level 
> that has no effect on logging modules you don't explicitly register
> MAKES ZERO SENSE!  It's USELESS!

It's absolutely logical and sensible - plus it's flexible, scalable and 
generic. We cannot hardcode modules into QOF, neither can we assume that such 
lists of modules exist.

gnucash has to define those lists itself and the simplest way of doing that is 
to keep a list of those modules in a function that initialises them for 
logging. The --debug option can then accept an integer value that scales that 
logging up through the levels.

You initialise this in the main init code, then you use global to moderate the 
level at which those initialised modules are actually logged, should the user 
require a different level of logs.

At no point did global actually initialise the list of modules - it simply 
read the hard-coded assumption. This assumption had to die and it has. Global 
still does what it did before - changes the log level for each registered 
module in one call. The init was separate and is separate.

> > Note that even with gnc-trace.h, if a module was not listed in the
> > hardcoded list, it was also ignored.
>
> Ah, but they _were_ listed and they all WORKED. 

If you replicate the intentions of that list in gnucash (only), then they will 
continue to work. What QOF cannot do is assume that such a list exists.

> You're breaking 
> things and making GnuCash _harder_ to debug with changes that aren't
> even a good idea.  The logging system _needs_ to work.

It does work, when you setup the list to be logged - just as you had to with 
the gnc-trace headers. It's just that this way it's more flexible and easier 
to add new modules. (Because you can call qof_log_set_level anywhere in the 
codebase.)

> > This was an essential step in the use of QOF outside gnucash because the
> > hard coded list was completely irrelevant for any non-gnucash process.
> >
> > I accept I did not post to the list about the removal of this assumption
> > - instead I did clearly document the results in the Doxygen output by
> > making it explicit in the qoflog.h file.
> >
> > Please, Chris, can we revert your change and ditch the assumption?
>
> No.  The assumption is 

false

> sane and GnuCash depends on it. 

Sorry, it does not. It cannot. Assumptions are BAD.

> There is _no_  
> other way to globally control the logging level in the presence of
> dynamically loaded code.

Read the code again. Dynamically loaded code (like a module) can initialise 
it's own logging - you don't need global control except to control the AMOUNT 
of logging, not the LIST of log modules.

In the init function for the dynamic code, put a call to qof_log_set_level - 
easy. If the module is loaded, it's setup for logging. If it's not, nobody 
cares.

> BTW, notice that the only way you could have kept GnuCash's logging
> system working with this broken API would have been to collect all the
> used logging modules into one place and register them before setting
> the global level, which would have basically been trading the
> gnc-trace.h for a bunch of function calls.  Not smart.

No, that was simply a shorthand method for the base level modules. You can 
call qof_log_set_level anywhere you like. Feel free to move all those to more 
relevant places if you need to do so. The shorthand was a temporary measure 
that was useful during the transition.

You can even call qof_log_set_level from within a test function - you lose 
that ability if the list is hardcoded OR assumed.

> If anything should be reverted it's the commit that originally broke
> the logging.  If you want to try to drastically change the way
> gnucash's logging system works I suggest that you make your proposal
> to gnucash-devel, build some consensus, and respond to constructive
> criticism.

I am responding - but the problem is that you're stuck on this false 
assumption that will bite.

> Your breaking things by making major changes with no approval is bad
> enough.  Complaining when people fix things is BAD FORM.

You haven't fixed it, you've broken it.

It was fine before your patch - any lack of logging in gnucash was down to:
a) my lack of time to revisit the shorthand list
b) your misunderstanding of the API, without the assumption.

> > If there are issues you want clarified in the docs, I'll gladly do that,
> > but this patch is just plain wrong.
>
> The docs do need to be updated - to reflect this change.

OK - then help me see how it should be expressed in the docs and REVERT your 
broken patch, please.

-- 

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20060128/519d59cb/attachment-0001.bin