Regarding NH-2821 - better way of finding log4net

[Oskar Berggren]

A pull request was submitted last year that delegates finding log4net to .Net.

https://github.com/nhibernate/nhibernate-core/pull/15

It was not merged at the time because there was some compatibility
concerns since if log4net was installed in the GAC, NHibernate would
now suddenly find this. Someone mentioned delaying it until the next
major version.

Since we are now aiming for 3.3, perhaps we should try to make a
decision on this.

Since we are only trying to load log4net if no other logging framework
has been configured, I think the proposed change would be ok. Also,
before the introduction of the logging abstraction, log4net would
always be loaded even when not configured.

/Oskar

[Fabio Maulo]

If no logging system is configured and there isn't log4net in the deploy folder the NoLoggingLogger is used..

In practice log4net should always not to be required

--
Fabio Maulo

[Richard Brown (gmail)]

I think it still won’t be required, it’s just that it will now be located if it’s in the GAC?

 

Sounds ok to me as long as it still reverts to the NoLoggingLogger when log4net isn’t in the deploy folder or the GAC.

[Stephen Bohlen]

So under this proposed change if Log4Net was in the GAC but you didn't want any logging you would have to explicitly configure NH for the NoLoggingLogger, is that right?

Steve Bohlen
sbo...@gmail.com
http://blog.unhandled-exceptions.com
http://twitter.com/sbohlen

[Richard Brown (gmail)]

That’s my understanding.

 

Also (as mentioned in the JIRA comments), unless you actually had a log4net config section defining appenders, your not going to get any log messages you weren’t expecting (I don’t think).

[Stephen Bohlen]

If that's true then I think this is probably fine to proceed with.  The only negative I could see in this scenario is that someone would be 'wasting' time and memory resolving and loading log4net.dll when they don't want/need it.  This seems a micro-optimization and so long as anyone who *really* cares can configure this problem away by explicitly selecting the NoLoggingLogger, it seems fine to me.  If we do make this change, then I'd also recommend that we make it obvious what's going on (in release notes and/or documentation) so that nobody is surprised by the (possibly) new behavior of their app.

Steve Bohlen
sbo...@gmail.com
http://blog.unhandled-exceptions.com
http://twitter.com/sbohlen

[Oskar Berggren]

2012/1/24 Stephen Bohlen <sbo...@gmail.com>:

> So under this proposed change if Log4Net was in the GAC but you didn't want
> any logging you would have to explicitly configure NH for the
> NoLoggingLogger, is that right?
>

Yes.

To paraphrase Fabio, the new behavior will be:

"If no logging system is configured and there isn't log4net in the

deploy folder OR IN THE GAC the NoLoggingLogger is used.."

If we want, I guess we could make even log4net require explicit
activation using the "nhibernate-logger" appsetting. Then you would
never get any logging unless you provide a "nhibernate-logger"
setting.

/Oskar

[Fabio Maulo]

IMO

If a guy need a logging system for NH he should configure it.

No configuration = no logging.

--
Fabio Maulo

[Stephen Bohlen]

I tend to agree.  Then this is this the most explicit way to achieve that --?

If we want, I guess we could make even log4net require explicit
activation using the "nhibernate-logger" appsetting. Then you would
never get any logging unless you provide a "nhibernate-logger"
setting.

Steve Bohlen
sbo...@gmail.com
http://blog.unhandled-exceptions.com
http://twitter.com/sbohlen

[Julian Maughan]

I believe the pull request will cause NH to behave as follows: If no logger is configured and the user happens to have log4net in their GAC, log4net will be used. As no config is provided, nothing will actually be logged anywhere, but personally I think this invites trouble - and goes against the principle of least surprise.

It wouldn't take much to change this behaviour to be as Fabio describes, no logger configured == no logging (i.e. use the NoLoggingLoggerFactory). 

[Stephen Bohlen]

IMO that's the option that's most in-line w the principle-of-least-surprise, so I think that's more the way to proceed.  "Surprise -- you've got logging!" isn't something that makes any sense to me :)

Steve Bohlen
sbo...@gmail.com
http://blog.unhandled-exceptions.com
http://twitter.com/sbohlen

[Oskar Berggren]

So to summarize, is this what we agree on?

nhibernate-logging missing or empty
Use Nologging. Do not attempt to load log4net, even if present in
the deploy directory.

nhibernate-logging set to full name and assembly of a class.
Attempt to use that with System.Type.GetType("") and
Activator.CreateInstance() which will find it in either GAC or deploy
directory.

nhibernate-logging equal to "log4net"
Attempt to load log4net using Assembly.Load("log4net"), which will
find it in either the GAC or deploy directory.
This is to simplify and reduce impact of breaking change for those
who just want to keep using log4net.

/Oskar

2012/1/24 Stephen Bohlen <sbo...@gmail.com>:

[Julian Maughan]

Unfortunately if we go down the "no log configuration == no logger" path, existing users who are already using log4net and upgrade to 3.3 will have to add an 'nhibernate-logger' entry to their <app.config /> that wasn't previously required.

I don't think it is reasonable for NHibernate, without any explicit configuration, to find and use a reference to log4net that may have been added to the GAC for use by an entirely different application. Particularly when the user may also not have configured log4net in their application. I think users prefer the simple xcopy deployment model where the application and its dependencies are contained in one folder.

I'm voting to reject the pull request, avoid the GAC, and maintain the status quo.

As an aside, I'm curious why the 'nhibernate-logger' setting is in <app.config /> and not <hibernate-configuration />...?

[Fabio Maulo]

A "simple" note to NH's ref ?

I mean an actualization of this section

http://nhforge.org/doc/nh/en/index.html#configuration-logging 

Perhaps we can even include a a link to this wiki  http://nhforge.org/wikis/howtonh/using-nlog-via-common-logging-with-nhibernate.aspx for additionals logging systems.

--
Fabio Maulo

[Fabio Maulo]

Answer to the question:

Probably because even the upload of the hibernate-configuration is logged and the log-conf have to be analyzed before start any kind of action (the famous "egg and chiken" stuff) 

--
Fabio Maulo

[Patrick Earl]

I also agree that we shouldn't be automatically loading assemblies just because they're in the GAC.

        Patrick Earl

[Oskar Berggren]

2012/1/25 Patrick Earl <hyn...@gmail.com>:

> I also agree that we shouldn't be automatically loading assemblies just
> because they're in the GAC.
>
>         Patrick Earl

Before the introduction of the loggin abstraction when log4net was
required, it would have been loaded from the GAC if available right?
When the change was made it would then mean that it was no longer
enough to have it in GAC, i.e. a breaking change (which I don't seem
to find in the release notes).

If we don't want to use Assembly.Load(), other alternatives:
1) Find the path of NHibernate.dll and expect log4net.dll to be in the
same folder. Though NHibernate.dll may be in the GAC.
2) Fix the code to properly handle semicolons in RelativeSearchPath,
which can occur according to the documentation.

/Oskar

[Rafał Kłys]

Hi, it was my pull request. I understand issues with Assembly.Load, and I have another idea. Check https://github.com/nhibernate/nhibernate-core/pull/73. 

This time it checks if log4net is already loaded in current AppDomain (using AppDomain.CurrentDomain.GetAssemblies()). If someone uses log4net, it will be loaded and it doesn't matter now if NH or log4net is in GAC or not. Only problem with this is if someone first initializes NH, and log4net afterwards, but it's a bug IMO: logging should be initialized first always.

[Oskar Berggren]

2012/2/8 Rafał Kłys <rafa...@gmail.com>:

Hi Rafał,

Thanks for working on this. However, I see some issues with this
patch. I agree with you that logging, if used, should be initialized
first, but I also fear that this behaviour could lead to strange and
difficult to debug problems, where it may not be obvious why logging
doesn't work.

In fact, when I think about it I have to wonder if this special case
automatic-but-only-if-in-a-specific-directory loading of log4net is
really necessary. Perhaps we should rip that out as a breaking change
in the upcoming 3.3 and *require* even log4net users to set the
nhibernate-logging application setting. It's certainly not difficult.

To be clear: My idea is to *keep* the log4net logger factory to make
log4net use easy, but *require* setting nhibernate-logging="log4net"
to turn it on. In this case, we wouldn't care where the assembly is
located, as long as the runtime can find it.

/Oskar

[Fabio Maulo]

That behavior is there because 95% of users does not know the pretty new feature (no specific logging-framework dependency) and they are using log4net or NH-Prof.

If you change the behavior please mail Ayende because he may have to find a solution for NH-Prof.

--
Fabio Maulo

[Rafał Kłys]

Yes, now it is quite strange and difficult to debug why logging work without any configuration inside running application, but not inside NUnit:)

I would say that this special case is not necessarily needed, but is a great convenience. Also from my point of view, even that I know that there is <appSettings> key for that and also LoggerProvider.SetLoggersFactory (which is even better in my case, because I don't even need to have app.config for my unit tests).

From the other side, my pull request makes another change in behavior, now not the log4net.dll triggers NH logging, but calling any log4net method before any NH one. Still, IMO, this behavior is better.

On Wednesday, February 8, 2012 8:02:10 PM UTC+1, Oskar Berggren wrote:

2012/2/8 Rafał Kłys <rafalklys at gmail.com>: