Skip to content
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

Changing cider-injected-middleware-version will not propagate to 'cider-jack-in-lein-plugins without emacs restart #3133

Closed
dgtized opened this issue Jan 17, 2022 · 7 comments

Comments

@dgtized
Copy link
Contributor

dgtized commented Jan 17, 2022

Expected behavior

(setq cider-injected-middleware-version "0.27.4")

Should update the cider-jack-in* command to use version 0.27.4 of cider/cider-nrepl.

;;  Startup: /usr/local/bin/clojure -Sdeps '{:deps {nrepl/nrepl {:mvn/version "0.9.0"} cider/piggieback {:mvn/version "0.5.2"} refactor-nrepl/refactor-nrepl {:mvn/version "3.2.0"} cider/cider-nrepl {:mvn/version "0.27.4"}} :aliases {:cider/nrepl {:main-opts ["-m" "nrepl.cmdline" "--middleware" "[refactor-nrepl.middleware/wrap-refactor,cider.nrepl/cider-middleware,cider.piggieback/wrap-cljs-repl]"]}}}' -Mdev:cider/nrepl

Actual behavior

cider/cider-nrepl version in generated commandline is unchanged and still "0.28.0". IE:

;;  Startup: /usr/local/bin/clojure -Sdeps '{:deps {nrepl/nrepl {:mvn/version "0.9.0"} cider/piggieback {:mvn/version "0.5.2"} refactor-nrepl/refactor-nrepl {:mvn/version "3.2.0"} cider/cider-nrepl {:mvn/version "0.28.0"}} :aliases {:cider/nrepl {:main-opts ["-m" "nrepl.cmdline" "--middleware" "[refactor-nrepl.middleware/wrap-refactor,cider.nrepl/cider-middleware,cider.piggieback/wrap-cljs-repl]"]}}}' -Mdev:cider/nrepl

However, if the setq is called prior to cider require, then the value is updated to 0.27.4.

Steps to reproduce the problem

Use the setq above to update cider-injected-middleware at runtime, and then run cider-jack-in-clj or cider-jack-in-cljs. The injected plugins will stay 0.28.0.

Workaround Fix

The problem appears to be that 'cider-jack-in-lein-plugins is updated at top-level during require from the current value of cider-injected-middleware-version. This happens at

cider/cider.el

Lines 467 to 468 in 8bb6717

(cider-add-to-alist 'cider-jack-in-lein-plugins
"cider/cider-nrepl" cider-injected-middleware-version)
. So executing:

(cider-add-to-alist 'cider-jack-in-lein-plugins "cider/cider-nrepl" "0.27.4")

Is sufficient to update the injected plugin.

It's not entirely clear to me what the best fix is. Initially I thought it would be easiest to ensure the top level call to cider-jack-in-lein-plugins is calculated as late as possible, but it's used in cider-jack-in-normalized-lein-plugins, cider-clojure-cli-jack-in-dependencies, cider-shadow-cljs-jack-in-dependencies, cider-jack-in-cljs and cider-jack-in-clj&cljs at minimum, so it appears to have a pretty wide surface area.

It's also not clear that the cider-add-to-alist behavior should be disabled, if 'cider-jack-in-lein-plugins is directly mutated it seems like it should override the value, and I suspect there is .dir-locals or package setup code in the wild doing that.

However, it is still confusing to have a specific defcustom that is ignored unless it's at the first require. It breaks the published docs and API, but I think the easiest fix might be to remove the defcustom and document mutating the plugins list directly. Otherwise I think it might require a careful rewrite of the access to cider-jack-in-lein-plugins to ensure it can calculate the version at runtime.

I also appreciate this is a very edge case error, so maybe it's best just to improve the documentation on it and leave it be for now.

Environment & Version information

CIDER version information

;; CIDER 1.3.0-snapshot (package: 20220113.610), nREPL 0.9.0
;; Clojure 1.10.3, Java 1.8.0_312

Emacs version

GNU Emacs 29.0.50 (build 14, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 1.16.0) of 2022-01-16

Operating system

Ubuntu 20.04

@bbatsov
Copy link
Member

bbatsov commented Jan 17, 2022

I think we should simply replace the direct usage of the var cider-jack-in-lein-plugins with a function can computes this dynamically. Updating the configuration at the top-level is a classic anti-pattern, but it didn't really cause much problems over the years and there was little incentive to fix this.

@vemv
Copy link
Member

vemv commented Jan 18, 2022

A straightforward change might be a breaking one.

Which might be fine is done carefully.

Would be also a good chance to use a more veridic name since cider-jack-in-lein-plugins is used incider-clojure-cli-jack-in-dependencies i.e. it's not exclusive to Lein concerns.

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2022

I've tweaked this minimally for now, but at some point I'll just reap all of this code as it's just ridiculous how cider-jack-in-lein-plugins is being used left and right.

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2022

On second thought - it wasn't particularly hard to decouple cider-jack-in-lein-plugins from clojure-cli and boot and I just did it right away. It does change a bit the jack-in API, but users shouldn't be using any of those functions directly, so they won't notice the changes. Technically speaking none of them should have been public to begin with.

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2022

Hmm, actually this will likely break clj-refactor.el, but there's no way to have a sane logic and keep the backward compatibility here. It was there were this insanity started, presumably to separate a "real" dep from a plugin type of dep, but it seems to me that
it should simply be updated to add its dep somewhere else (e.g. the regular deps).

@vemv
Copy link
Member

vemv commented Feb 13, 2022

Not sure I follow, Lein plugins aren't just dependencies, they have middleware so they surely deserve a separate type?

At this point I'm fine with breakage, but these requirements seem sensible:

  • 3rd party .el libs can specify deps to be added
    • this can be 'abstract'
  • 3rd party .el libs can specify plugins to be added
    • This is lein-specific
  • enrich-classpath can be disabled via a simple (setq cider-enrich-classpath nil)

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2022

I meant I removed the usage of cider-jack-in-lein-plugins for non-Lein projects. For Lein everything is as before, but for for Clojure CLI and Boot it seems that refactor-nrepl was being added as a Lein plugin with the assumptions that they would concat any plugins to the list of ordinary cider-jack-in-dependencies. This opens up the question how a tool like refactor-nrepl can add some dependency in a way where it's not duplicated for Leiningen, but that's not a big deal.

bbatsov added a commit that referenced this issue Feb 13, 2022
Basically, we extend the fix for cider-injected-middleware-version and
encapsulate the required deps nREPL and cider-nrepl in a reusable function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants