Team, Attaching the patch that will convert FOP to using Commons Logging [1]. I've tested several sample files with it, it appears to run fine throughout, but as the changes are extensive, I suspect there will be a few places where a logger was not properly set, etc. Let me know what you see, but I suspect many small bugs we can fix after the patch is applied. Only issue found was in setting a logger for an Avalon "ContainerUtil", which currently won't work with Commons Logging and needs its own Avalon logger. I just commented out the line (in two places) where that ContainerUtil logging would otherwise have been enabled (I don't think we're currently using this functionality anyway.) In the future, we can look at other options here--such as just feeding it an Avalon console logger, to providing a file conversion between the two logging choices, to just not logging here, to using an alternative to ContainerUtil. I'll hold off on this for three days to allow for team comments. (Please minimize your changes to the HEAD code base in the interim, as updating this patch would be very complex.) Thanks, Glen [1] http://marc.theaimsgroup.com/?l=fop-dev&m=108009551327746&w=2
Created attachment 11047 [details] patch file, avalon logging->commons logging conversion
Hi Glen I'm a bit surprised at the way you did the change. Like this I don't see any benefit over Avalon-style logging. You still have to pass a lot of loggers to several objects. This simply trades a 70KB JAR with a 30KB JAR for the same thing while Avalon still provides some additional features. I'd have expected you to make use of the "static" way to obtain a logger (as seen in Commons Digester for example): protected Log log = LogFactory.getLog("org.apache.commons.digester.Digester"); The logger is then configured externally. This makes life a lot easier for the FO tree (if you use a static variable) and for the PDF library, for example. See also: http://jakarta.apache.org/commons/logging/guide.html
Glen, the Avalon logger for the containerUtil that you mention. I think this is the logger for Batik in the Transcoder. If we dont set it, does this mean there is no way of seeing warnings from Batik processing? Have a look at PDFTranscoder.transcode method to see what I mean.
[Jeremias] Yes, this work definitely needs optimization--what you mention is a major area (Axis does exactly as you say over and over again.)--I just did a straight conversion first (several hours of work). I'm not a logging expert--I just need a foothold for the conversion that can *then* be optimized. If right now as you say, we're just the same as Avalon, I'm quite thrilled that I could even manage that. The changes you are suggesting can happen next. BTW, would you already know how to make these changes quickly, after I commit the patch? If you can zoom through this in 20-30 minutes, that would save me a lot of time. Else I'll research it. [Chris] <Q> I think this is the logger for Batik in the Transcoder. If we dont set it, does this mean there is no way of seeing warnings from Batik processing? </Q> Well, Batik doesn't use Avalon. It was the logger for the Avalon ContainerUtil (whether the latter is currently activated, I didn't research), which in turn can't accept a Commons-Logger. If we need to, we can give it an Avalon ConsoleLogger instead. Or, switch to a Container that will allow us to use Commons-Logging. Or, not use a container at all (I don't know what a "container" here does for us anyway.) Or, switch to System.err.println() a la Batik and Xalan. I don't care much here. (Perhaps even ContainerUtil can be subclassed to take a Commons Logging object?) Comments welcome--but we should probably get this working as well. Glen
Chris, Good news--I fixed the issue you brought up below--never mind my previous response. It turns out that ContainerUtil.enableLogging() [1] is just a convenience function for setting a logger for the object in question. Once I realized what it did, it was trivial to directly to do the same directly via a Commons-Logger. I fixed it in two of three places. In the third (AbstractPSTranscoder), no logger was defined for this class, nor is any logging yet done within it. So enableLogging() below wouldn't do anything for it, and so I skipped giving it a Commons-Logger at this time. I'll upload a new patch. Thanks, Glen [1] http://avalon.apache.org/framework/api/org/apache/avalon/framework/container/ContainerUtil.html Jeremias, if you wish I'll happily take care of the issue you mentioned previously. It's just you know more about the static loggers (and centralization of them) than me, so I suspect you can get this looking really clean much more quickly than I after I apply this patch. Your choice, however--I realize you weren't the biggest proponent of this switch to begin with! ;) Thanks, Glen
Created attachment 11066 [details] new logger patch, disregard previous
ContainerUtil is just a little helper to avoid a lot of checks when setting up Avalon-compliant objects. Avalon Framework defines a number of interfaces (LogEnabled for one) which are all optional to implement. ContainerUtil simply does a check if a logger or a Configuration can be set, because one implementation may, the other may not. I'm busy today but I'll try to do the whole thing for the PDF and library code on Thursday evening (CET). Does anyone mind if I just check in these parts? Avalon is not necessary there and when they get extracted into separate components it is better for them to use JCL instead of Avalon Logging.
Fantastic, I committed the change. I will next update the examples--and copy what you do in the PDF libraries elsewhere if its advisable (Please advise!) Once all this is done, back to layout bugs for me...Bugzilla is getting huge. Also, it is good to know that ContainerUtil can work without a CommonsLogger, we just need to set it directly on the object in question. I.e., other portions of Avalon, including those that the Barcode generator may need to interface with FOP, can be kept. Don't forget--all three of the Jakarta web frameworks (Struts, Turbine, Tapestry) as well as Tomcat have now standardized on C-L, we're going to have a lot of happy future web app developers as a result of this change. Thanks for your help! Glen
If you're talking about the Barcode generator (Barcode4J/Krysalis Barcode, right?), that one doesn't need any interfacing over Avalon with FOP. It only uses Avalon services internally. BTW, the Barcode4J FOP extension for HEAD doesn't exist, yet. :-)
batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed