search

fzakaria / slf4j-timbre

SLF4J binding for Clojure's Timbre
Eclipse Public License 1.0
94 stars 24 forks source link

update to latest Timbre #8

Closed yogthos closed 7 years ago

yogthos commented 8 years ago

It looks like the 4.1.4 version of Timbre relies on an old version of encore that's still built against tools.reader 0.9.2. The tools.reader API changed in 0.10.0 and since slf4j-timbre uses :gen-class it ends up compiling the old versions of the libraries into the jar. See some details on the side effects in this issue https://github.com/ptaoussanis/sente/issues/181

Ideally, 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.

fzakaria commented 8 years ago

Long read

After reading through that, is the simpler fix just to wait for 4.1.5 to go into RELEASE ? I'm not too familiar with the different options for AOT compilation. (it's a bit out of my wheel house)

yogthos commented 8 years ago

Yeah, it would probably make sense to wait for the 4.1.5 final to show up on Clojars, just wanted to give a heads up on the issue. I'm not sure if there is a way to exclude external libs from AOT either, but figured I'd throw that out there.

Perhaps AOT might not be necessary and you could have a note that this namespace has to be AOT compiled by using :gen-class somewhere in the project?

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.

yogthos commented 8 years ago

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.

yogthos commented 8 years ago

maybe if the java classes were done using gen-class in clj then you could remove :aot :all in the project.clj and the code could ship uncompiled.

rufoa commented 8 years ago

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

https://github.com/qos-ch/slf4j/blob/2b23d26bb008b9eec151cc8244deec3bb4eed778/slf4j-api/src/main/java/org/slf4j/MDC.java#L90

https://github.com/qos-ch/slf4j/blob/2b23d26bb008b9eec151cc8244deec3bb4eed778/slf4j-api/src/main/java/org/slf4j/MarkerFactory.java#L52

yogthos commented 8 years ago

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.

yogthos commented 8 years ago

It's possible to change :aot :all to :aot [slf4j-timbre.factory], then if the slf4j-timbre.factory namespace called the require in its -init function then it would be possible to avoid compiling slf4j-timbre.adapter as AOT. However, new-logger would probably have to be initialized using a proxy at that point. Which might not be a big deal as this wouldn't be called all that often I don't think.

rufoa commented 8 years ago

Could the leiningen :provided profile be a solution to this? Never used it before but it seems to be the standard way to exclude e.g. bouncycastle from jars

yogthos commented 8 years ago

I tried setting timbre as provided with [com.taoensso/timbre "4.1.4" :scope "provided"], but the classes still end up being generated unfortunately. The way AOT compilation works is that it needs to generate classes for anything that's referenced from the namespace that's getting compiled.

rufoa commented 8 years ago

It's possible to change :aot :all to :aot [slf4j-timbre.factory], then if the slf4j-timbre.factory namespace called the require in its -init function then it would be possible to avoid compiling slf4j-timbre.adapter as AOT. However, new-logger would probably have to be initialized using a proxy at that point. Which might not be a big deal as this wouldn't be called all that often I don't think.

okay - this makes sense

I've made a branch where we can work on implementing this

rufoa commented 8 years ago

I'm not sure whether this is going to work.

If slf4j-timbre.adapter isn't AOT'd then we don't seem to be able to use com.github.fzakaria.slf4j.timbre.TimbreLoggerAdapter in slf4j-timbre.factory. This also affects attempts to instantiate it via reflection rather than proxying.

If we AOT slf4j-timbre.adapter then we need to move the require of timbre into -init so it doesn't get AOT'd. This causes clojure to complain about the references to the timbre/error macro etc. I tried to intern these symbols in a fake timbre ns then swap them out using remove-ns and alter-var-root, but it doesn't seem to work properly.

yogthos commented 8 years ago

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 encore lib in the project explicitly after all.

rufoa commented 8 years ago

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.

http://jira.qos.ch/browse/SLF4J-347

fzakaria commented 8 years ago

@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 ?

fzakaria commented 8 years ago

@rufoa I read your JIRA. Misunderstood your proposition

rufoa commented 8 years ago

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

fzakaria commented 8 years ago

Slf4J is OSS ? Let's submit the patch. At least fix it for ongoing versions. On 26 Dec 2015 6:08 p.m., "rufoa" notifications@github.com wrote:

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

— Reply to this email directly or view it on GitHub https://github.com/fzakaria/slf4j-timbre/issues/8#issuecomment-167375326 .

ptaoussanis commented 8 years ago

Hey there, this was just brought to my attention- have pushed [com.taoensso/timbre "4.1.5"] to Clojars. Will also push 4.2.0 in a few mins.

@yogthos Thanks for tracking down the cause of the trouble!

yogthos commented 8 years ago

@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.

kenrestivo commented 8 years ago

@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.

ptaoussanis commented 8 years ago

@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 :-)

fzakaria commented 8 years ago

Adding a new public getter should keep the API even if we keep the variable public access but solve the problem ? On 26 Dec 2015 10:04 p.m., "Peter Taoussanis" notifications@github.com wrote:

@yogthos https://github.com/yogthos, @kenrestivo https://github.com/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 workaround.

Another long-term option would be to pull slf4j-timbre (or something like it) into Timbre proper so that the two are automatically kept in sync. Even then would strongly prefer an AOT-free implementation if one's available.

Cheers :-)

— Reply to this email directly or view it on GitHub https://github.com/fzakaria/slf4j-timbre/issues/8#issuecomment-167385936 .

yogthos commented 8 years ago

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.

rufoa commented 8 years ago

This patch should do the job and maintain backwards compatibility with existing loggers:

https://github.com/qos-ch/slf4j/compare/master...rufoa:slf4j-347

fzakaria commented 8 years ago

Yea that change looks great. Did you cut a PR ?

Farid Zakaria

On Sun, Dec 27, 2015 at 8:52 PM, rufoa notifications@github.com wrote:

This patch should do the job and maintain backwards compatibility with existing loggers:

qos-ch/slf4j@master...rufoa:slf4j-347 https://github.com/qos-ch/slf4j/compare/master...rufoa:slf4j-347

— Reply to this email directly or view it on GitHub https://github.com/fzakaria/slf4j-timbre/issues/8#issuecomment-167484997 .

rufoa commented 8 years ago

https://github.com/qos-ch/slf4j/pull/138

kenrestivo commented 8 years ago

What's the likelihood of that change actually making it into an slf4j release? When would Timbre and/or slf4j-timbre be able to upgrade to it? What is to be done about other projects that depend on older versions of slf4j that don't have this fix?

Would it be more practical and/or expedient to change slf4j-timbre to use java shims instead of AOT'ing?

fzakaria commented 8 years ago

You can always force a dependency (even transitive) to a specific version if you specify it in your project.clg/pom.xml

So for those facing this issue would be able to fix it in that regard - can't speak to the likelyhood the PR is approved.

The original implementation is Java-shim (although it had AOT still). On 30 Dec 2015 4:46 p.m., "ken restivo" notifications@github.com wrote:

What's the likelihood of that change actually making it into an slf4j release? When would Timbre and/or slf4j-timbre be able to upgrade to it? What is to be done about other projects that depend on older versions of slf4j that don't have this fix?

Would it be more practical and/or expedient to change slf4j-timbre to use java shims instead of AOT'ing?

— Reply to this email directly or view it on GitHub https://github.com/fzakaria/slf4j-timbre/issues/8#issuecomment-168100963 .

kenrestivo commented 8 years ago

OK I guess I'll wait to see how this shakes out.

Would it be possible to just remove the AOT then? I'm not clear on why AOT was required.

rufoa commented 8 years ago

current discussion is happening here https://github.com/rufoa/slf4j/commit/200e6c7e2ddb9558abd1eac72b84a25d9db07433

yogthos commented 8 years ago

looks like this might actually happen :)

ptaoussanis commented 8 years ago

Awesome, that looks promising. BTW please feel free to ping me if there's anything else I can do to assist from this end :-)

rufoa commented 8 years ago

What's the likelihood of that change actually making it into an slf4j release?

it's just been approved for the upcoming 1.7.14 release

When would Timbre and/or slf4j-timbre be able to upgrade to it?

I'm working on it in this branch and it's almost ready. Peter does not need to upgrade anything in Timbre

yogthos commented 8 years ago

this is pretty great news, nice to see slf4j guys play ball on this

ptaoussanis commented 8 years ago

Just noticed that slf 1.7.14 is out; anyone able to confirm if this issue's now fully resolved?

fzakaria commented 8 years ago

Oooo +1

Farid Zakaria

On Mon, Jan 25, 2016 at 12:11 AM, Peter Taoussanis <notifications@github.com

wrote:

Just noticed that slf 1.7.14 is out; anyone able to confirm if this issue's now fully resolved?

— Reply to this email directly or view it on GitHub https://github.com/fzakaria/slf4j-timbre/issues/8#issuecomment-174432864 .

kenrestivo commented 8 years ago

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

Exception in thread "main" java.lang.ExceptionInInitializerError
    at clojure.main.<clinit>(main.java:20)
Caused by: java.lang.ExceptionInInitializerError, compiling:(cljs/analyzer.cljc:1:1)
    at clojure.lang.Compiler.load(Compiler.java:7239)
    at clojure.lang.RT.loadResourceScript(RT.java:371)
    at clojure.lang.RT.loadResourceScript(RT.java:362)
    at clojure.lang.RT.load(RT.java:446)
    at clojure.lang.RT.load(RT.java:412)
    at clojure.core$load$fn__5448.invoke(core.clj:5866)
    at clojure.core$load.doInvoke(core.clj:5865)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5671)
    at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
    at clojure.core$load_lib.doInvoke(core.clj:5710)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$load_libs.doInvoke(core.clj:5749)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$require.doInvoke(core.clj:5832)
    at clojure.lang.RestFn.invoke(RestFn.java:457)
    at figwheel_sidecar.utils$eval21$loading__5340__auto____22.invoke(utils.clj:1)
    at figwheel_sidecar.utils$eval21.invoke(utils.clj:1)
    at clojure.lang.Compiler.eval(Compiler.java:6782)
    at clojure.lang.Compiler.eval(Compiler.java:6771)
    at clojure.lang.Compiler.load(Compiler.java:7227)
    at clojure.lang.RT.loadResourceScript(RT.java:371)
    at clojure.lang.RT.loadResourceScript(RT.java:362)
    at clojure.lang.RT.load(RT.java:446)
    at clojure.lang.RT.load(RT.java:412)
    at clojure.core$load$fn__5448.invoke(core.clj:5866)
    at clojure.core$load.doInvoke(core.clj:5865)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5671)
    at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
    at clojure.core$load_lib.doInvoke(core.clj:5710)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$load_libs.doInvoke(core.clj:5749)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$require.doInvoke(core.clj:5832)
    at clojure.lang.RestFn.invoke(RestFn.java:1289)
    at figwheel_sidecar.system$eval15$loading__5340__auto____16.invoke(system.clj:1)
    at figwheel_sidecar.system$eval15.invoke(system.clj:1)
    at clojure.lang.Compiler.eval(Compiler.java:6782)
    at clojure.lang.Compiler.eval(Compiler.java:6771)
    at clojure.lang.Compiler.load(Compiler.java:7227)
    at clojure.lang.RT.loadResourceScript(RT.java:371)
    at clojure.lang.RT.loadResourceScript(RT.java:362)
    at clojure.lang.RT.load(RT.java:446)
    at clojure.lang.RT.load(RT.java:412)
    at clojure.core$load$fn__5448.invoke(core.clj:5866)
    at clojure.core$load.doInvoke(core.clj:5865)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5671)
    at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
    at clojure.core$load_lib.doInvoke(core.clj:5710)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$load_libs.doInvoke(core.clj:5749)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$require.doInvoke(core.clj:5832)
    at clojure.lang.RestFn.invoke(RestFn.java:457)
    at figwheel_sidecar.repl_api$eval9$loading__5340__auto____10.invoke(repl_api.clj:1)
    at figwheel_sidecar.repl_api$eval9.invoke(repl_api.clj:1)
    at clojure.lang.Compiler.eval(Compiler.java:6782)
    at clojure.lang.Compiler.eval(Compiler.java:6771)
    at clojure.lang.Compiler.load(Compiler.java:7227)
    at clojure.lang.RT.loadResourceScript(RT.java:371)
    at clojure.lang.RT.loadResourceScript(RT.java:362)
    at clojure.lang.RT.load(RT.java:446)
    at clojure.lang.RT.load(RT.java:412)
    at clojure.core$load$fn__5448.invoke(core.clj:5866)
    at clojure.core$load.doInvoke(core.clj:5865)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5671)
    at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
    at clojure.core$load_lib.doInvoke(core.clj:5710)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$load_libs.doInvoke(core.clj:5749)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:634)
    at clojure.core$use.doInvoke(core.clj:5843)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at user$eval3$loading__5340__auto____4.invoke(user.clj:1)
    at user$eval3.invoke(user.clj:1)
    at clojure.lang.Compiler.eval(Compiler.java:6782)
    at clojure.lang.Compiler.eval(Compiler.java:6771)
    at clojure.lang.Compiler.load(Compiler.java:7227)
    at clojure.lang.RT.loadResourceScript(RT.java:371)
    at clojure.lang.RT.loadResourceScript(RT.java:358)
    at clojure.lang.RT.maybeLoadResourceScript(RT.java:354)
    at clojure.lang.RT.doInit(RT.java:468)
    at clojure.lang.RT.<clinit>(RT.java:330)
    ... 1 more
Caused by: java.lang.ExceptionInInitializerError
    at java.lang.Class.forName0(Native Method)
    at java.lang.Class.forName(Class.java:348)
    at clojure.lang.RT.classForName(RT.java:2154)
    at clojure.lang.RT.classForName(RT.java:2163)
    at clojure.lang.RT.loadClassForName(RT.java:2182)
    at clojure.lang.RT.load(RT.java:436)
    at clojure.lang.RT.load(RT.java:412)
    at clojure.core$load$fn__5448.invoke(core.clj:5866)
    at clojure.core$load.doInvoke(core.clj:5865)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5671)
    at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
    at clojure.core$load_lib.doInvoke(core.clj:5710)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$load_libs.doInvoke(core.clj:5749)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:634)
    at clojure.core$use.doInvoke(core.clj:5843)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.tools.reader.reader_types$loading__5340__auto____266.invoke(reader_types.clj:9)
    at clojure.tools.reader.reader_types__init.load(Unknown Source)
    at clojure.tools.reader.reader_types__init.<clinit>(Unknown Source)
    at java.lang.Class.forName0(Native Method)
    at java.lang.Class.forName(Class.java:348)
    at clojure.lang.RT.classForName(RT.java:2154)
    at clojure.lang.RT.classForName(RT.java:2163)
    at clojure.lang.RT.loadClassForName(RT.java:2182)
    at clojure.lang.RT.load(RT.java:436)
    at clojure.lang.RT.load(RT.java:412)
    at clojure.core$load$fn__5448.invoke(core.clj:5866)
    at clojure.core$load.doInvoke(core.clj:5865)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5671)
    at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
    at clojure.core$load_lib.doInvoke(core.clj:5710)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$load_libs.doInvoke(core.clj:5749)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:634)
    at clojure.core$use.doInvoke(core.clj:5843)
    at clojure.lang.RestFn.invoke(RestFn.java:421)
    at clojure.tools.reader$loading__5340__auto____486.invoke(reader.clj:9)
    at clojure.tools.reader__init.load(Unknown Source)
    at clojure.tools.reader__init.<clinit>(Unknown Source)
    at java.lang.Class.forName0(Native Method)
    at java.lang.Class.forName(Class.java:348)
    at clojure.lang.RT.classForName(RT.java:2154)
    at clojure.lang.RT.classForName(RT.java:2163)
    at clojure.lang.RT.loadClassForName(RT.java:2182)
    at clojure.lang.RT.load(RT.java:436)
    at clojure.lang.RT.load(RT.java:412)
    at clojure.core$load$fn__5448.invoke(core.clj:5866)
    at clojure.core$load.doInvoke(core.clj:5865)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5671)
    at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
    at clojure.core$load_lib.doInvoke(core.clj:5710)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$load_libs.doInvoke(core.clj:5749)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$require.doInvoke(core.clj:5832)
    at clojure.lang.RestFn.invoke(RestFn.java:805)
    at cljs.analyzer$eval27$loading__5340__auto____28.invoke(analyzer.cljc:9)
    at cljs.analyzer$eval27.invoke(analyzer.cljc:9)
    at clojure.lang.Compiler.eval(Compiler.java:6782)
    at clojure.lang.Compiler.eval(Compiler.java:6771)
    at clojure.lang.Compiler.load(Compiler.java:7227)
    ... 94 more
Caused by: java.io.FileNotFoundException: Could not locate clojure/tools/reader/impl/ExceptionInfo__init.class or clojure/tools/reader/impl/ExceptionInfo.clj on classpath.
    at clojure.lang.RT.load(RT.java:449)
    at clojure.lang.RT.load(RT.java:412)
    at clojure.core$load$fn__5448.invoke(core.clj:5866)
    at clojure.core$load.doInvoke(core.clj:5865)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5671)
    at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
    at clojure.core$load_lib.doInvoke(core.clj:5710)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$load_libs.doInvoke(core.clj:5749)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.core$require.doInvoke(core.clj:5832)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.tools.reader.impl.utils$loading__5340__auto____268.invoke(utils.clj:9)
    at clojure.tools.reader.impl.utils__init.load(Unknown Source)
    at clojure.tools.reader.impl.utils__init.<clinit>(Unknown Source)
    ... 165 more

Which appears to be due to http://yogthos.net/posts/2015-12-26-AOTGotchas.html

But with 

 [com.fzakaria/slf4j-timbre "0.2.3"]

It fails because it looks like 0.2.3 hasn't been released yet?

rufoa commented 8 years ago

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

fzakaria commented 8 years ago

@yogthos closing this as we've updated. @rufoa also got his merges accepted into slf4j-api which means everything is much more peachy

yogthos commented 8 years ago

yeah sounds good

kenrestivo commented 8 years ago

Confirmed!

Works!

Thanks!

rufoa commented 8 years ago

excellent, you're welcome Ken

ptaoussanis commented 8 years ago

Indeed, much thanks everyone!

yogthos commented 7 years ago

@fzakaria Since the SLF4J merged https://jira.qos.ch/browse/SLF4J-347 the :aot shouldn't be necessary.

rufoa commented 7 years ago

Are you sure about this @yogthos? We make extensive use of gen-class which I believe necessitates aot - unless you've somehow managed to get it working without?

yogthos commented 7 years ago

I thought the original reason for using gen-class was the problem with the SFL4J API. Now that it's been fixed, wouldn't be possible to get rid of that as you did in your branch earlier?

rufoa commented 7 years ago

(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:

yogthos posted: since slf4j-timbre uses :gen-class it ends up compiling the old versions of the libraries into the jar.

I assume you're revisiting this issue because this is still a problem?

yogthos posted: I thought the original reason for using gen-class was the problem with the SFL4J API. Now that it's been fixed, wouldn't be possible to get rid of that as you did in your branch earlier?

iirc the SLF4J API problem was the reason for us using Java for some of our classes. Using :gen-class in a pure clojure project was the original aim. Once we got the patch to SLF4J accepted, we were able to successfully get rid of all our Java code, replacing it :gen-class.

I think the idea was that getting rid of Java would allow us to get rid of :aot :all, which would solve our original problem:

yogthos posted: maybe if the java classes were done using gen-class in clj then you could remove :aot :all in the project.clj and the code could ship uncompiled.

rufoa posted: 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

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 gen-class so can never get rid of AOT entirely.

yogthos posted: Ideally, 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.

Even if we now restrict :aot to our own slf4j-timbre.* classes, and specify timbre as a :provided dependency, Timbre still gets compiled into our artifacts.

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.

yogthos commented 7 years ago

The original problem I had with using :gen-class and :aot was that it generates Java bytecode using the versions of the libraries specified in the project when the artifact is generated. There are a number of problems with that.

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 slf4j-timbre.* would still be an improvement as it minimizes the AOT surface.

kenrestivo commented 7 years ago

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.

rufoa commented 7 years ago

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: https://github.com/fzakaria/slf4j-timbre/commit/80eb0dd13303d8cb612c2c89cc199ea766bace58

Released to clojars as [com.fzakaria/slf4j-timbre "0.3.5"]