search

clojure-emacs / orchard

A fertile ground for Clojure tooling
Eclipse Public License 1.0
326 stars 54 forks source link

Jump-to-definition with Boot jumps to temporary files #52

Closed alexander-yakushev closed 5 years ago

alexander-yakushev commented 5 years ago

When using the latest cider-nrepl with the latest Orchard, and after recompiling namespaces with tools.namespace, pressing M-. on a function name from one of the recompiled namespaces jumps not to the actual file, but to the "temporary" cache file that Boot creates in ~/.boot/tmp/....

Looks like the commit that broke it is https://github.com/clojure-emacs/orchard/commit/b83826f42604f5f9bf59e25a535ef9d3ddb3c5db

A bit more context: image

bbatsov commented 5 years ago

Thanks for reminding me of this!

Basically we need to decide whether to restore this right away (which was my preference) and cut 0.5, and then merge https://github.com/clojure-emacs/orchard/pull/37 and cut 0.6. I've been super busy the past couple of weeks and had almost no time for open-source, so I hope someone will beat me to fixing this.

bbatsov commented 5 years ago

Here's some extra context https://github.com/clojure-emacs/orchard/pull/37#issuecomment-491297455

bbatsov commented 5 years ago

@alexander-yakushev Is this fixed in the latest beta of orchard/cider-nrepl?

alexander-yakushev commented 5 years ago

It appears that the latest cider-nrepl SNAPSHOT with orchard beta4 hasn't been built yet. The one that I download from clojars still has beta3.

bbatsov commented 5 years ago

My bad. I just deployed the new release.

alexander-yakushev commented 5 years ago

Something is wrong. The JAR for cider-nrepl 0.22.0-beta2 doesn't contain any inlined deps. 0.22.0-SNAPSHOT hasn't updated since May 5th. Is this expected?

bbatsov commented 5 years ago

I've deployed with make deploy that applies MrAnderson first, but it seems something got messed up. //cc @benedekfazekas @arichiardi I think it's not the first time we've had some issues after the deployment unless I've run make clean before this. Probably something's wrong in the make target.

As for the snapshots - I've stopped updating those after beta1. There's no auto-deploy for snapshot since the move to CircleCI and pushing those manually after each PR is annoying, as I'm not always around a computer in the first place. There's definitely room for improvement here.

bbatsov commented 5 years ago

Actually it seems I might have used lein deploy accidentally. 😆 I'm on the road today, but I'll cut a new beta when I'm back home.

bbatsov commented 5 years ago

@alexander-yakushev I've cut 0.22.0-beta3.

alexander-yakushev commented 5 years ago

I've tried this version, and jump to definition unfortunately still doesn't work correctly (it jumps into temp files). Even worse – seems it now has nothing to do with tools.namespace and jumps into incorrect files even when I compile files manually with C-c C-k.

arichiardi commented 5 years ago

@alexander-yakushev I have tried here the C-c C-k and M-. and it was working here, do you have a project somewhere I can try out?

alexander-yakushev commented 5 years ago 
$ boot -d boot/new new -n myprj

Here're relevant parts of classpath:

/Users/alex/.m2/repository/org/clojure/clojure/1.10.0/clojure-1.10.0.jar
/Users/alex/.m2/repository/org/clojure/core.specs.alpha/0.2.44/core.specs.alpha-0.2.44.jar
/Users/alex/.m2/repository/boot/core/2.8.3/core-2.8.3.jar
/Users/alex/.m2/repository/boot/pod/2.8.3/pod-2.8.3.jar
/Users/alex/.m2/repository/org/tcrawley/dynapath/1.0.0/dynapath-1.0.0.jar
/Users/alex/.m2/repository/org/projectodd/shimdandy/shimdandy-impl/1.2.1/shimdandy-impl-1.2.1.jar
/Users/alex/.m2/repository/org/clojure/spec.alpha/0.2.176/spec.alpha-0.2.176.jar
/Users/alex/.boot/cache/tmp/private/tmp/myprj/162x/-z78xm5
/Users/alex/.boot/cache/tmp/private/tmp/myprj/162x/ir9bl8
/Users/alex/.boot/cache/tmp/private/tmp/myprj/162x/yy2bwd
/Users/alex/.boot/cache/tmp/private/tmp/myprj/162x/439q3o
/Users/alex/.boot/cache/tmp/private/tmp/myprj/162x/yhxp15
/Users/alex/.m2/repository/cider/cider-nrepl/0.22.0-beta3/cider-nrepl-0.22.0-beta3.jar
/Users/alex/.m2/repository/refactor-nrepl/refactor-nrepl/2.4.0/refactor-nrepl-2.4.0.jar
/Users/alex/.m2/repository/nrepl/nrepl/0.6.0/nrepl-0.6.0.jar
/Users/alex/.boot/cache/bin/2.8.3/boot.jar
arichiardi commented 5 years ago

Cool thanks will let you know

arichiardi commented 5 years ago

@alexander-yakushev I have this file here:

(ns myprj.bar
  (:require [myprj.core :as core]))

(core/foo "Andrea")

and I can reproduce.

bbatsov commented 5 years ago

What's the cause? I assume we simply misplaced the code that deals with the Boot classpath, right?

arichiardi commented 5 years ago

I don't know I am debugging

arichiardi commented 5 years ago

So it seems that, as of today, the :file key of what info is already good while cider-nrepl is trying to post process it again:

{:ns myprj.core, :name foo, :doc I don't do a whole lot., :file myprj/core.clj, :arglists ([x]), :line 3, :column 1}

I don't know what changed in cider-nrepl before my PR got merged and I will need to investigate...but it seems that we don't need to post process info results anymore.

arichiardi commented 5 years ago

Uhm..the info op definitely returns the wrong thing:

(<--
  id           "19"
  session      "72ffd0c6-55a7-4549-a94f-f3cc4c188913"
  time-stamp   "2019-06-06 20:38:08.093006094"
  arglists-str "[x]"
  column       1
  doc          "I don't do a whole lot."
  file         "file:/home/arichiardi/.boot/cache/tmp/home/arichiardi/tmp/my..."
  line         3
  name         "foo"
  ns           "myprj.core"
  resource     "myprj/core.clj"
  status       ("done")
)
arichiardi commented 5 years ago

Solved it, pushing PRs to both projects and kind of having one question here:

I see we have these two guys here in format-response:

https://github.com/clojure-emacs/cider-nrepl/blob/master/src/cider/nrepl/middleware/info.clj#L42

https://github.com/clojure-emacs/cider-nrepl/blob/master/src/cider/nrepl/middleware/info.clj#L44

Do they belong there or in the orchard?

Are they "presentation only"? Because the first one actually does resource resolution and in my opinion should probably be pulled down in the orchard...or maybe not...depending on how we see it :smile:

bbatsov commented 5 years ago

Are they "presentation only"? Because the first one actually does resource resolution and in my opinion should probably be pulled down in the orchard...or maybe not...depending on how we see it 😄

The first conversions are presentation-only (before the two you mentioned). I remember this was needed back in the day to make sure we don't try to encode something non-encodable, although we'll have revisit those at some point. As for the final two - they seem to add new info to the response, so they probably don't belong in this function. Likely some design oversight.

arichiardi commented 5 years ago

Cool I had the same thought so I will definitely bring them back to where they belong. Our tree sanctuary 😅

arichiardi commented 5 years ago

I've deployed with make deploy that applies MrAnderson first, but it seems something got messed up. //cc @benedekfazekas @arichiardi I think it's not the first time we've had some issues after the deployment unless I've run make clean before this. Probably something's wrong in the make target.

As far I understand you always need to make clean because the .inline-deps file absence is the one that triggers Mr.Anderson.

Having said that it never failed me with make clean install