Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logback conversion #889

Merged
merged 25 commits into from Feb 7, 2014
Merged

Logback conversion #889

merged 25 commits into from Feb 7, 2014

Conversation

joshmoore
Copy link
Member

This PR moves any explicit use of log4j to logback. No log4j.properties or similar should remain in the repository, and it should in fact be possible to remove the log4j jars.

A next step will be to remove all hard-coded dependencies on any slf4j binding, perhaps by providing our own which delegates.

Testing instructions will be forthcoming. Certainly all of the merge/test jobs (openbytes, performance, full-repo, test_images_good) need to continue to work as expected.

melissalinkert and others added 10 commits February 4, 2014 19:19
Rather than attempt to do all of the logback configuration
in the ReflectedUniverse, this commits moves the LB logic
out into LogbackTools and only references those static methods
from the ReflectedUniverse. The next step will be to do this
behind a SPI interface.
logback is also found in the artifacts directory
during execution causing a warning to be printed
during testing.
joshmoore and others added 6 commits February 5, 2014 18:01
If log4j is present in ImageJ1 and is found before
logback, then having explicit imports of logback
classes in key loci-plugins classes will prevent
Bio-Formats from working at all.

This commit follows the previous strategy of hiding
the hefty lifting in LogbackTools and just referencing
those methods safely from a ReflectedUniverse in
DebugTools.
The Maven build should now pass.
Build and default logging fixes
@joshmoore
Copy link
Member Author

@sbesson : are you ok to test out the state of matlab logging with a merge build including joshmoore#2 ?

@joshmoore
Copy link
Member Author

Here seems to be the state of this PR as far as I can tell (with @melissalinkert's PR merged):

IJ2+bioformats_package.jar+no logging jars

sh run in ImageJ prints SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder". on the console 🆗

IJ2+bioformats_package.jar+logback jars

Works as expected. Edit>Options>Debug clearly shows BF logging 👍

IJ2+bioformats_package.jar+log4j jars

Nothing on the console: 👍
Edit>Options>Debug shows err >log4j:WARN No appenders could be found for logger (ome.scifio.common.ReflectedUniverse). 👍 This is probably better than the no logging jars scenario.


Next step is to examine the CI fixes.

@joshmoore
Copy link
Member Author

Status update:

  • I'm in the process of looking at bftools.zip as well. There we have the issue that an SLF4J should almost certainly be provided.
  • Log4j will need to be re-added for MATLAB. Commit forthcoming.

This reverts commit 9b1a957.
In order to match the behavior in MATLAB with 4.4,
`DebugTools.enableLogging("INFO")` etc. should continue
to work. Since log4j is shipped with MATLAB 2013, the
only way for this to function is for us to fallback to
configuring Log4j if Logback is not present.

Note: the use of `LOGGER.debug` in `ReflectedUniverse`
makes it unusable for the initial logging configuration.
Likely all uses of LOGGER should be removed from RU which
shouldn't be an issue since for each log statement a
`ReflectException` with the same message is thrown.
@joshmoore
Copy link
Member Author

With the last change, the following seems to work:

bioformats/artifacts$ CLASSPATH=loci_tools.jar jython -c 'import ome.scifio.common.DebugTools as DT; DT.enableLogging("DEBUG"); import org.slf4j.LoggerFactory as LF; LF.getLogger("foo").info("bar"); print LF.getLogger("foo").__class__'
bar
<type 'org.slf4j.impl.Log4jLoggerAdapter'>

bioformats/artifacts$ CLASSPATH=bioformats_package.jar jython -c 'import ome.scifio.common.DebugTools as DT; DT.enableLogging("DEBUG"); import org.slf4j.LoggerFactory as LF; LF.getLogger("foo").info("bar"); print LF.getLogger("foo").__class__'
bar
<type 'ch.qos.logback.classic.Logger'>

However, I'm not sure which logback.xml file is getting used (if any).

@joshmoore
Copy link
Member Author

With the last commit (4579666), the above Jython tests also work with the mvn jars:

export WHICH=loci_tools
export CLASSPATH=$HOME/.m2/repository/ome/$WHICH/5.0.0-SNAPSHOT/$WHICH-5.0.0-SNAPSHOT.jar

@sbesson
Copy link
Member

sbesson commented Feb 6, 2014

Using Bio-Formats 4.4.10

>> [~, version] = bfCheckJavaPath()

version =

4.4.10

>> loci.common.DebugTools.enableLogging('DEBUG');
>> r=bfGetReader('~/Desktop/SoftwareDay-Data/09072011_WTMEFs_ca_001.dv');
loci.formats.in.DeltavisionReader.initFile(/Users/sebastien/Desktop/SoftwareDay-Data/09072011_WTMEFs_ca_001.dv)
Reading header
Populating core metadata
Reading extended header
Populating original metadata
Populating OME metadata
Reading header
Populating original metadata
Populating OME metadata
Expected positive value for PhysicalSizeZ; got 0.0

Using the bfmatlab.zip artifact of http://ci.openmicroscopy.org/view/Failing/job/BIOFORMATS-5.0-merge-daily/712/

>> [~, version] = bfCheckJavaPath()

version =

5.0.0-DEV

>> loci.common.DebugTools.enableLogging('DEBUG');
>> r=bfGetReader('~/Desktop/SoftwareDay-Data/09072011_WTMEFs_ca_001.dv');
DeltavisionReader initializing /Users/sebastien/Desktop/SoftwareDay-Data/09072011_WTMEFs_ca_001.dv
DeltavisionReader initializing /Users/sebastien/Desktop/SoftwareDay-Data/09072011_WTMEFs_ca_001.dv
loci.formats.in.DeltavisionReader.initFile(/Users/sebastien/Desktop/SoftwareDay-Data/09072011_WTMEFs_ca_001.dv)
Reading header
Populating core metadata
Reading extended header
Populating original metadata
Populating OME metadata
Reading header
Populating original metadata
Populating OME metadata
Expected positive value for PhysicalSizeZ; got 0.0

I guess that's a 👍 from the MATLAB side

@melissalinkert
Copy link
Member

bftools changes seem fine on Windows and Linux.

@ximenesuk
Copy link
Contributor

Adding debug="true" to the logback.xml file under the bftools I get the following when running one of the tools:

jrs-macbookpro-25031:tools cblackburn$ ./bfconvert ~/Work/Images/dv/SMN10ul03_R3D_D3D.dv smn.tiff
14:01:59,747 |-INFO in ch.qos.logback.classic.LoggerContext[default] - Could NOT find resource [logback.groovy]
14:01:59,747 |-INFO in ch.qos.logback.classic.LoggerContext[default] - Could NOT find resource [logback-test.xml]
14:01:59,747 |-INFO in ch.qos.logback.classic.LoggerContext[default] - Found resource [logback.xml] at [file:/Users/cblackburn/Work/repos/ome/components/bioformats/tools/logback.xml]
14:01:59,916 |-INFO in ch.qos.logback.core.joran.action.AppenderAction - About to instantiate appender of type [ch.qos.logback.core.ConsoleAppender]
14:01:59,919 |-INFO in ch.qos.logback.core.joran.action.AppenderAction - Naming appender as [stdout]
14:01:59,944 |-INFO in ch.qos.logback.core.joran.action.NestedComplexPropertyIA - Assuming default type [ch.qos.logback.classic.encoder.PatternLayoutEncoder] for [encoder] property
14:01:59,988 |-INFO in ch.qos.logback.classic.joran.action.RootLoggerAction - Setting level of ROOT logger to INFO
14:01:59,988 |-INFO in ch.qos.logback.core.joran.action.AppenderRefAction - Attaching appender named [stdout] to Logger[ROOT]
14:01:59,989 |-INFO in ch.qos.logback.classic.joran.action.ConfigurationAction - End of configuration.
14:01:59,991 |-INFO in ch.qos.logback.classic.joran.JoranConfigurator@41aac3f8 - Registering current configuration as safe fallback point
...

which looks good to me.

@joshmoore
Copy link
Member Author

Great, thanks, @ximenesuk. Merging to open another PR.

joshmoore added a commit that referenced this pull request Feb 7, 2014
@joshmoore joshmoore merged commit ec23327 into ome:dev_5_0 Feb 7, 2014
@joshmoore joshmoore mentioned this pull request Feb 7, 2014
@pwalczysko
Copy link
Member

I do not have the error output in the logging window when having just "loci_tools.jar" in the plugins folder. cf section "Bio-formats only" of https://docs.google.com/document/d/1O5xQqhFpWi1zlMwgDn75qvtYwCXmTCLToIs8M5fYQVA/edit#
screen shot 2014-02-07 at 15 04 30

@joshmoore
Copy link
Member Author

--rebased-to #910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants