Disable log4cxx dependency in logging API

[stephen]

The logging API does not work with versions of log4cxx earlier than 0.9.8.

The suggested fix is to temporarily disable log4cxx and leave the API writing directly to stdout.

[stephen]

Commit 0afb9dde6cef8abfa38935e85030ab975002cf91 ready for review.

Have separated out the API and implementation (which is ticket #529, so will be closed with this one) and have provided an implementation that writes its output to stdout.

[stephen]

I'm adding the fixes to points addressed by jinmei (#559) and will put it back for review when I've finished.

[stephen]

Commit 2ebf994dfcb704b90f9b1dcccbf56a4b38c84a36 ready for review. See ticket #559 for a response to the bug report concerning initialization problems.

[stephen]

Updated before review to bf5f7b5f56ed649f78206fa2c3b4f9acdee1e310 to fix problem in FreeBSD distribution check.

[jelte]

I'm unsure what the goal (and/or the next step) is now; I had the following comments, until I realized that the whole purpose was to really remove log4cxx use, but only 'temporarily', as it says in the ticket, so what to do about this?

lib/log/Makefile.am (also) contains a few commented-out includes (for LOG4CXX) that can probably be removed now (as well as the commented-out part of configure.ac)

is logger_impl_log4cxx.h still used (or cc for that matter)?

The pimpl use itself (in logger_impl.h) seems a bit off, isn't the idea not to actually *define* the xxxImpl class in the headers, but to only name it and let the .cc define it?

Minor points for the code:

is lib/log/tests/xdebuglevel_unittest still used? (it got commented out in the makefile) (perhaps also related to above)

Really minor (and i mean MINOR, one would almost call it trivial) naming point:
LoggerImpl?* getLoggerptr() {
It's private and all, just wanted to mention that we tend to capitalize 'ptr' in the rest of our code :)
(although, come to think of it, we use the Ptr postfix usually to name a shared_ptr)

You might want to have the generator include some comments before that initializer and instantiator as to why they are necessary, and why they have such a weird name (and/or why that doesn't matter).

LoggerImpl?, lots of lexical casts there (because of the change of MessageID I guess), which seem unnecessary in this context, why not pass the ident and text as separate arguments to output(), and have that function operator<<() them separately?

I am not sure I oppose your suggestion at the bottom of #559, btw, perhaps more of this actually warrants a focused phone discussion?

Apart from this, the code looks ok, btw :) (though perhaps Jinmei wants to have a quick look at the changes for the init problem)

[jelte]

oh, two more things, I pushed a change with some lower-than-trivial fixes for documentation, and there are of course some problems on solaris (which I think is just a case of the right #includes, looking at it)

[jelte]

fix for sunstudio also pushed, commit 7b51465130cede5f68994d4535b86ece6cc2f11d

[stephen]

I'm unsure what the goal (and/or the next step) is now; I had the following comments,
until I realized that the whole purpose was to really remove log4cxx use, but only
'temporarily', as it says in the ticket, so what to do about this?

lib/log/Makefile.am (also) contains a few commented-out includes (for LOG4CXX) that can
probably be removed now (as well as the commented-out part of configure.ac)

is logger_impl_log4cxx.h still used (or cc for that matter)?

I suggest that the redundant files (logger_impl_logcxx.* and xdebuglevel.*) are left in the distribution for now, until we decide what packages we're using (at the Prague meeting). The file documentation.txt does mention them and notes how to get back to using log4cxx.

The pimpl use itself (in logger_impl.h) seems a bit off, isn't the
idea not to actually *define* the xxxImpl class in the headers, but
to only name it and let the .cc define it?

As I understand it, pimpl means that the client code using the public API - in this case code using logger.h - is not exposed to the internals of the implementation. How the implementation class is laid out - defined in the .cc file or in its own .cc file - is not part of pimpl.

is lib/log/tests/xdebuglevel_unittest still used? (it got commented out in the makefile)
(perhaps also related to above)

No - it is related to the above.

Really minor (and i mean MINOR, one would almost call it trivial) naming point:
LoggerImpl?* getLoggerptr() {
It's private and all, just wanted to mention that we tend to capitalize 'ptr' in the rest of
our code :) (although, come to think of it, we use the Ptr postfix usually to name a shared_ptr)

I'll change it - "Logger" and "Ptr" are two words so should be caiptalised. It also occurs to me that I should disable the copy constructor and assignment operator. (Copying loggers makes no sense - just create another one.)

You might want to have the generator include some comments before that initializer and
instantiator as to why they are necessary, and why they have such a weird name (and/or
why that doesn't matter).

Done.

LoggerImpl?, lots of lexical casts there (because of the change of MessageID I guess), which
seem unnecessary in this context, why not pass the ident and text as separate arguments to
output(), and have that function operator<<() them separately?

Good catch, done.

I am not sure I oppose your suggestion at the bottom of #559, btw, perhaps more of this
actually warrants a focused phone discussion?

OK, I'll organise something.

Apart from this, the code looks ok, btw :) (though perhaps Jinmei wants to have a quick
look at the changes for the init problem)

I'll pass it to Jinmei for his comments.

[jinmei]

Replying to stephen:

Apart from this, the code looks ok, btw :) (though perhaps Jinmei wants to have a quick
look at the changes for the init problem)

I'll pass it to Jinmei for his comments.

Okay, I've taken a closer look at the code, focusing on:

• the initialization fiasco for RootLoggerName?
• initialization of the default messages (messagedef.h)

As for RootLoggerName?, the original problem seems to be solved. But I
now wonder whether it has to be a class. Unlike the Logger class or
messages, this must be explicitly initialized in the application (such
as b10-xxx) in some way, right? And since this is done in the
application (which should have main()), we can control the
initialization timing. So, in practice, it should be sufficient to
provide a set of free functions like "setRootLoggerName()" (which
would do the same thing as the RootLoggerName? constructor. The
function based interface is more restrictive, but the behavior will be
more intuitive and provide more predictable behavior, which I would
rather prefer in this case.

As for the changes to messagedef.h, I have several questions.

1. is it really guaranteed that the symbol for the MessageInitializer? object remains? It's statically detectable that instantiate_messagedef_xxx is never used and its construction is actually no-op, so I could imagine some aggressive linker optimizes the resulting code by stripping instantiate_messagedef_xxx, and then stripping messagedef_xxx. Or is there any standard that precludes this scenario?

2. Even if this approach is guaranteed to work, I don't see the need for defining the static variable in the header file (which is in general not a good practice because a copy of that object will be created in all translation units that include this header file). My experiment indicates it should be sufficient if we create another .cc that has the following code:

   namespace {
   const isc::log::MessageInstantiator initiator(
       &isc::log::messagedef_cc_Tue_Feb__8_18_01_54_2011);
   }
   

3. not directly related to the main topic, but I'm concerned about the static MessageID variables defined in messagedef.h (such as MSG_DUPLNS), too. Each translation unit that includes messagedef.h will have a copy of MSG_xxx variables, and, depending on the compiler/linker optimization there may be copies of the defined strings (such as "DUPLNS"). Is there any reason that we cannot declare these MSG_xxx as extern in messagedef.h and define only a single instance of each of them in messagedef.cc? In my quick experience it seems to be possible. This way we can ensure each MSG_xxx symbol is unique within the program, and we can possibly compare these IDs by addresses, which should be much faster than comparison as strings.

Other random comments, mostly off topic in this context but I happened
to notice in reviewing the relevant part:

• I've made some trivial editorial/style fixes directly on the branch and pushed the changes.
• I don't (yet) fully understand the expected usage, but is each app (e.g. b10-auth) and (some of) BIND 10 library (e.g. libdns++) is expected to create its own message definition and register it via MessageInitializer?? If so, are these specific definitions expected to override the default MSG_xxx defined in lib/log/messagedef.h or expected to be different sets of IDs? If it's the latter, should we worry about the current default definition (DUPLNS, etc) can cause conflict?
• It seems the return value of RootLoggerName::getName() could be 'const string&', and it would be a bit (or much, depending on the std::string implementation) faster.
• same comment applies to MessageDictionary::getText()
• do we have performance consideration? Logger::xxx() methods seem to involve expensive string comparison and many object copies and may have substantial performance impact.
• why does MessageDictionary::globalDictionary() return a pointer instead of a reference? In general when we can ensure it can never be a NULL pointer (and in this case we can) it would be better to use a reference, because then we don't have to worry about one corner case where a null pointer is returned by some accident or bug.
• in logger.cc, I'd try to avoid relying on macro. At least it seems to be achieved easily for MESSAGE_SIZE (which could be a const size_t in an unnamed namespace). I see some difficulty in FORMAT_MESSAGE(), but Logger::xxx methods already have some duplicate, so I'd rather push the va_xxx stuff in each method and avoid relying on macro.
• minor editorial nit: some of the copyright years may be incorrect. Shouldn't all these be 2011?

[stephen]

As for RootLoggerName?, the original problem seems to be solved. But I now wonder whether it has to be a class.
:

It doesn't have to be a class - I've changed it to a pair of functions.

is it really guaranteed that the symbol for the MessageInitializer? object remains?
:

It worked in the tests with Ubuntu and OS/X, but I guess that you are right and that it is not guaranteed. The aim is that anyone creating a message file does not need to modify a global initialization function to pull in the symbols; just including the file in the link (or in a library that is included in the link process) is enough to make the symbols available.

Even if this approach is guaranteed to work, I don't see the need for defining the static
variable in the header file (which is in general not a good practice because a copy of that
object will be created in all translation units that include this header file). My experiment
indicates it should be sufficient if we create another .cc that has the following code:

namespace {
const isc::log::MessageInstantiator initiator(

&isc::log::messagedef_cc_Tue_Feb8_18_01_54_2011);

}

If there is no reference to "initiator" in the code, what causes the linker to pull in this file? (But see below.)

Not directly related to the main topic, but I'm concerned about the static MessageID variables
defined in messagedef.h (such as MSG_DUPLNS), too. Each translation unit that includes
messagedef.h will have a copy of MSG_xxx variables, and, depending on the compiler/linker
optimization there may be copies of the defined strings (such as "DUPLNS"). Is there any reason
that we cannot declare these MSG_xxx as extern in messagedef.h and define only a single
instance of each of them in messagedef.cc? In my quick experience it seems to be possible.
This way we can ensure each MSG_xxx symbol is unique within the program, and we can possibly
compare these IDs by addresses, which should be much faster than comparison as strings.

This was an option I suggested at the end of my comments on ticket #559. Since both you and Jelte are happy with the idea, I've modified the code to do that.

It also solves the problem of the referencing of the MessageInitializer - that can be in the same file as the definitions of the the message symbols. Since the file will be included at link time to resolve the message symbol references, the MessageInitializer object will also be included.

I've made some trivial editorial/style fixes directly on the branch and pushed the changes.

OK

I don't (yet) fully understand the expected usage, but is each app (e.g. b10-auth) and (some
of) BIND 10 library (e.g. libdns++) is expected to create its own message definition and
register it via MessageInitializer?? If so, are these specific definitions expected to override
the default MSG_xxx defined in lib/log/messagedef.h or expected to be different sets of IDs? If
it's the latter, should we worry about the current default definition (DUPLNS, etc) can cause
conflict?

We can do it either way - I was leaning more to the idea that each facility would define its own symbols in a local file. (If we include an explanation of each message in the message file - the aim being that a "system messages" manual can created for the user - a single file could get very large.)

Of course, the problem with this is that the intended usage is that each message ID (not the symbol used to reference it - these can be in different namespaces) should be unique in the program. Splitting the message IDs between files runs the risk of using the same one twice. This is difficult to detect at compile time, but not at run-time. When the MessageInitializer object loads a message file into the global dictionary, it records any duplicate IDs found. This list is output when the logger system is initialized (in the init() function in logger_support.cc). So providing the server logs are checked during tests, it should be possible to identify duplicate IDS.

(as an aside, since message declaration (.h) and definition (.cc) files are generated from the message file, I see only the message file being checked into git. If the BIND10 build procedure were to build the message compiler first, it could run through the BIND10 source tree and generate the declaration and definition files from each message file before starting the build.)

It seems the return value of RootLoggerName::getName() could be 'const string&', and it
would be a bit (or much, depending on the std::string implementation) faster.

same comment applies to MessageDictionary::getText()

Done.

do we have performance consideration? Logger::xxx() methods seem to involve expensive string
comparison and many object copies and may have substantial performance impact.

Isn't this premature optimisation? :-)

Seriously, with the backing out of log4cxx, this implementation is a quick fix to get an API in place (so we can start adding logging to the code) in time for the next release. At the F2F meeting in Prague, we need to decide if we are going to write our own logger implementation or choose some other package.

However, to address the concerns for now, I've tried to improve performance. Most of the string comparisons now only occur if options are set for a specific logger. If all loggers use the default (root) options, there should only be a few integer comparisons.

why does MessageDictionary::globalDictionary() return a pointer instead of a reference?

Historical accident, now changed.

in logger.cc, I'd try to avoid relying on macro.
:

Fair enough. I was trying to put as much as possible in the implementation-independent logger.cc. But I've followed your suggestion and removed the macros, putting more into the implementation.

minor editorial nit: some of the copyright years may be incorrect. Shouldn't all these be 2011?

Changed for files created in 2011. (And as a side-effect, I've updated my new file template.)

[jinmei]

I'm looking into the changes, but a couple of trivial nits first:

• in the new change you reverted parentheses around return, against the style guideline, e.g.

  -    return (isc::strutil::trim(current_time));
  +    return isc::strutil::trim(current_time);
  

  Please fix them.
• there are some redundant white spaces in blank lines, e.g.

  +             "\n";
  +        
  +        // Declare the message symbols themselves.
  

  git diff with colors (or colours:-) highlights these. I suggest you check the result of git diff and fix such trivial issues.

[jinmei]

Replying to stephen:

Comments about the main topic first:

namespace {
const isc::log::!MessageInstantiator initiator(
    &isc::log::messagedef_cc_Tue_Feb__8_18_01_54_2011);
}

If there is no reference to "initiator" in the code, what causes the linker to pull in this file? (But see below.)

The same question applies to your original solution, and that's
exactly why I asked the question in my previous comment. This
approach is effective only if the call to the constructor with the
reference to the mesagedef_... symbol isn't removed. I didn't
figure out how exactly this one worked. I guess it was because it
happened in a different translation unit, but I was not sure about
that, and I was not sure whether it's always guaranteed.

It also solves the problem of the referencing of the MessageInitializer - that can be in the same file as the definitions of the the message symbols. Since the file will be included at link time to resolve the message symbol references, the MessageInitializer object will also be included.

I'm still not sure if it's always guaranteed. Isn't it possible that
the linker detect we actually don't need "initializer" and removes the
corresponding code as optimization?

The rest of the comments are something I happened to notice. It's not
comprehensive review, but I didn't think I was requested for it.

logging performance

do we have performance consideration? Logger::xxx() methods seem to involve expensive string
comparison and many object copies and may have substantial performance impact.

Isn't this premature optimisation? :-)

Actually, I didn't worry about performance at this stage. I simply
wondered whether there was any consideration was given, which could be
"this version is intended to provide necessary stuff for the purpose
of year2 deliverable and doesn't care about performance yet". As long
as we understand the current (possible) limitation about performance
and have a plan to revisit it later, I'm okay with that.

root_logger_name.cc

• regarding root_name_: to be pedantic, namespace level static is deprecated in C++. I would use an unnamed namespace (but read the next bullet first). I'd also remove the trailing _ from the variable name as it's not a member variable of a class.
• the use of namespace level static object (root_name_) opens up the possibility of initialization fiasco again, even though it's much less likely to happen (consider, e.g. the case where another static object calls getRootLoggerName() in its constructor). One compromise would be to note the restriction in the documentation (such as get/setRootLoggerName() shouldn't be used inside a constructor of a class). We could also use a proxy function like:

  namespace {
  std::string&
  getRootLoggerNameInternal() {
      static std::string root_name;
      return (root_name);
  }
  }
  

  which would probably be better.
• getRootLoggerName is not documented (in the .h)

messagedef.cc

I'd make things more cost:

//extern const isc::log::MessageID MSG_DUPLNS = "DUPLNS";
extern const isc::log::MessageID const MSG_DUPLNS = "DUPLNS";
...
///isc::log::MessageInitializer initializer(values);
const isc::log::MessageInitializer initializer(values);

logger_support_test.cc

• The comment line isn't accurate: There's now no root logger declaration.

  // Declare root logger and a loggers to use an example.
  Logger logger_ex("example");
  Logger logger_dlm("dlm");
  

• also, does logger_xxx have to be in the namespace level? if not, I'd move them inside main(). it would generally be better to use narrower scope.
• the call to (isc::log::)init() seems to suggest the name is too generic, especially when we omit the namespace:

      // Update the logging parameters
      init("alpha", severity, dbglevel, localfile);
  

  While it could sound redundant, I'll be a bit more verbose, e.g. initLogger()

logger.{h,cc}

• why does Logger have virtual functions? It doesn't seem to be (intended to be/become) a base class.
• about the macro: if the concern is to have a duplicate pattern in derived classes, we could use a template method pattern (or C++ template), but that would be beyond the subject of this ticket.

LoggerImpl::output() in logger_impl.cc

• maybe a matter of taste, but I'd avoid using a temporary variable "format":

      //const string format = MessageDictionary::globalDictionary().getText(ident);
      //vsnprintf(message, sizeof(message), format.c_str(), ap);
      vsnprintf(message, sizeof(message),
                MessageDictionary::globalDictionary().getText(ident).c_str(), ap);
  

• in general, I don't like to override the end of a buffer with a nul, possibly changing the meaning of the string:

      message[sizeof(message) - 1] = '\0';    // Guarantee trailing NULL
  ...
      chr_time[sizeof(chr_time) - 1] = '\0';  // Guarantee a trailing NULL
  

  The best is to use a sufficient size of buffer so that there should be no need for overriding. At least it seems we should be able to do that for the latter case (until year 10000). And, in that sense, using a magic number of "32" doesn't seem to be good. Besides, since both vsnprintf() and strftime() should guarantee nul termination, the above code is actually redundant, too. If we worry about buggy implementation of such library functions, I'd rather use an assertion:

      message[sizeof(message) - 1] = '\0'
      vsnprintf(message, sizeof(message), format.c_str(), ap);
      assert(message[sizeof(message) - 1] == '\0');
  

[stephen]

If there is no reference to "initiator" in the code, what causes the linker to pull in
this file? (But see below.)

The same question applies to your original solution, and that's
exactly why I asked the question in my previous comment. This
approach is effective only if the call to the constructor with the
reference to the mesagedef_... symbol isn't removed. I didn't
figure out how exactly this one worked. I guess it was because it
happened in a different translation unit, but I was not sure about
that, and I was not sure whether it's always guaranteed.

In the original solution, the reference to the external symbol and the declaration of the MessageInitiator were in the header file, so were included in files that would be pulled into the link for other reasons. Although the compiler's optimiser would have probably removed the external reference had it been there alone (as there would be nothing in the module that referred to it), passing it as an argument to the MessageInitiator - which the optimiser could not remove as the declaration will cause the running of its constructor which is in another module - should mean that it would remain. So the module containing the #include should contain a reference to the initialization object, which should cause it to be pulled in.

It also solves the problem of the referencing of the MessageInitializer - that can be in the
same file as the definitions of the the message symbols. Since the file will be included at
link time to resolve the message symbol references, the MessageInitializer object will also
be included.

I'm still not sure if it's always guaranteed. Isn't it possible that
the linker detect we actually don't need "initializer" and removes the
corresponding code as optimization?

I think it's unlikely under Unix, but possible under Windows Visual Studio. The latter contains a "Whole program optimization" switch that defers the optimization to link time. However I would have thought that the optimization would detect that MessageInstantiator refers to MessageInitializer whose constructor invokes MessageDictionary which is referred to by other modules in the program. It will be interesting to find out when we finally port BIND10 to Windows - although I suspect that that will be one of our lesser problems :-)

root_logger_name.cc

• regarding root_name_: to be pedantic, namespace level static is deprecated in C++. I would

use an unnamed namespace (but read the next bullet first). I'd also remove the trailing _ from
the variable name as it's not a member variable of a class.

• the use of namespace level static object (root_name_) opens up the possibility of

initialization fiasco again, even though it's much less likely to happen (consider, e.g. the
case where another static object calls getRootLoggerName() in its constructor). One compromise
would be to note the restriction in the documentation (such as get/setRootLoggerName()
shouldn't be used inside a constructor of a class). We could also use a proxy function like:

A case of "more haste, less speed" when I made the changes. Both points are entirely correct, and I've altered the code to use an anonymous namespace and a proxy function (and have updated the documentation).

I'd make things more cost:

extern const isc::log::MessageID MSG_DUPLNS = "DUPLNS";
extern const isc::log::MessageID const MSG_DUPLNS = "DUPLNS";

Not needed. MessageID is typedef'ed to "const char*".

const isc::log::MessageInitializer initializer(values);

Good catch, done.

The comment line isn't accurate: There's now no root logger declaration.

Corrected.

also, does logger_xxx have to be in the namespace level? if not, I'd move them inside main().
it would generally be better to use narrower scope.

In logger_support_test.cc I've moved one of them inside main(). This will be another check that the logger works at both function and namespace level.

the call to (isc::log::)init() seems to suggest the name is too generic, especially when we
omit the namespace:
:

While it could sound redundant, I'll be a bit more verbose, e.g. initLogger()

Changed.

why does Logger have virtual functions? It doesn't seem to be (intended to be/become) a base
class.

For flexibility, e.g. someone may want to intercept or modify logger messages (or a subset of them) for some reason or another, which they can do by using a derived class. When we come to doing a programme of comprehensive performance improvement, we can remove the virtual keyword if there are no derived classes.

maybe a matter of taste, but I'd avoid using a temporary variable "format":

const string format = MessageDictionary::globalDictionary().getText(ident);
vsnprintf(message, sizeof(message), format.c_str(), ap);
vsnprintf(message, sizeof(message),

MessageDictionary::globalDictionary().getText(ident).c_str(), ap);

My preference is to keep it as it does perform small bit of of documentation and prevents the function call from being excessively long.

in general, I don't like to override the end of a buffer with a nul, possibly changing the
meaning of the string:

message[sizeof(message) - 1] = '\0'; Guarantee trailing NULL

...

chr_time[sizeof(chr_time) - 1] = '\0'; Guarantee a trailing NULL

The best is to use a sufficient size of buffer so that there should be no need for overriding.
At least it seems we should be able to do that for the latter case (until year 10000). And, in
that sense, using a magic number of "32" doesn't seem to be good. Besides, since both vsnprint
f() and strftime() should guarantee nul termination, the above code is actually redundant, too
If we worry about buggy implementation of such library functions, I'd rather use an assertion:

message[sizeof(message) - 1] = '\0'
vsnprintf(message, sizeof(message), format.c_str(), ap);
assert(message[sizeof(message) - 1] == '\0');

For the format supplied, 32 is the next power of two above the 20 characters needed for the string. The trailing nulls are, I'm afraid, a hangover of long years of "C" programming with "strncpy" which doesn't guarantee a trailing null - I got into the habit of terminating strings with a null, "just in case".

I've removed the addition of the training nulls.

[jinmei]

Replying to stephen:

I'm still not sure if it's always guaranteed. Isn't it possible that
the linker detect we actually don't need "initializer" and removes the
corresponding code as optimization?

I think it's unlikely under Unix, but possible under Windows Visual Studio. The latter contains a "Whole program optimization" switch that defers the optimization to link time. However I would have thought that the optimization would detect that MessageInstantiator refers to MessageInitializer whose constructor invokes MessageDictionary which is referred to by other modules in the program. It will be interesting to find out when we finally port BIND10 to Windows - although I suspect that that will be one of our lesser problems :-)

Okay. Please leave some notes about this point somewhere (maybe in compiler/message.cc)?

Oh, and by looking at message.cc I noticed the doxygen style comment mentions a non existent option:

/// If \c -p is specified, the C++ files are not written; instead a Python file
/// of the same name (but with the file extension .py) is written.

I've also made a couple of minor editorial fixes and pushed them to
the central repo.

Other points look okay, and I don't think we need further review.
Please make any suggested changes above with your discretion and merge
it.

[jinmei]

Forgot to say one last thing (I noted this in my first review but have
forgotten since then):

I'd suggest a cleaner update to configure.ac than commenting out a
large block:

# Configure log4cxx header and library path
#
# If explicitly specified, use it.
# AC_ARG_WITH([log4cxx],
#   AC_HELP_STRING([--with-log4cxx=PATH],
...
# fi
# AC_SUBST(LOG4CXX_LDFLAGS)

[stephen]

Added information to message.cc.

Re configure.ac, it uses the fix Jeremy put in to temporarily work around the log4cxx problems (so it still accepts "--with-log4cxx", but also accepts "--with-log4cxx=no". So although log4cxx can be specified, it has no effect. (This should avoid the need to alter test scripts for the moment.) We can remove the log4cxx stuff if necessary pending the outcome of discussions in the Prague meeting.

[stephen]

Forgot to say, change applied to master as commit 57736fd520bda626de430e55d9f7f0b1c13e6353