Use revamped enrich-classpath plugin #3364
Conversation
|
Might be good to also mention some of things you've written in the description of the PR in the docs. |
|
Ready. I've kept QAing this and adding resilience. Doc got some updates but I'm also leaving a bit minimalistic. A follow-up PR will add Clojure CLI compat - that work is done already. |
|
Moving this to draft again because I'll want to also QA this for figwheel repls. (I had for shadow-cljs repls only, as far as cljs is concerned) Otherwise the PR is reviewable. |
|
|
||
| For Lein users, it has the additional benefit of running a single JVM, instead of the two JVMs that Lein programs typically involve. | ||
|
|
||
| `enrich-classpath` is still in beta and defaults to being disabled. |
There was a problem hiding this comment.
You might want to slap a NOTE: before this.
|
|
||
| You can enable it by setting the `cider-enrich-classpath` defcustom to `t`. | ||
|
|
||
| With it enable, `cider-jack-in` will activate enrich-classpath, given the following conditions: |
|
|
||
| * You are on macOS/Linux | ||
| * You are on Leiningen | ||
| * You are launching a vanilla JVM repl (and not a cljs, or clj+cljs one) |
There was a problem hiding this comment.
Can you launch a mixed REPL? In general I find the 3rd bullet point a bit confusing
.circleci/config.yml
Outdated
| @@ -22,7 +22,7 @@ commands: | |||
| name: Install Emacs latest | |||
| command: | | |||
| echo "HOMEBREW_NO_AUTO_UPDATE=1" >> $BASH_ENV | |||
| brew install homebrew/cask/emacs | |||
| brew install homebrew/cask/emacs || brew install homebrew/cask/emacs || brew install homebrew/cask/emacs | |||
There was a problem hiding this comment.
Why do we need to retry this? I haven't noticed it failing often.
cider.el
Outdated
| @@ -855,7 +855,7 @@ middleware and dependencies." | |||
| (cider-jack-in-normalized-lein-plugins) | |||
| (if cider-enrich-classpath | |||
| (append cider-jack-in-lein-middlewares | |||
| '("cider.enrich-classpath/middleware")) | |||
| '("cider.enrich-classpath.plugin-v2/middleware")) | |||
There was a problem hiding this comment.
Note to self: avoid this when the intended repl type is not 'clj.
Not that it's problematic, but seems a sensible way to start.
There was a problem hiding this comment.
(Mostly a note to self)
For Lein users, it would be important to evaluate the Lein :init code.
Perhaps CIDER can make this happen, e.g. we determine the final :init value (after applying profiles, plugins, etc), persist it or store it in memory, and then we evaluate CIDER-side it as the first thing that happens after a successful (and enriched, i.e. no fallback was applied) connection.
edit: maybe it simply can be a clojure.main arg here https://github.com/clojure-emacs/enrich-classpath/blob/585f80f12b610b2438492e7523582796c5c1ee03/lein-plugin/src/cider/enrich_classpath/plugin_v2.clj#L90
|
@bbatsov: this is starting to look great, however I plan to merge it after the 1.8.0 release to contain risks. This way, users can choose to use 1.8.0 or master. |
There was a problem hiding this comment.
I've been refining the Clojure CLI integration (both in https://github.com/clojure-emacs/enrich-classpath itself and the clojure.sh wrapper script, now included in the PR).
I'm more confident about it now and have successfully tried with CIDER in large projects.
cider.el
Outdated
| (lambda () | ||
| (when (symbol-file 'cider-jack-in-command) | ||
| (concat (file-name-directory (symbol-file 'cider-jack-in-command)) | ||
| "clojure.sh"))) |
There was a problem hiding this comment.
I intend to ship the clojure.sh wrapper into the artifact to be unzipped into ~/.emacs.d/elpa/cider[...]
symbol-file would help me find that.
SGTY? @bbatsov
There was a problem hiding this comment.
Bundled with what - CIDER or enrich?
There was a problem hiding this comment.
Ah, I see it's bundled with CIDER. I'm not sure what symbol-file does, but there's package-user-dir or something like this that specifies the path to ELPA, so it seems you can use it instead.
There was a problem hiding this comment.
I just want to mention that package-user-dir seems to only be used by package.el. So if you are using something like straigth.el to manage your Emacs packages (like I do), my CIDER files won't be in the directory contained in package-user-dir. E.g., package-user-dir in my case contains "~/.emacs.d/elpa", but my packages are at ~/.emacs.d/straight-emacs-29/straight/build/ (for Emacs 29.1; I keep a separate package directory per Emacs version).
(symbol-file 'cider-jack-in-command) on the other hand gives me "/home/iarenaza/.emacs.d/straight-emacs-29/straight/build/cider/cider.elc" which seems about right 😄
There was a problem hiding this comment.
Agree with @iarenaza to not use package-user-dir (personally I use borg as package manager).
I think using locate-library is pretty common and I saw it a few times where the packages needs to find the sources for compiling a native module like vterm or jinx.
For cider this would be something like:
(file-name-directory (locate-library "cider.el" t))
There was a problem hiding this comment.
My bad, I keep forgetting not everyone is using ELPA. :-) I agree that locate-library is probably the best approach (mostly because it's obvious what you're trying to do with it).
|
Seems the windows integration tests are failing all the time now. Perhaps there's some real problem with them? |
cider.el
Outdated
| "clojure.sh"))) | ||
| "The location of enrich-classpath's clojure.sh wrapper script." | ||
| :package-version '(cider . "1.8.0") | ||
| :type 'function) |
There was a problem hiding this comment.
Seems weird that something that looks like a filename should be a function IMO.
cider.el
Outdated
| ('clojure-cli (cider--resolve-command cider-clojure-cli-command)) | ||
| ('clojure-cli (if (and cider-enrich-classpath | ||
| (not (eq system-type 'windows-nt)) | ||
| (executable-find (funcall cider-enrich-classpath-clojure-cli-script))) |
There was a problem hiding this comment.
Here it also looks that this is a function. Perhaps you can just have a function that just expands properly whatever is in the defcustom (e.g. tries to find whatever was specified by the user and falls back to clojure.sh)?
There was a problem hiding this comment.
I tentatively left this as a simple defun, and with a clearer name. Users would rarely have the need to point elsewhere for the script.
One can always edit the script inline, create a symlink, etc.
Probably not. In Circle they're passing as usual, while GHA's jobs have been more flaky. Maybe their Win servers were having a slow day. |
Its new functioning is described in clojure-emacs/enrich-classpath#23
|
Looks OK to me in the current state. I guess we'll squash this down to 1 commit, right? |
|
Thanks! I'm fixing something in enrich itself, will squash afterwards. |
|
clojure.sh will be melpa-distributed after melpa/melpa#8706 is in |
It now is in. @iarenaza, @dakra, given that you had the generosity to review, you might also want to give it a spin? Doc lives at https://docs.cider.mx/cider/config/basic_config.html#use-enrich-classpath Cheers - V |
|
Am I right in assuming that I need |
|
@iarenaza Yeah, you need to be on |
|
Be advised, I already got quite some feedback, nothing critical, but enough for not bothering more people (yet). Either way, CIDER 1.7.0 -> 1.8.0 has lots of reliability improvements (changelog), so feedback for all those other areas would be most useful. |
The
/Users/vemv/.mx.cider/enrich-classpath/1601/2516905847/1773208541.jar:/Users/vemv/.mx.cider/unzipped-jdk-sources/1601:/Library/Java/JavaVirtualMachines/adoptopenjdk-16.jdk/Contents/Home/lib/src.zippart indicates what Enrich is adding.Because those artifacts are appended at the tail of the classpath (as guaranteed by using a bespoke
javacommand, as opposed to delegating classpath construction to Lein), they won't give problems.Under this code path, Lein users now run just one JVM!