Clojure-Contrib

AOT compilation of clojure-contrib.jar pre-sets logging implementation

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

Using ant to build clojure-contrib.jar causes clojure.contrib.logging to be AOT-compiled.

This means that the classpath at contrib build time affects which logging implementation is used, not the classpath in use when the code using c.c.l is compiled.

Using a standard pre-built clojure-contrib.jar one can never use anything but java.util.logging! That's bad for anyone who uses released snapshots, or who uses different log implementations on different projects, but doesn't want to maintain different builds of clojure-contrib.jar.

Furthermore, someone building clojure-contrib.jar with, say, log4j in their classpath will produce a jar in which c.c.l will fail when log4j is unavailable:

user=> (use 'clojure.contrib.logging)
    java.lang.NoClassDefFoundError: org/apache/log4j/Level (NO_SOURCE_FILE:0)

c.c.l should be adjusted so that the logging implementation is not discovered until runtime, or otherwise be removed from the AOT-compiled set.

Activity

Hide
Assembla Importer added a comment -

rnewman said: This simple diff fixes things, albeit at the cost of runtime work.

diff --git a/build.xml b/build.xml
index e72d36e..fa1161c 100644
--- a/build.xml
+++ b/build.xml
@@ -118,6 +118,7 @@
         <exclude name="**/repl_utils/javadoc.clj"/>
         <exclude name="clojure/contrib/load_all.clj"/>
         <exclude name="**/tests.clj"/>
+        <exclude name="**/logging.clj"/>
       </fileset>
       <chainedmapper>
         <packagemapper from="${src}/*.clj" to="*" />
Show
Assembla Importer added a comment - rnewman said: This simple diff fixes things, albeit at the cost of runtime work.
diff --git a/build.xml b/build.xml
index e72d36e..fa1161c 100644
--- a/build.xml
+++ b/build.xml
@@ -118,6 +118,7 @@
         <exclude name="**/repl_utils/javadoc.clj"/>
         <exclude name="clojure/contrib/load_all.clj"/>
         <exclude name="**/tests.clj"/>
+        <exclude name="**/logging.clj"/>
       </fileset>
       <chainedmapper>
         <packagemapper from="${src}/*.clj" to="*" />
Hide
Assembla Importer added a comment -

cemerick said: That's not really a solution, especially for those of us who can't (or don't want to) ship .clj files.

Would folks be amenable to having each logging system setup fn return a map of implementation fns, which can sit in a top-level delay? This would push selection of a logging system out of compile-time, regardless of AOT. Adds the cost of a deref to each logging call, but I can certainly live with that.

Show
Assembla Importer added a comment - cemerick said: That's not really a solution, especially for those of us who can't (or don't want to) ship .clj files. Would folks be amenable to having each logging system setup fn return a map of implementation fns, which can sit in a top-level delay? This would push selection of a logging system out of compile-time, regardless of AOT. Adds the cost of a deref to each logging call, but I can certainly live with that.
Hide
Assembla Importer added a comment -

rnewman said: Yes, I don't consider it a solution, but it at least allows me and other people to use something other than j.u.l until Alex rewrites this library.

Sure would be nice to have eval-when���

Show
Assembla Importer added a comment - rnewman said: Yes, I don't consider it a solution, but it at least allows me and other people to use something other than j.u.l until Alex rewrites this library. Sure would be nice to have eval-when���
Hide
Assembla Importer added a comment -

cemerick said: I don't think what I suggested this morning would require much. If Alex et al. are amenable to it, I'd be happy to produce a patch when I have some spare moments.

Show
Assembla Importer added a comment - cemerick said: I don't think what I suggested this morning would require much. If Alex et al. are amenable to it, I'd be happy to produce a patch when I have some spare moments.
Hide
Assembla Importer added a comment -

mattrevelle said: I'm going to go ahead and write up a patch for Chas' proposition. Alex, if you already have something in mind then feel free to kick me off the ticket.

Show
Assembla Importer added a comment - mattrevelle said: I'm going to go ahead and write up a patch for Chas' proposition. Alex, if you already have something in mind then feel free to kick me off the ticket.
Hide
Assembla Importer added a comment -

mattrevelle said: [file:deght-7A8r3P1ZeJe5aVNr]

Show
Assembla Importer added a comment - mattrevelle said: [file:deght-7A8r3P1ZeJe5aVNr]
Hide
Assembla Importer added a comment -

mattrevelle said: Patch attached above. Feedback very welcome.

Show
Assembla Importer added a comment - mattrevelle said: Patch attached above. Feedback very welcome.
Hide
Assembla Importer added a comment -

ataggart said: I didn't notice this ticket until today; guess the initial assignment got lost in all the email noise.

I'll take a look at what's been done so far.

Show
Assembla Importer added a comment - ataggart said: I didn't notice this ticket until today; guess the initial assignment got lost in all the email noise. I'll take a look at what's been done so far.
Hide
Assembla Importer added a comment -

ataggart said: I must say, I'm a bit put off by the notion that we should have to discard perfectly useful aspects of Clojure to suit some peoples reaction to whether a JAR contains some .clj files vs. exclusively .class files. AOT should be an option for code, not a mandate that infects all code in contrib.

That said, I'm still going to go over the proposed changes.

Show
Assembla Importer added a comment - ataggart said: I must say, I'm a bit put off by the notion that we should have to discard perfectly useful aspects of Clojure to suit some peoples reaction to whether a JAR contains some .clj files vs. exclusively .class files. AOT should be an option for code, not a mandate that infects all code in contrib. That said, I'm still going to go over the proposed changes.
Hide
Assembla Importer added a comment -

technomancy said: I recommend turning off AOT until Alex gets a chance to go over this patch in more detail.

Show
Assembla Importer added a comment - technomancy said: I recommend turning off AOT until Alex gets a chance to go over this patch in more detail.
Hide
Assembla Importer added a comment -

scgilardi said: if nobody objects, I plan to make Phil's suggested minimal workaround patch "change build.xml to turn off aot for this one file" today.

Show
Assembla Importer added a comment - scgilardi said: if nobody objects, I plan to make Phil's suggested minimal workaround patch "change build.xml to turn off aot for this one file" today.
Hide
Assembla Importer added a comment -

mattrevelle said: Sounds good.

Show
Assembla Importer added a comment - mattrevelle said: Sounds good.
Hide
Assembla Importer added a comment -

scgilardi said: Checked in to the master branch: change to build.xml so it doesn't compile logging.clj. (suggested by Richard Newman, Phil Hagelberg).

Do we need this interim change on the 1.1.x branch? Re: 1.1.x, Stuart mentioned "assume all library code is frozen on these branches". I don't plan to push this change to the 1.1.x branch without more discussion.

Show
Assembla Importer added a comment - scgilardi said: Checked in to the master branch: change to build.xml so it doesn't compile logging.clj. (suggested by Richard Newman, Phil Hagelberg). Do we need this interim change on the 1.1.x branch? Re: 1.1.x, Stuart mentioned "assume all library code is frozen on these branches". I don't plan to push this change to the 1.1.x branch without more discussion.
Hide
Assembla Importer added a comment -

rnewman said: The problem is actually worse for people who use a pre-packaged
release: c.c.logging will use the logging implementation available on
the machine on which the release was built. I'm in favor of pushing
the fix wherever it can go.

Show
Assembla Importer added a comment - rnewman said: The problem is actually worse for people who use a pre-packaged release: c.c.logging will use the logging implementation available on the machine on which the release was built. I'm in favor of pushing the fix wherever it can go.
Hide
Assembla Importer added a comment -

stuart.sierra said: Added to 1.1.x branch, included in 1.1.0-RC2

Show
Assembla Importer added a comment - stuart.sierra said: Added to 1.1.x branch, included in 1.1.0-RC2
Hide
Assembla Importer added a comment -

stuart.sierra said: That is, the temporary fix (not compiling logging.clj) is in 1.1.x and 1.1.0-RC2

Show
Assembla Importer added a comment - stuart.sierra said: That is, the temporary fix (not compiling logging.clj) is in 1.1.x and 1.1.0-RC2
Hide
Assembla Importer added a comment -

timothypratley said: The issue remains in clojure-contrib-1.1.0-master-20100111.160148-18.jar (do RC live elsewhere than http://build.clojure.org/snapshots ?)

Might it be possible to have the best of both worlds by having a dynamic binding of the macros? By this I mean the log macros can be rebound when the lib is used, so macro expansions after loading the lib will use the current environment. This would preclude AOT libs from using logging though, as the macro would be different in the lib compile environment and the user compile environment.

It seems the most robust solution would be to allow the choice of either fully dynamic log binding (might be useful for libs), or macro expanded logging... which would require two namespaces: clojure.contrib.logging and clojure.contrib.logging-dynamic or something like that.

Show
Assembla Importer added a comment - timothypratley said: The issue remains in clojure-contrib-1.1.0-master-20100111.160148-18.jar (do RC live elsewhere than http://build.clojure.org/snapshots ?) Might it be possible to have the best of both worlds by having a dynamic binding of the macros? By this I mean the log macros can be rebound when the lib is used, so macro expansions after loading the lib will use the current environment. This would preclude AOT libs from using logging though, as the macro would be different in the lib compile environment and the user compile environment. It seems the most robust solution would be to allow the choice of either fully dynamic log binding (might be useful for libs), or macro expanded logging... which would require two namespaces: clojure.contrib.logging and clojure.contrib.logging-dynamic or something like that.
Hide
Assembla Importer added a comment -

ataggart said: [file:asSL5Ea1qr34B3eJe5aVNr]: patch to handle aotc issues

Show
Assembla Importer added a comment - ataggart said: [file:asSL5Ea1qr34B3eJe5aVNr]: patch to handle aotc issues
Hide
Assembla Importer added a comment -

ataggart said: I've taken Matt's idea regarding delays and made a less invasive change.

The patch also contains some switches that can be set during compilation to obviate certain performance penalties introduced in order to handle AOTC.

See the note at the bottom of the ns docs for more info.

Show
Assembla Importer added a comment - ataggart said: I've taken Matt's idea regarding delays and made a less invasive change. The patch also contains some switches that can be set during compilation to obviate certain performance penalties introduced in order to handle AOTC. See the note at the bottom of the ns docs for more info.
Hide
Assembla Importer added a comment -

ataggart said: Ok, I clearly was working on this too late in the evening. There are some kinks to work out. Setting back to new until I have them ironed out.

Show
Assembla Importer added a comment - ataggart said: Ok, I clearly was working on this too late in the evening. There are some kinks to work out. Setting back to new until I have them ironed out.
Hide
Assembla Importer added a comment -

ataggart said: [file:b3wOBUbi0r37IeeJe5aVNr]: Final (I hope) patch attempt

Show
Assembla Importer added a comment - ataggart said: [file:b3wOBUbi0r37IeeJe5aVNr]: Final (I hope) patch attempt
Hide
Assembla Importer added a comment -

ataggart said: Wow, I'm an idiot.

The solution was simply to make the implementation-specific-def'ing macros (e.g., log4j-logging) into functions that call eval at runtime.

Sigh.

Show
Assembla Importer added a comment - ataggart said: Wow, I'm an idiot. The solution was simply to make the implementation-specific-def'ing macros (e.g., log4j-logging) into functions that call eval at runtime. Sigh.
Hide
Assembla Importer added a comment -

ataggart said: Not sure what "Next Release" means, but the latest patch (on Jan 18 15:59) can be pushed to master at anytime.

Show
Assembla Importer added a comment - ataggart said: Not sure what "Next Release" means, but the latest patch (on Jan 18 15:59) can be pushed to master at anytime.
Hide
Assembla Importer added a comment -

stuart.sierra said: Applied in commit 655060b3f265026ef3b45b44f5ab22d8897b3034

Show
Assembla Importer added a comment - stuart.sierra said: Applied in commit 655060b3f265026ef3b45b44f5ab22d8897b3034

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: