conservatively inlining the most likely deps that could crash with project versions #48
Conversation
|
What's |
|
Well the following I guess: ;;[cheshire "5.2.0"]
;; [com.fasterxml.jackson.core/jackson-core "2.2.1"]
;; [com.fasterxml.jackson.dataformat/jackson-dataformat-smile "2.2.1"]
;; [tigris "0.1.1"]
;;[clj-stacktrace "0.2.7"]
[clojure-complete "0.2.3" :scope "test" :exclusions [[org.clojure/clojure]]]
[commons-io "2.4"]
[fs "1.3.3"]
[org.apache.commons/commons-compress "1.3"]
[ibdknox/analyzer "0.0.2"]
[org.clojure/clojure "1.5.1"]
[org.clojure/clojurescript "0.0-2202" :exclusions [[org.apache.ant/ant]]]
[com.google.javascript/closure-compiler "v20131014"]
[args4j "2.0.16"]
[com.google.code.findbugs/jsr305 "1.3.9"]
[com.google.guava/guava "15.0"]
[com.google.protobuf/protobuf-java "2.4.1"]
[org.json/json "20090211"]
[org.clojure/data.json "0.2.3"]
[org.clojure/google-closure-library "0.0-20140226-71326067"]
[org.clojure/google-closure-library-third-party "0.0-20140226-71326067"]
[org.mozilla/rhino "1.7R4"]
[org.clojure/tools.nrepl "0.2.3" :scope "test" :exclusions [[org.clojure/clojure]]] ;; overridden by lein to 0.2.6 for me
;;[org.clojure/tools.reader "0.9.2"] |
|
@rundis I'm trying to test this but it doesn't seem to be working. When I open an InstaREPL tab and enter an expression – Any ideas? Maybe I'm not deploying the plugin correctly. I pulled your branch into _\LightTable\deploy\plugins\Clojure_, ran |
|
sorry should have added instructions. Never used the build scripts myself, they might be broken. But in any case I didn't think of adding the necessary changes to them for this wip pull (: Firstly, the fix includes using mranderson : https://github.com/benedekfazekas/mranderson To test locally, go to the lein-light sub project in the clojure plugin
So the following needs to be addressed also before any merge and subsequent release I think:
The big drawback of using mranderson/source inlining is that there is considerable overhead (several seconds on my machine) firing up the nrepl. (Think its due to loading/compiling a whole bunch of more classes). Cider has the same issue btw. |
|
To be even clearer; Btw i think the bundling in the github project is confusing and would argue that the middleware lein-light could happily deserve a separate github proj with a travis build and deploy facility of it's own. Hey we could even start adding some tests even :-) Its fair that the runner and the clojurescript part stays bundled like todays, since that sets up the default version of lein-light that works with its corresponding clojurescript plugin. However when connecting to a running nrepl outside of lt we should be free to promote newer versions of the middleware (say regular snapshots etc) for people to try. |
|
@rundis Whew! Thanks for the info. I noticed that you hadn't modified the plugin per se but I didn't suspect that I could skip deploying the plugin as a whole. Nor did I suspect that the latest code just doesn't work. I'll follow your instructions to test this. I still can't tell why the plugin failed for me anyways. The code that's failing, because a core function is missing (or never existed), looks like it was copied from the main repo prior to the first tagged version of this plugin. I am confused and I'd like to understand how I screwed up. Also, the build.sh script doesn't compile the ClojureScript code for the plugin. I'm not sure how to best do that. @joshuafcole Any tips on how to compile the ClojureScript for this? Any reason why it shouldn't be part of the build script? Or a separate build script? |
|
I ran into an error running |
|
I opened an issue for Leiningen on the off chance that it's a bug with it. I'll try on some combination of my Ubuntu and Mac OS X boxes tonight if I can. |
|
I was able to run |
|
@rundis I was able to run
[I edited your comment above to change your bullets into checkboxes. I like the checkbox convention as an indication of tasks that we should complete, or explicitly 'delete' or strike, before we close an issue.] I still ran into the original issue, at least with my personal project. Light Table's lament: |
|
if following this steps doesn't seem to work, would you mind checking the jar file if it contains the inlined code for the the dependencies at all. Just looking at the size of the jar would be a hint, but you might want to do a quick scan to ensure that you find some tools.readers namespaces in the jar file |
|
@kenny-evitt did you get any further with trying this out ? |
|
@rundis I just tried to test this on an Ubuntu box.
But it seemed like the issue is still un-resolved, at least for my personal project. Light Table's complaint is below. Is there anything I need to do beyond the initial two commands? Should I close and re-open LT? Refresh the plugin list? Is trying to reproduce the original issue the right way to test this? Are your changes intended to fix some but not all of the (possibly) conflicting dependencies? |
|
@rundis I got the same error even after closing and re-opening LT. |
|
Arghh.. My bad with regards to LightTable/LightTable#1052 Starting a new clojure client from Light Table has two steps:
The problem is with 2, when the project classpath is set up target/classes obviously comes as one of the first entries in the classpath.When uberjaring the sample project in question has an old version of tools.reader left in target/classes. I thought that inlining tools.reader would resolve the problem, and for some issues with like the actual code in lein-light it will, but the problem kenny got above is from trying to compile the clojurescript dependency which also depends on tools.reader (0.8.3). The runner project tries to enforce tools.reader 0.8.3 when kicking off the nrepl process, but I believe to no avail in this case as target/classes wins. So what to do: in summary: someone please give me a dependency isolation story in clojure/jvm that actually works in a repl/nrepl scenario ! |
|
Unsurprisingly inlining the ClojureScript dependency was certainly a dead end. |
|
@rundis Thanks for the heroic effort. Maybe the previous iterations of this plus #43 are enough? In my case, I've never run into anything except the uberjar corner case. Also, a quick Google search for "clojure repl dependency isolation" included boot. It seems like they solve the same or similar problem by just not having any dependencies:
|
|
@rundis Thanks for your herculean efforts here. I agree with @kenny-evitt that some version of #43 (b) is plenty for the uberjar corner case. We'll address other corner cases as needed. As for this PR, do you have an example project that fails without these fixes? The guestbook example connects fine with the latest master (which has no fs). |
|
@cldwalker If I use the guestbook example I can reproduce the problem with master of the clojure plugin. The issue is with tools.reader not fs ! The [compojure "1.1.6"] dep in guestbook pulls in tools.reader 0.7.3. So when I do lein ring uberjar, tools.reader will be extracted (and aot'ed ?) into target/classes One version of #43 could of course be to try and nuke only classes from deps that overlap with deps we aren't inlining as part of our nrepl middleware |
|
@rundis Please excuse me if this is stupid, but what about uberjar-ing the runner? Is there some compelling reason why that's a bad idea? |
|
@kenny-evitt The runner is already uberjar'ed |
|
@rundis Ahhh – and it's lein-light-nrepl that's failing to be loaded because of the classpath non-isolation. How crazy would it be to remove all of the dependencies from lein-ligh-nrepl? We'd still run into problems because of Clojure/ClojureScript version mismatches even if we did that, right? |
|
@rundis I think the only failsafe means of isolation is what Leiningen does:
That seems like a major rewrite. |
|
Now that #52 has landed, guestbook should be fine. Is there a known test project this PR fixes? |
|
@cldwalker Well not that I can think of from the top of my head. I'd have to try and create one with a conflict for any of the remaining dependencies in the lein-light middleware. I think we can close this for now and rather reconsider introducing (an updated version of) something like this if we run into frequent issues related to dependency conflicts after we ship 0.8 |
|
@rundis Sounds good. Other options to consider next time are https://github.com/jonase/eastwood/tree/master/copy-deps-scripts or to read from the project.clj and give better errors |
First attempt at addressing #44
Took the liberty of upping tools.reader to a sensibly up to date version while at it.