Replace loci_tools.jar with bioformats_package.jar #403
Conversation
|
Current status:
|
|
Hi @GenevieveBuckley. Sorry, I just saw this. Are things now working for you with 43c9b7f ? |
|
Not quite. I think I've inched a little bit closer. ChangedI've changed these lines Lines 347 to 348 in 68c560e to this instead logback = jpype.JPackage('ch.qos.logback.classic')
logger_context = logback.LoggerContext() logback.BasicConfigurator().setContext(logger_context)
logback.BasicConfigurator().configure(logger_context)I'm relatively confident that's probably right, but I don't know how to test it for sure. Still stuck on thisI'm still stuck on what these two lines should be: Lines 349 to 350 in 68c560e I can figure out how to get |
|
Gotcha. It might be easier to use the This prevents the need to load more of the logback internals. But if that is necessary, you can see all the necessary classes & code in https://github.com/ome/ome-common-java/blob/9685c5d2f6a7d05ee6a100987fe34c0d573f54a5/src/main/java/loci/common/LogbackTools.java#L75-L84 Note: |
|
@GenevieveBuckley Sorry, I made this need a rebase via #399 |
tacaswell
force-pushed
the
bioformats-package-jar
branch
from
43c9b7f
to
1075f48
Compare
|
I took the liberty of rebasing, hopefully that is OK. |
tacaswell
force-pushed
the
bioformats-package-jar
branch
from
1075f48
to
fb0b988
Compare
|
I rebased again. What is the state of this? I would like to do a release ASAP and getting in log4j fixes is obviously good! |
|
Deferring to @GenevieveBuckley on the functionality she wanted, but reading the code 👍 |
|
Status: log4j is replaced by logback, but it may not log things correctly. Current status is summarized here #403 (comment) I haven't had time to try Josh's suggestion here. It is unlikely I can prioritize that on the timeframe you want. Do others have more bandwidth right now? |
|
OK, I'm going to do the release without this and trust that someone down-stream is making sure that patched versions of log4j are being used. |
https://docs.openmicroscopy.org/bio-formats/6.9.1/developers/logging.html Approach suggested by Josh Moore
Codecov Report
@@ Coverage Diff @@
## main #403 +/- ##
==========================================
- Coverage 62.61% 62.58% -0.04%
==========================================
Files 31 31
Lines 4486 4498 +12
==========================================
+ Hits 2809 2815 +6
- Misses 1677 1683 +6
Continue to review full report at Codecov.
|
|
Update: I've added the line I think Josh is right, the DebugTools logging is perhaps the easiest way. The other suggestion is not possible on my machine for the same reason I had trouble earlier (anytime I try to access Logger, I get a class definition error). I swear at one point all the tests passed with |
We did a slicerator release so I thought at least that one should be fixed... |
|
More details on the failing tests here: #416 |
|
Any updates here? This would be nice to have, also to be able to use the newest bioformats versions, which do not include the loci_tools.jar anymore. |
|
All the changes here are still 👍 from the OME point-of-view. |
|
No, no change here. |
|
...and that's still the case today. If the main branch tests aren't passing, that means I don't have a good way to test the changes on this PR branch. So the status is the same: I think my changes here are helpful, but I have no way to know for sure. This is probably something that a maintainer is going to need to dig into. There are a handful of open issues about failing test on main: |
Closes #400
This PR aims to do two things:
loci_tools.jarwithbioformats_package.jar, andlog4jwithlogbackWe want to do this due to the Log4Shell vulnerability (for more details, see Josh's post here https://forum.image.sc/t/cve-2021-44228-log4shell-assessment-for-omero-bio-formats/61032 and the linked github issues above)