New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #150] Replace clojure-complete with compliment #153

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@bbatsov
Copy link
Contributor

bbatsov commented Jul 25, 2014

Turned out to be even easier than I thought, but one of the integration tests is failing and I'll need your help to fix it.

I've played with the completion is REPLy and it's working just fine; I have no idea why the test misbehaves.

@trptcolin

This comment has been minimized.

Copy link
Owner

trptcolin commented Jul 25, 2014

Ah, right. REPLy has to also work in situations where the nREPL is a remote classloader/process and does not have clojure-complete on the classpath (e.g. lein repl). That's why this test is failing, and what all this noise is responsible for doing: REPLy ships the completion library over the nREPL connection when it's not already available.

The nice thing about clojure-complete here is that it's just 1 file to ship across. With a bunch of namespaces in compliment, in order to integrate it I guess we'd have to ship every namespace over the nREPL connection.

Also, this bit of code wouldn't be necessary for compliment, since this resolve-class stuff is just monkey-patching clojure-complete.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Jul 25, 2014

Ah, right. REPLy has to also work in situations where the nREPL is a remote classloader/process and does not have clojure-complete on the classpath (e.g. lein repl). That's why this test is failing, and what all this noise is responsible for doing: REPLy ships the completion library over the nREPL connection when it's not already available.

I see. I had no idea why this was necessary.

The nice thing about clojure-complete here is that it's just 1 file to ship across. With a bunch of namespaces in compliment, in order to integrate it I guess we'd have to ship every namespace over the nREPL connection.

Guess that extracting this:

'~(try
               (formify-file
                 (-> (Thread/currentThread)
                     (.getContextClassLoader)
                     (.getResource "complete/core.clj")))
               (catch Exception e
                 '(throw (Exception. "Couldn't find complete/core.clj")))

And applying it all namespaces should be enough, right?

Also, this bit of code wouldn't be necessary for compliment, since this resolve-class stuff is just monkey-patching clojure-complete.

I'll remove it.

@trptcolin

This comment has been minimized.

Copy link
Owner

trptcolin commented Jul 25, 2014

Yep, that sounds right. It will probably be some kind of nasty expression-building because of the cross-process stuff, but should work.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Jul 25, 2014

@trptcolin Have a look at the updated code. Seems to me everything is being loaded (by inspecting the expressing passed to eval in a REPL), but the problem persists.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Jul 25, 2014

@trptcolin

This comment has been minimized.

Copy link
Owner

trptcolin commented Jul 25, 2014

The current issue looks like the files needing to be required in an order where the dependencies are satisfied when they try to load. (Locally I'm printing out the value of f# from here.)

Bumping utils, context, & sources up to the top appears to get things working for me locally, but I'm going to want to do a lot more testing before merging. The remote-process/classloader stuff is hairy to deal with, as you're seeing 😄

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Jul 25, 2014

Yeah, I imagined it might be a load order issue; should have pursued this train of thought. I'm glad you got things working and I guess I'll be leaving the PR in your more capable hands from here on. :-)

Thanks for taking the time to walk me though some of the code.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Jul 25, 2014

Is there a way to build the namespace dependency tree automatically? It doesn't sound like a good idea to hardcode the list of files from other project. Ideally some function should do that, or at least I can put a function into Compliment itself that would yield a list of Compliment's namespaces/files.

@trptcolin

This comment has been minimized.

Copy link
Owner

trptcolin commented Jul 25, 2014

Good point @alexander-yakushev

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Jul 25, 2014

OK, it seems that the way to go is for me to put this into compliment.core:

(def all-files
  "List of all Compliment files in the correct loading order. This is required
  by REPLy."
  (map (partial format "compliment/%s.clj")
       ["utils"
        "context"
        "sources"
        "sources/class_members"
        "sources/ns_mappings"
        "sources/namespaces_and_classes"
        "sources/keywords"
        "sources/special_forms"
        "core"]))

Anyway it would make more sense for me to maintain this list than for Colin. Thoughts?

@trptcolin

This comment has been minimized.

Copy link
Owner

trptcolin commented Jul 25, 2014

That'd be awesome, though it makes me sad for you to have to do that.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Jul 25, 2014

Well, any automatic solution is unreasonably complex. Besides new files aren't added that often. I'll do this first thing in the morning.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Jul 26, 2014

So, done alexander-yakushev/compliment@401aa20. Also I pushed it to Clojars as compliment 0.1.2.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Jul 26, 2014

I've updated the PR accordingly. Finally, everything seems to be working. :-)

Thanks, @alexander-yakushev!

@trptcolin

This comment has been minimized.

Copy link
Owner

trptcolin commented Aug 5, 2014

OK, I got a chance to try this out more thoroughly tonight, and so far there are a couple of things I've found that are holding me back from merging (see below). I can totally see myself getting addicted to the Java instance method completion - good stuff!

  1. Results are sorted starting with the shortest matches, which is sometimes what you want, but not always. I can imagine if the completions fill a single drop-down, this would be the preferred structure, but when using completion to discover APIs where the results have a lot of room to breathe like in the REPL, alphabetical sorting is nicer. It doesn't look like there's currently an option for this, but if there's interest, maybe there could be another arity of compliment.core/completions that takes a sort fn? Happy to PR that to compliment if there's interest, but someone else can probably get to it faster than I can right now.
  2. This one is also to do w/ discovering APIs: a prefix of java.util.concurrent. only has 1 completion, Callable, while clojure-complete gives everything under that package. I think this is by design: the class completion source only gives classes imported in the current ns. I'm not sure if there's an easy fix here.
@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Aug 5, 2014

It doesn't look like there's currently an option for this, but if there's interest, maybe there could be another arity of compliment.core/completions that takes a sort fn? Happy to PR that to compliment if there's interest, but someone else can probably get to it faster than I can right now.

Sounds like a good idea. You're right that the current sorting is optimized for drop-down completion.

This one is also to do w/ discovering APIs: a prefix of java.util.concurrent. only has 1 completion, Callable, while clojure-complete gives everything under that package.

Sounds like a bug as classes are always usable if referred to by a FQN. @alexander-yakushev is this intentional?

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Aug 5, 2014

This is not hard to do.

clojure-complete uses classpath parsing to recover the list of all available classes. Since Compliment was originally designed with Android compatibility in mind, I had to abnegate this approach. Now that I think of it, I can parse classpath if on JVM and just ignore it on Android. So let's say this is feasible too.

alexander-yakushev added a commit to alexander-yakushev/compliment that referenced this pull request Aug 5, 2014

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Oct 6, 2014

@trptcolin Guess you should test this with the new compliment 0.2-SNAPSHOT. The features you requested are now present there.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Oct 6, 2014

A little typo by @bbatsov, it is 0.2.0-SNAPSHOT. You are welcome:)

@bbatsov bbatsov force-pushed the bbatsov:compliment branch from 15527d8 to 8d75e65 Nov 1, 2014

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Nov 1, 2014

I've updated the PR.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Nov 1, 2014

I would suggest to wait until solid 0.2.0 is shipped. I need the confirmation for clojure-emacs/cider#549, and then after a week of no complaints I can release it.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Nov 1, 2014

@alexander-yakushev You'll also have to check what's wrong with compliment and reply now. 0.1.2 passed the test suite, but 0.2 is currently failing one test.

alexander-yakushev added a commit to alexander-yakushev/compliment that referenced this pull request Nov 1, 2014

alexander-yakushev added a commit to alexander-yakushev/compliment that referenced this pull request Nov 1, 2014

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Nov 12, 2014

@alexander-yakushev Guess you can release that 0.2 version.

@bbatsov bbatsov force-pushed the bbatsov:compliment branch from 8d75e65 to f432e93 Nov 12, 2014

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Nov 12, 2014

🎺 Released! 🎺

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Nov 12, 2014

💯 Great! I've just updated this PR. Hopefully it will get merged soon!

@trptcolin

This comment has been minimized.

Copy link
Owner

trptcolin commented Nov 22, 2014

Ack, yeah the failing tests on this PR (see Travis CI) look like they're due to the classfile size limit ("Invalid method Code length 77485"). An explanation from Chas here.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Nov 23, 2014

in class file reply/exports$eval908$fn__909

What does this class refer to, and how can we fix the error?

@trptcolin

This comment has been minimized.

Copy link
Owner

trptcolin commented Nov 23, 2014

I assume the spot calling formify-files will need to change somehow to send over each one of (compliment-files) at a time, so that they're not all rolled up into one eval in the catch Exception part of completion-code.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Feb 14, 2015

@alexander-yakushev Can you look into this? I'd love us to finally wrap this legendary change.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Feb 14, 2015

I remember I already did, and something prevented me from fixing it. I have to look again.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Feb 15, 2015

@bbatsov We have discussed it before, haven't we? All Compliment files together are too big to be loaded in one load call.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Feb 15, 2015

I assume the spot calling formify-files will need to change somehow to send over each one of (compliment-files) at a time, so that they're not all rolled up into one eval in the catch Exception part of completion-code.

@alexander-yakushev Yeah, that's what I mean when I asked if you can look into this. :-)

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Feb 15, 2015

Well, there's not much I can do on the Compliment side, is there?

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Mar 7, 2016

@bbatsov @trptcolin

It's zombie time! I was wondering what should we do with this PR. Just refreshing your memories so that you don't have to — we were stuck with the following problem.

  1. REPLy wants to be self-contained. This means to be able to connect with Leiningen+Reply to any project regardless to whether it has tooling libraries on the classpath.
  2. To achieve 1, REPLy grabs all source code of clojure-complete and evals it in one big load call.
  3. To accommodate for 2, I included a list of all Compliment files easily accessible for REPLy.
  4. clojure-complete is one small source file. Compliment grew... a little... large, at least too large to fit into one load. JVM says "no go".

So, what do we do, gents? Investigate possible solutions further, or call it a day and close for good?

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Mar 8, 2016

Yeah, 4 was the blocker. I obviously want to see the switch done, guess @trptcolin wouldn't mind it as well.

@holyjak

This comment has been minimized.

Copy link

holyjak commented Dec 1, 2017

I don't know the code but would it be so difficult to have one load per namespace?

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Dec 3, 2017

@jakubholynet Likely something like this would be possible. When I started working on this I just tried to emulate directly what was being done for core.complete.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Dec 3, 2017

As @trptcolin commented himself:

I assume the spot calling formify-files will need to change somehow to send over each one of (compliment-files) at a time, so that they're not all rolled up into one eval in the catch Exception part of completion-code.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Dec 3, 2017

I wonder if this asks for a more generic solution. Say, client-side cider-nrepl that can force-feed the server process with all the necessary code, even if the server started with bare nREPL. Too much?

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Dec 3, 2017

But what do you do when you just what to use REPLy by without CIDER's middleware?

Ah, right. REPLy has to also work in situations where the nREPL is a remote classloader/process and does not have clojure-complete on the classpath (e.g. lein repl). That's why this test is failing, and what all this noise is responsible for doing: REPLy ships the completion library over the nREPL connection when it's not already available.

Maybe really the simplest way would be to load the namespaces sequentially.

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Dec 3, 2017

But what do you do when you just what to use REPLy by without CIDER's middleware?

Do you mean without CIDER but still wanting a completion? Perhaps, cider-nrepl could be modular in what it can ship over network.

Maybe really the simplest way would be to load the namespaces sequentially.

It probably would be. And it would be nice if cider-nrepl had a standardized way of doing that, so that any middleware could be uploaded like that. Or, perhaps, not - I'm just thinking aloud, no particular usecase in mind.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Dec 3, 2017

That should probably an nREPL concern, though, right?

It would certainly be nice to be able to easily "enrich" an already running nREPL server. AFAIK this the main selling point of unrepl.

@tirkarthi

This comment has been minimized.

Copy link
Contributor

tirkarthi commented Feb 3, 2018

Sorry to bump this thread. clojure-complete has a bug that crashes leiningen on JDK 9. It has been fixed upstream but not pushed to Clojars. I tried to port clojure-complete and luckily I found this PR. Couple of points.

  1. compliment uses cond-> in https://github.com/alexander-yakushev/compliment/blob/master/src/compliment/sources/ns_mappings.clj#L91 which was introduced in Clojure 1.5 and reply runs with Clojure 1.4 so applying the latest compliment doesn't work.
  2. Since this migration seems like a big change that will also require changes in leiningen to remove clojure-complete being declared as a dependency though not directly used I think it will be better to merge this or to use the fixed clojure-complete release in the short term which could also help leiningen to upgrade.

I don't know if collaborators in clojure-complete (@trptcolin ) can push a release to clojars but it will be great if it can be done for the short term fix.

Thanks

@trptcolin trptcolin referenced this pull request Feb 4, 2018

Closed

Move to compliment #176

@bbatsov bbatsov referenced this pull request Mar 8, 2018

Open

[ci] Adding .travis.yml #26

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Mar 8, 2018

compliment uses cond-> in https://github.com/alexander-yakushev/compliment/blob/master/src/compliment/sources/ns_mappings.clj#L91 which was introduced in Clojure 1.5 and reply runs with Clojure 1.4 so applying the latest compliment doesn't work.

As by now Clojure 1.4 is dead and buried I don't think that bumping this would be a big problem for everyone. These days few libs aim to support anything older than 1.7.

Since this migration seems like a big change that will also require changes in leiningen to remove clojure-complete being declared as a dependency though not directly used I think it will be better to merge this or to use the fixed clojure-complete release in the short term which could also help leiningen to upgrade.

Well, I wouldn't call this that big of a change. End users probably won't even notice it.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Dec 6, 2018

Btw, there's a hope we'll wrap this at some point. After a short convo with @cgrand he made the astute observation that all we need to do to not the hit the JVM class limit is to evaluate smaller forms. We can either evaluate the forms in all the files one by one, which will certainly work, or try to eval each file as a single form, which will probably work, as none of the files are big. Basically the only reason why the PR's failing right now is that we're concatenating all files together and trying to evaluate them all at once.

I can't believe how we didn't figure this out back in the day, but better late than never. :-)

@alexander-yakushev

This comment has been minimized.

Copy link
Contributor

alexander-yakushev commented Dec 6, 2018

It doesn't look there is a trivial way to do this right now. The completion initializer assumes everything can be done in a single call through nREPL. A fixed version should somehow perform multiple calls, one per each file.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Dec 6, 2018

Yeah, that should certainly be changed. I don't see any particular reason for this having to be done in a single eval request.

Alternatively we can wait for @cgrand to port the sideloading to nREPL and then REPL-y would be able to simply inject compliment in this manner. That will probably be the preferred way to load libs from the clients down the road, so I guess we should aim in this direction.

@cgrand

This comment has been minimized.

Copy link

cgrand commented Dec 6, 2018

I've looked a bit more into it and while the idea is still to avoid having a big constant (string or form it doesn't matter; chunked or not it doesn't matter; it's the overall pr-dupped size of all constants that matter for the class size limit) it's a little bit more involved than what I previously thought (I missed the fact that the code I was shown was in syntax-quoted etc.).

But @bbatsov is right, in the end side loading is the answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment