Bugzilla – Full Text Bug Listing
| Summary: | commons-logging fails to discover available logging systems | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] Orbit | Reporter: | Ales Dolecek <ales_d> | ||||
| Component: | bundles | Assignee: | Gunnar Wagenknecht <gunnar> | ||||
| Status: | RESOLVED DUPLICATE | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | ales.dolecek, david_williams, gunnar, simon_kaegi, steffen.pingel | ||||
| Version: | unspecified | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Ales Dolecek
BTW: com.springsource.org.apache.commons.logging uses same machanism but also optionally depends on org.apache.log javax.servlet I don't know why the above two are included. The negative side-effect of javax.servlet dependency is that it will eventually pull-in many otherwise unused packages if you use PDE features to auto-compute required dependencies with "Include optional" enabled. Chris, I'm assigning to you, since you are listed as the contact. What do you think? The fix is trivial. Seems that Chris had abandoned the plugin. What can I do to help to solve this? Ales Is it really so difficult to add one or two optional packages to MANIFEST?
$ diff bug/META-INF/MANIFEST.MF fix/META-INF/MANIFEST.MF
10a11,13
> Import-Package: org.apache.avalon.framework.logger;version="[4.1.3, 4.
> 1.3]";resolution:=optional,org.apache.log4j;version="[1.2.12, 2.0.0)"
> ;resolution:=optional
Ales
(In reply to comment #4) > Is it really so difficult to add one or two optional packages to MANIFEST? > > $ diff bug/META-INF/MANIFEST.MF fix/META-INF/MANIFEST.MF > 10a11,13 > > Import-Package: org.apache.avalon.framework.logger;version="[4.1.3, 4. > > 1.3]";resolution:=optional,org.apache.log4j;version="[1.2.12, 2.0.0)" > > ;resolution:=optional > > Ales Sort of. Not mechanically, of course. But, conceptually. Why is it these should be added? Are you saying, essentially, that any implementation (even new ones, in future) would have to be added? I do not see org.apache.avalon in Orbit. Where's that coming from? Some other Eclipse project packages it? Ordinarily, we would not list things as required (even optionally) if they are not available. So ... some detailed explanation would be required. Gunnar, I think of you as a "logging expert" ... perhaps you can comment on the merits of this suggested change? OK then import only log4j. The commons-logging is just thin wrapper which delegates to other logging systems. But under OSGi it does not see any except java.util.logging which is part of Java since 1.4 Thus without the import it can't use Log4j even if other bundles use it. If some of the bundles you use depend on Log4j and others that depend on commons-logging you end up with two different logging systems. If you allow commons-logging to *see* log4j it will use it. So any bundle that uses commons-logging will actually use log4j. The point of commons-logging is that you can swap logging systems at will and the library will shadow you from being dependent on them - at the price of being dependent on commons-logging. This is usfull in heterogenous world where every library use different logging system. Without the imports the the commons logging is useless. Aavlon is other logging system - as log4j or java.util.logging. I havent seen any project using it recently. Ales Adding the optional imports makes sense. Ales, can you prepare a patch? I was just reading, and I think the same thing was discussed in bug 181813 in comment 1 and there explained why adding log4j to manifest (of the one, official Orbit distributed bundle) was not a good idea, and explained some possible ways to approach the solution, and it was closed as "won't fix". Let me know if I have misread bug 181813 but if not, I suggest we close this one as a "dup", which implies "won't fix" and hopefully one of those other solutions would work for you? Is this bug not the same as bug 181813? No they done it wrong way. Since it depended on on plugin. Generally not only log4j plugin can provide log4j. The http://www.slf4j.org/ or http://logback.qos.ch/ also provide the Log4j API. By importing only *package* org.apache.log4j (and none of its subpackages) the commons logging will use any bundle that provides Log4j API. And by making the import *optional* it can exist even without log4j - if for example someone wants to use java.util.logging as backend of commons-logging. I asure you, that optional import of package org.apache.log4j is correct. --- The patch is is already included in text of my yesterdays comment. If you do not want to include avalon LogKit because Orbit does not provide it then it would be only: $ diff bug/META-INF/MANIFEST.MF fix/META-INF/MANIFEST.MF 10a11,12 > Import-Package: org.apache.log4j;version="[1.2.12, 2.0.0)";resolution: =optional As I explained I can not provide completely new MANIFEST.MF since it contains digital signatures. It has to be changed somewhere in "source" from which you generate the bundle JAR. Ales BTW: Have a look at commons-logging web. Even the authors state that commons-logging optionally depend on avalon-framework 4.1.3 log4j 1.2.12 logkit 1.0.1 http://commons.apache.org/logging/commons-logging-1.1.1/dependencies.html Ales (In reply to comment #11) > BTW: Have a look at commons-logging web. Even the authors state that > commons-logging optionally depend on > > avalon-framework 4.1.3 > log4j 1.2.12 > logkit 1.0.1 > > http://commons.apache.org/logging/commons-logging-1.1.1/dependencies.html > Thanks, Ales, that looks like a good reference, though I am personally a little confused, since it it also says javax.servlet is required. Plus, it makes it sound like log4j (and avalon and logkit) are not "optional" but literally required. I have a feeling that commons-logging was not really designed for a "participatory" architecture like Eclipse plugins. In other words, I have a feeling adding log4j might make your case work, but if someone installs something else, then their logging solution might not work as expected? It has been the way its been for a while, and I can't help but wonder if the others associated with bug 181813 (originator of it, or its dups) have found a solution more along the lines of those suggested in bug 181813? Ales, do you have an "RCP" app that you are trying to use this for? Or a feature that you want to plug in to anyone's Eclipse IDE? Thanks, Yes I have applications where I run into this issue. I use log4j, but some bundles use commons-logging. In my current project for example org.eclipse.emf.teneo and org.eclipse.emf.teneo.mapper while org.hibernate is using log4j. Since commons-logging is able to delegate to log4j I use log4j. With Orbit's commons-logging would Teneo use java.util.logging (since it is the only logging framework your bundle "sees") and while Hibernate woudl use Log4j. If you add optiona import of log4j both Teneo and Hibernate would use Log4j. I have used various workaround over the time: 1) Before I got better understanding of OSGi I used com.springsource.org.apache.commons.logging which does the imports right - it simply "just works (TM)" - you do not have to know why - the springsource bundle however has dependency on the javax.servlet and when auto-computing dependencies (with optional enabled) it drags in many bundles that I actually do not need 2) When I found out where the problems I packaged my own commons-logging bundle 3) Now I use slf4j bridge (see http://www.slf4j.org/legacy.html) - SFL4J provides both commons-logging and log4j API so the bundles that just import the packages org.apache.commons.logging and/or org.apache.log4j are satisfied by SLF4J without actually using their code. --- All workarounds have one in common. They do NOT use commons-logging from Orbit. The only reason why I have commons-logging bundle in my Eclipse is that some plugins (I suppose from Eclipse and maybe directly from Orbit) depend on it. Not on via import of package but directly on the plugin. The only downside of this is that Eclipse prefers Orbit plugin over all other and when computing dependencies they choose it. I belive it is because it does not depend on anything which makes it better choice that the others which have more dependencies (springsource or my own bundle) or are fragments (SLF4J). --- The point is that if you are so scared that you stick to bad bundle for compatibility reasons then just clos this ticket. I have read other solutions proposed (like using fragments or buddy bundles) and all miss one point. If you use commons-logging outside of OSGi and have more logging framewors on classpath then the commons-logging will also choose only one. So the wrong choice is not OSGi specific, but is generic problem of commons-logging. And is documented and there is a way how to override auto-discovery of commons logging. (http://commons.apache.org/logging/guide.html#Configuration) But declining to import log4j because someone thinks taht java.util.logging is "better" because it is in every JVM 1.4 does not make sense. Most people actually do not care and choose according what other libraries they use have chosen. Which is BTW reason why commons-logging exist. Given the above reasoning I could argue that java.util.logging is wrong because every OSGi Framework has LogService. --- This is my attempt to convice you. I'm surprised how hard it is to "push" this issue forward. (In reply to comment #13) > > ... I'm surprised how hard it is to "push" this > issue forward. I think the difficulty is simple to explain, a) it's been proposed before, and was rejected, b) its been the way its been for many releases, c) We are concerned about "changing behavior" ... if someone was using it and using "simple logger" or something and that was what they desired, then there's some risk when they upgrade, then the behavior will change (if there is also a log4j logger that happens to be there in the stack). Right? And they will have to do something special to restore their desired behavior. Right? That said, none of those issues rules it out ... not to mention Gunnar said it was reasonable :) ... just takes lots of discussion. So, Ales, can you speak to this risk of changing behavior? Can you describe under what conditions behavior would change? That is, what kind of combination of bundles would work one way before, but different after this fix? Can you also address more specifically what those users would have to do to restore previous behavior? I know you already gave the reference link to how to control the logger, but it speaks about a properties file, commons-logging.properties, and in my experience, in an OSGi environment, "adding a properties file to the classpath" is not so easy ... and as far as I know, would required users creating, adding, installing a fragment or something (which, was already one of the work arounds suggested, so, we'd just be changing the burden of who has to add the fragment)? Don't give up, if you really think this is the right thing to do, but seems to me, it still will not be quite right (since doesn't include avalon, or servlet) so, it is unclear to me if a "partial solution" that helps some people, but not all users of commons logging, is worth the risk that other people might have to change things to keep their previous behavior. Put another way, it it there now, as it is, because some bundles (such as soap or something) require it be there on the classpath, but, the way it is now, is not really intended to be used for logging (expect for simplest fall-back case). Which you are trying to do. This is complicated by the fact we don't have an Orbit Committer that has "stepped up" to owning this bundle and issue ... it shouldn't be me, since I know so little about loggers. Chris, as current "contact", any suggestions? Gunnar, interested in "taking it over" as the responsible committer? Ales, thanks for your persistence ... and patience! (In reply to comment #14) > Gunnar, interested in "taking it over" as the responsible committer? Yes, I can take over the bundle as it fits into the logging space. If Ales is an Eclipse committer we can also nominate him to contribute to the bundle in the future. As for what can possibly go wrong ... we have seen various bugs with logging systems in the past. I think that there is a likely chance that suddenly users will start to see a lot log output produced by LOG4J in the console. We had that before. But we could handle/fix this very well. It just needs to be discovered. Thus, bringing it in for the next milestones actually helps. Ales, are you still interested in this change? Hello, I'm willing to push this issue further - ideally to fully resolving it. Have a look at http://commons.apache.org/logging/guide.html#Configuration as it gives short info about how the CL is configured. 1) Change of behavior: * if there is bundle which exports org.apache.log4j package then Log4j will be used * so far java.util.logging was used in such case Personally I wonder why anyone would use 2 different logging frameworks in same project, but agree that it is change in current behavior. If no bundle exports org.apache.log4j then there is no change of behavior. 2) Overriding the setting The link above explains the configuration of CL. From the text it seems that only Log4j, java.util.logging and CL SimpleLog implementations are considered. Same states API doc at http://commons.apache.org/logging/commons-logging-1.1.1/apidocs/index.html. The source however includes AvalonLogger and LogKitLogger => I'll checkout the code and find out which are really used by CL 1.1.1 --- As for override there are three possibilities within OSGi: a) use of System property org.apache.commons.logging.Log - which means adding option -Dorg.apache.commons.logging.Log=org.apache.commons.logging.impl.Jdk14Logger when launching Equinox (or other OSGi framework) b) use property file with key-value pair org.apache.commons.logging.Log=org.apache.commons.logging.impl.Jdk14Logger c) explicitly set the factory by code --- Personally I would fix the problem in following way. * add the optional import of Log4j as suggested * create fragment bundle that would override the defaults back to Jdk14Logger Thsi means change of behavior whic can be reverted by either adding System property when starting the OSGi framework or by including the fragment bundle. Alternative way to fix the problem is to patch the CL code to use different order of Log implementetion lookup trying JUL before Log4j. This would mean that you get the old behavior and have to set System property to get the Log4j. Might be bit confusing to CL users however as it works different from the original code. --- I'll get back with more comments once I inspect the auto-discovery implementation of CL. Ales Direct link for the API dos is: http://commons.apache.org/logging/commons-logging-1.1.1/apidocs/org/apache/commons/logging/LogFactory.html#getLog%28java.lang.String%29 The URL I have given is for top level frame of javadoc. Thank you, Gunnar. Created attachment 209604 [details]
Fragment for commons-logging allowing it to use Log4j
How blind am I? Simplest solution of all. Create fragment to org.apache.commons.logging that will import org.apache.log4j.
Current Orbit package does not need to be changed. If you include the fragment bundle it will use Log4j if not then nothing changes.
Included is the fragment bundle and simple test bundle that prove it. If you include it the output on console will be:
log4j:WARN No appenders could be found for logger (org.apache.commons.logging.test.Activator).
log4j:WARN Please initialize the log4j system properly.
If you ommit it the output will be:
17.1.2012 9:19:21 org.apache.commons.logging.test.Activator start
INFO: Wonder what logging framework is processing this
Ales
Excellent! Why don't we mark this as a dup of bug 181813 then (which is marked as "won't fix". In one of it's already dupped bugs, bug 186489 comment 1 it was suggested <quote> In a product the situation is a little bit different in that they really might want to control all logging. For those cases the product could add a fragment to the commons logging bundle and wire it to log4j or some other logger type. </quote> So, by "excellent", I mean "excellent you have proven or confirmed this worked!" Thanks so much. Gunnar, you are still welcome to "take over" the bundle by updating contact in ip log. Ales, great contribution. Thanks again. *** This bug has been marked as a duplicate of bug 181813 *** Hello, before closing this bug you should create the fragment in Orbit and possibly include it with commons-logging bundle. Otherwise other users unfamiliar with the issue might not get it work. Ales (In reply to comment #21) > Hello, > > before closing this bug you should create the fragment in Orbit and possibly > include it with commons-logging bundle. Otherwise other users unfamiliar with > the issue might not get it work. > > Ales I'll let Gunner decide if that's reasonable and wise, but will comment that providing the fragment as an Orbit deliverable would require more care, at least in documentation, for the same reasons of "changing behavior". For example, if someone (let's say "web tools") decided to include the fragment, then that would "change behavior" for everyone using or extending webtools. Not desirable. I think the value of using the fragment is for those providing a "stand alone" RCP app or product where they are sure they are the sole responsible party for deciding what/how logging is done. And, for that, I think your attached example would suffice. And, I would think any such use might require custom tweaking, in setting properties or similar? So, little that I know, I am content to leave as "an example" and not make it an official Orbit deliverable. [As always, I could be completely misunderstanding ... but, will let Gunnar take it from here.] Thanks again, Ales. Your comments, example, and persistence have been very helpful. (In reply to comment #21) > before closing this bug you should create the fragment in Orbit and possibly > include it with commons-logging bundle. Otherwise other users unfamiliar with > the issue might not get it work. I think it's ok to commit and provide the fragment for documentation purposes. However, we need to make sure that it doesn't get pulled into any release train package by default. It's ok (IMHO) if a feature installs it explicitly. (In reply to comment #23) > (In reply to comment #21) > > before closing this bug you should create the fragment in Orbit and possibly > > include it with commons-logging bundle. Otherwise other users unfamiliar with > > the issue might not get it work. > > I think it's ok to commit and provide the fragment for documentation purposes. > However, we need to make sure that it doesn't get pulled into any release train > package by default. It's ok (IMHO) if a feature installs it explicitly. Thanks Gunnar. Perhaps a "cloned bug" is best way to document the fragment specific work. But can you clarify for me ... > However, we need to make sure that it doesn't get pulled into any release train > package by default. It's ok (IMHO) if a feature installs it explicitly. These two sentences seem contradictory. I'm guessing you mean "pulled into any release train package by default" means something like "pulled into because p2 treats optional dependencies as greedy"? I'm not sure how that works for fragments. We could maybe fix that by p2.inf file, to be sure always handled correctly regardless ... but, FYI, I hope by Juno M5 to have our "p2 publisher" upgraded so greedy=false is the default, in case that makes a difference to you. But still ... if a feature specifies (includes) it explicitly ... I sure hope they do it with the blessing of everyone who might use that feature! (Unless, as often the case, I am misunderstanding). (In reply to comment #24) > These two sentences seem contradictory. I'm guessing you mean "pulled into any > release train package by default" means something like "pulled into because p2 > treats optional dependencies as greedy"? I'm not sure how that works for > fragments. Exactly. It has to be verified that publishing the fragment in the Orbit repo has no effect given the current setup, i.e. someone has to include the fragment on purpose in order to get it. > But still ... if a feature specifies (includes) it explicitly ... I sure hope > they do it with the blessing of everyone who might use that feature! That's exactly what I was thinking as well. If someone includes the fragment and contributes to a packages then they need to verify that it doesn't change things in an unexpected way. |