Have more robust log4net loading

[fredericDelaporte]

In some circumstances, it appears the log4net loading mechanism may fail with null reference exceptions, which does not help understanding what is the trouble. (See #1836.)

Moreover, log4net loading is triggered after some probing logic which is in no way involved in how it is actually loaded, creating a possibility the probing considers it loadable while it is not.

Fixes #1836

[fredericDelaporte]

The method calls following the get type are using the type without checking if it was found, so better not allow GetType to yield null.

[hazzik]

Stupid mobile version makes all comments pending requiring a "submit"

[hazzik]

Wouldn't it be enough to just Assembly.Load("log4net")? I don't have a windows machine handy to check the scenario with log4net being in the GAC (my laptop has died of old age).

[fredericDelaporte]

The method is not even documented for usage with short names. With the GAC tendency to reject short names unless they are already loaded in the application domain, I would not rely on this.

Since the code from ReflectionHelper is the one used for loading reflection based drivers, I think it is quite proven it works well in many situation, and so I would rather avoid deviating from it /simplifying it.

[hazzik]

With the GAC tendency to reject short names unless they are already loaded in the application domain

But this is anyway what happens here. Isn't it?

[fredericDelaporte]

It happens with the Type.GetType call, but then the ReflectHelper logic fallback on searching among already loaded assemblies any loosely matching one for loading the type from it, and I guess that is the part doing the trick for the case reported here. The last fallback, attempting to explicitly load the assembly with a short name, is likely to be useless in the case reported here. But maybe it still helps in some other cases.
Once again here, I am a bit conservative, preferring to stick to code which works since years without known complains, rather than trying to simplify it without being sure it won't harm any case.

[hazzik]

code which works since years without known complains

The "logic fallback on searching among already loaded assemblies any loosely matching one for loading the type from it" has been added a few month back in 5.2

[fredericDelaporte]

By yourself, granted, but the Assembly.Load logic was already there, and that is the one we are currently discussing, aren't we? There is no comment to tell in which case it may help, but does it harms to keep it? Less code is always better, provided we are sure removing it will not break some cases. There, I am not sure.

[wensaint]

@fredericDelaporte could you send a fixed release version, i failed to build the dll.
thanks,
my email is wensaint@qq.com

[fredericDelaporte]

Expands the checks of this PR, find the "NHibernate (Release Package)" one and click on "Details". Teamcity may ask you for a login but an anonymous option is normally available. Then browse to the artifacts tab of the build and take what you need.

But the current release build was not having artifacts, I do not know why. I have relaunched it, wait for it to complete if it is not finished yet.

[fredericDelaporte]

There was some discussion in this PR about whether the loading code could be simplified or not. I was, and still am, for keeping this code as near as possible to the loading logic of ReflectHelper (which we cannot use directly due to otherwise creating a circular reference trouble), even if some of it is maybe not useful in all circumstances.
There are just too many possible cases (in GAC or not in GAC, netfx or netcoreapp or mono, directly referenced by the application or just sitting somewhere in a loadable path or in the GAC, strong signed application or not, running in special hosts like Sharepoint, ...) for being sure to have checked them all.

So,

As proposed on the development group, and since no continued grounded disagreement has been expressed there since one week after raised concerns being addressed, I intend to merge this PR next week (likely on Wednesday).

(I am issuing this notice on PR issued before starting to apply this handling of merges. I will not do it for new PR.)

[hazzik]

As I still disagree with this change - does it count as "continuous disagreement"?

[fredericDelaporte]

Of course, but I would like to better understand what cause you to still disagree. I believe to have explained why this change was the way it was, where you have expressed concerns. May you tell what is wrong with those explanations?

[hazzik]

Approved by now just to unblock 5.2. However, I still think that the code needs to be simplified and not related on the code borrowed from ReflectHelper. Also, exception flow logic has to be removed.

[fredericDelaporte]

In fact changes are to be done.