update to latest Timbre #8
Comments
|
Long read After reading through that, is the simpler fix just to wait for 4.1.5 to go into RELEASE ? |
|
Yeah, it would probably make sense to wait for the Perhaps AOT might not be necessary and you could have a note that this namespace has to be AOT compiled by using The current situation is a bit problematic as the compiled namespaces will mask the actual namespaces included from the timbre version used in the project. |
|
I guess looking at the code you do need the gen-class to give it the name and add the interface. The only other way would be to use proxy, but that could have performance implications. |
|
maybe if the java classes were done using gen-class in clj then you could remove |
|
I wish it were possible to use gen-class but slf4j (wrongly imho) relies on the SINGLETON field existing in StaticMDCBinder/StaticMarkerBinder rather than using a getter like StaticLoggerBinder does, so I think we're stuck with java unless there's something I've not thought of |
|
Ah that's unfortunate, I wonder if there might be a way to resolve the logger implementation at runtime in the StaticLoggerBinder. Then just the Java classes could be compiled. |
|
It's possible to change |
|
Could the leiningen |
|
I tried setting timbre as provided with |
okay - this makes sense I've made a branch where we can work on implementing this |
|
I'm not sure whether this is going to work. If If we AOT |
|
Yeah it's not looking good unfortunately, only way I can think is to use a proxy instead of creating a class for the adapter. However, I'm pretty sure there would be performance implications with that. Maybe the best approach is to simply include the latest |
|
IMHO the root cause of this is the way SLF4J implements the singleton pattern and ideally (wishful thinking?) it would be fixed there. I've opened a ticket on their JIRA to try to get to the bottom of why they do this, and whether it could be changed. |
|
@rufoa dont they do that so you can bind it at runtime and code to the API and not ship a logger implementation with your library ? |
|
@rufoa I read your JIRA. Misunderstood your proposition |
|
tldr: If SLF4J used getters in the two places I linked, we could do the whole thing in clojure with gen-class and wouldn't need to :aot :all, so timbre wouldn't get dragged in as a dependency |
|
Slf4J is OSS ?
|
|
Hey there, this was just brought to my attention- have pushed @yogthos Thanks for tracking down the cause of the trouble! |
|
@ptaoussanis thanks for pushing a new release out. I think that the main problem is that when aot is used then the version of timbre and its dependencies that slf4j-timbre uses cannot be overriden in the project. It sounds like the only solution at the moment is to keep the library releases in sync. The approach @rufoa proposed would be the ideal solution. @fzakaria, slf4j is oss, so it should be easy to submit a patch there, but if it breaks the existing API they may be hesitant to accept it. I think it's worth a try though. |
|
@fzakaria It's been suggested that the best approach would be to use a Java shim instead of AOT'ing slf4j-timbre. Thus it would not require @ptaoussanis to have to track your changes and vice versa. |
|
@yogthos, @kenrestivo Haven't had an opportunity to look at the slf4j-timbre implementation so can't comment other than to say: any strategy that avoids AOT would definitely be preferable since AOT tends to lead to weird problems like this that are often tough to diagnose and work around. Another long-term option would be to pull slf4j-timbre (or something like it) into Timbre proper so that the two are inherently in sync. Even then would strongly prefer an AOT-free implementation if one's available. Cheers :-) |
|
Adding a new public getter should keep the API even if we keep the variable
|
|
I think so, and that shouldn't get much friction from the sl4fj team hopefully. I definitely agree that avoiding aot would be ideal. At the very least it would be nice to only aot code local to slf4j-timbre. |
|
This patch should do the job and maintain backwards compatibility with existing loggers: |
|
Yea that change looks great. Farid Zakaria On Sun, Dec 27, 2015 at 8:52 PM, rufoa [email protected] wrote:
|
|
What's the likelihood of that change actually making it into an slf4j release? Would it be more practical and/or expedient to change slf4j-timbre to use java shims instead of AOT'ing? |
|
Awesome, that looks promising. BTW please feel free to ping me if there's anything else I can do to assist from this end :-) |
it's just been approved for the upcoming 1.7.14 release
I'm working on it in this branch and it's almost ready. Peter does not need to upgrade anything in Timbre |
|
this is pretty great news, nice to see slf4j guys play ball on this |
|
Just noticed that slf 1.7.14 is out; anyone able to confirm if this issue's now fully resolved? |
|
Oooo +1 Farid Zakaria On Mon, Jan 25, 2016 at 12:11 AM, Peter Taoussanis <[email protected]
|
|
Well, with this: [org.slf4j/log4j-over-slf4j "1.7.14"]
[com.fzakaria/slf4j-timbre "0.2.2"]It gives the same failure, which is Which appears to be due to But with [com.fzakaria/slf4j-timbre "0.2.3"]It fails because it looks like 0.2.3 hasn't been released yet? |
|
It'll be 0.3.0 when it's pushed to clojars probably later tonight. See the pull req / other branch if you want to try it before then |
|
yeah sounds good |
|
Confirmed! Works! Thanks! |
|
excellent, you're welcome Ken |
|
Indeed, much thanks everyone! |
|
@fzakaria Since the SLF4J merged https://jira.qos.ch/browse/SLF4J-347 the |
|
Are you sure about this @yogthos? We make extensive use of |
|
I thought the original reason for using |
|
(Please forgive me while I get back up to speed on this issue) I think at some point, sidetracked by trying to get patches into SLF4J, I must have lost sight of the original problem we were trying to solve:
I assume you're revisiting this issue because this is still a problem?
iirc the SLF4J API problem was the reason for us using Java for some of our classes. Using I think the idea was that getting rid of Java would allow us to get rid of
However, looking at the code now, our assumption doesn't seem to be true at all. As I understand it, we're always going to need
Even if we now restrict Please let me know if I'm misunderstanding or missing anything. As far as I can tell, all the work on sending patches to SLF4J and rewriting this project in pure clojure didn't actually solve the original issue, and I'm not sure where we go from here. |
|
The original problem I had with using First, you end up with a large artifact as all the dependencies along with transient ones end up being compiled into the resulting jar. However, this is a minor issue in my opinion. The second problem is that you're now stuck with the bytecode generated against a particular version of the JVM the library was compiled against. Setting a low byte code version helps with that. For example, if you set the bytecode level to 1.6, then it could be used with JRE 1.6+. Finally, the biggest issue is that classes packaged in the library will be used in favor of source dependencies in projects using it. I think this is the biggest problem with using :aot. If I add a dependency in my project it could be masked by the version that slf4j-timbre was compiled against leading to odd behaviors. Ideally, AOT should happen in the top level project producing the executable that will be run. If it's not possible to get rid of AOT entirely, I think that restricting it to |
|
This caused me huge problems several years ago. I thought getting rid of the Singleton and using getters/setters in the upstream library solved the problem, at least for me, but I'm not sure. In any case, I haven't seen the problem in years, and I'm still extensively using slf4j and timbre. Agreed, no AOT compliation should be done by libraries. My understanding is that gen-class is only done when AOT compiling anyway, and if so, gen-class wouldn't be a problem, but I'm not at all sure about this. |
|
Turns out this is a longstanding issue http://dev.clojure.org/jira/browse/CLJ-322 I think the easiest fix atm is to just delete the classes we don't want from the jar: 80eb0dd Released to clojars as |
|
Interesting, didn't realize it was a compiler bug. Deleting the classes seems like a reasonable workaround. |
|
Hi @fzakaria, The AOT issue has bitten us well. Since we rely on I think the Clojure community could benefit from this, so would you be interested in a patch once it's ready? Cheers! |
|
Hi @aroemers, Thanks for your work on that PR - I will check it out. To be clear, are you still encountering AOT problems with slf4-timbre >=0.3.5? The JIRA ticket you linked is from Jan 2016 and I think I finally resolved the problem a few months ago. Note that the still-unresolved blocker CLJ-322 mentioned on JIRA is what I'm working around in 80eb0dd using
I'd be very interested to see an example project that causes the error you're encountering and/or the output of Thanks again for getting in touch and sharing your proposed fix! I look forward to your response. |
|
@rufoa By the way, I have halted my work on an AOT-less version of SLF4J for now. While I still think it is a good idea to do so, it turned out not to be the root cause of the |
It looks like the
4.1.4version of Timbre relies on an old version of encore that's still built against tools.reader0.9.2. The tools.reader API changed in0.10.0and since slf4j-timbre uses:gen-classit ends up compiling the old versions of the libraries into the jar. See some details on the side effects in this issue taoensso/sente#181Ideally, if there was a way to not include Timbre and its related classes in the jar that would be the best solution I think. Perhaps it's possible to treat them as provided or limit AOT to only slf4j-timbre namespaces explicitly.
The text was updated successfully, but these errors were encountered: