fzakaria / slf4j-timbre

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

timbre 4.5.0 api breakage #20

Closed emidln closed 8 years ago

emidln commented 8 years ago

This commit (included in v.4.5.0 tag): https://github.com/ptaoussanis/timbre/commit/e5b95cbd753a8b0c20db7ec7d759fd1aab9a92de seems to break sl4j-timbre with this:

Caused by: clojure.lang.ArityException: Wrong number of args (9) passed to: timbre/-log!
        at clojure.lang.AFn.throwArity(AFn.java:429)
        at clojure.lang.AFn.invoke(AFn.java:67)
        at slf4j_timbre.adapter$_info_String_Object.doInvoke(adapter.clj:81)
        at clojure.lang.RestFn.invoke(RestFn.java:439)
        at com.github.fzakaria.slf4j.timbre.TimbreLoggerAdapter.info(Unknown Source)

Pinning to v4.4.0 of timbre resolves the issue. I might be able to contribute a PR later, but this is a heads up to anyone else playing along at home.

brunchboy commented 8 years ago

I was just coming here to report the same issue, although in my case trying to translate a warning rather than an info message:

1. Unhandled clojure.lang.ExceptionInfo
   Invalid logging level: clojure.lang.ArityException: Wrong number of
   args (9) passed to: timbre/-log!
   {:level #error {
    :cause "Wrong number of args (9) passed to: timbre/-log!"
    :via
    [{:type clojure.lang.ArityException
      :message "Wrong number of args (9) passed to: timbre/-log!"
      :at [clojure.lang.AFn throwArity "AFn.java" 429]}]
    :trace
    [[clojure.lang.AFn throwArity "AFn.java" 429]
     [clojure.lang.AFn invoke "AFn.java" 67]
     [slf4j_timbre.adapter$_warn_String doInvoke "adapter.clj" 80]
     [clojure.lang.RestFn invoke "RestFn.java" 423]
     [com.github.fzakaria.slf4j.timbre.TimbreLoggerAdapter warn nil -1]

I didn’t see anything in the Timbre change log that seemed to suggest this kind of breakage was expected, but looking at the documentation for -log! that seems to be because Peter does not expect anyone to call that function directly:

(defn -log! "Core low-level log fn. Implementation detail!"
  [config level ?ns-str ?file ?line msg-type ?err vargs_
   ?base-data callsite-id]
  ; ...
)

Surely it would be safer to call log!?

(defmacro log! ; Public wrapper around `-log!`
  ; ...
)
ptaoussanis commented 8 years ago

@emidln Hi Brandon, this was just brought to my attention.

Just skimmed sl4j-timbre's code and it looks like it's actually already using the correct log! API. Have you tried running lein clean?

brunchboy commented 8 years ago

Yes, lein clean was the first thing I tried, actually. I can see it in my command history nearly two hours ago. But I just tried again to be double certain, and the problem returns as soon as I change my dependency from timbre 4.4.0 to 4.5.0, even if I lein clean in between.

brunchboy commented 8 years ago

For what it’s worth, I see no warnings in lein deps :tree, either.

ptaoussanis commented 8 years ago

the problem returns as soon as I change my dependency from timbre 4.4.0 to 4.5.0, even if I lein clean in between.

Just to clarify: you need to run lein clean after upgrading to v4.5.0.

As you can see at: https://github.com/fzakaria/slf4j-timbre/blob/master/src/slf4j_timbre/adapter.clj#L51, slf4j-timbre only ever calls Timbre's public log! API.

The log! call will macro-expand to a -log! function call. So if you're seeing a -log! call with 9 arguments in slf4j-timbre, that implies that there's a Timbre v4.4.0 log! expansion in your codebase, trying to call into the Timbre v4.5.0 API.

I.e. you've almost certainly got out-of-date build artifacts somehow.

In case lein clean isn't working for some reason, you may like to manually move/remove your project's target dir.

brunchboy commented 8 years ago

OK, I can confirm that lein clean eliminates my target dir completely. And starting from that state, with timbre 4.5.0 as my dependency, I receive the same crash I cited above. I need to go to bed for now, but in case it is of any help, you can find the relevant project here: https://github.com/brunchboy/beat-link-trigger

ptaoussanis commented 8 years ago

That's very odd. Okay, will keep looking. Appreciate the confirmation - cheers!

brunchboy commented 8 years ago

Rats, that’s not going to help you much, at least not easily, because beat-link (a dependency, and another project of mine) is a Maven project, and I can’t deploy snapshots as easily as I can for clojars. So you would probably need to download and install that snapshot locally, which is probably more trouble than it is worth. Hopefully there is an easier way to reproduce this. Cheers!

ptaoussanis commented 8 years ago

@emidln @brunchboy This should be resolved with [com.taoensso/timbre "4.5.1"], please let me know if you can confirm? Thanks!

brunchboy commented 8 years ago

I can confirm that version 4.5.1 resolves the issue I was having. Thank you, @ptaoussanis !

ptaoussanis commented 8 years ago

Great, thanks for the confirmation!

@fzakaria Hi Farid- should be good to close at your convenience.

fzakaria commented 8 years ago

@ptaoussanis thanks for looking into it. I started a new job so I've been very busy. Closing issue.

emidln commented 8 years ago

I can confirm this fixed my issue! Thanks @ptaoussanis and @brunchboy