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

Fix dependency injection at jack-in time #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

futuro
Copy link
Contributor

@futuro futuro commented Mar 30, 2022

Commit clojure-emacs/cider@8989f40d in cider
stopped using cider-jack-in-lein-plugins as a method for defining
the dependencies to insert, which causes the sayid dependencies to be
missed upon jack-in.

Since we're using Cider anyways, and likely using it to define our
nREPL version, we're going to leverage the more generic
cider-jack-in-dependencies variable for defining the sayid
dependency.

To avoid potential version collisions, I've opted to no longer add the
sayid plugin to the cider-jack-in-lein-plugins variable.

Commit clojure-emacs/cider@8989f40d in cider
stopped using `cider-jack-in-lein-plugins` as a method for defining
the dependencies to insert, which causes the sayid dependencies to be
missed upon jack-in.

Since we're using Cider anyways, and likely using it to define our
nREPL version, we're going to leverage the more generic
`cider-jack-in-dependencies` variable for defining the sayid
dependency.

To avoid potential version collisions, I've opted to no longer add the
sayid plugin to the `cider-jack-in-lein-plugins` variable.
@bbatsov
Copy link
Member

bbatsov commented Mar 31, 2022

The problem is that there is a Lein plugin as well https://github.com/clojure-emacs/sayid/blob/master/src/sayid/plugin.clj

That's one of the messed up things about the deps injection right now - how to avoid duplicating deps for Lein projects. Probably in CIDER we should remove stuff from deps if they already appear in the plugins. For said it seems it'd be best if we add sayid to both the plugins and the deps.

@vemv
Copy link
Member

vemv commented Apr 14, 2022

Perhaps we can take the same exact approach as https://github.com/clojure-emacs/clj-refactor.el/blob/f368c56c83843396b160440f472a661a3b639862/clj-refactor.el#L4199-L4218, for homogeneity; that one implementation is known to work by now

@futuro
Copy link
Contributor Author

futuro commented Sep 13, 2022

@bbatsov would @vemv's suggestion work for sayid? (I'd totally forgotten about this issue until today when I deleted my cached package directory, also getting rid of my local copy of these changes and then wondering why sayid stopped working.)

@bbatsov
Copy link
Member

bbatsov commented Sep 14, 2022

The implementation in clj-refactor seems wrong to me, as it relies on what's the preferred build tool, instead of the actual build tool being used at jack-in time. I still think it'd be best if this was handled in a generic manner in CIDER itself by adding some extra logic for Leiningen there which strips deps that also appear as plugins. There's very little point in asking each plugin to do exactly the same thing.

@bbatsov
Copy link
Member

bbatsov commented Sep 14, 2022

For a short-term fix - here probably the dep needs to be added to both lists, instead of only to one. Kind of ugly, but it won't break anything IMO.

@futuro
Copy link
Contributor Author

futuro commented Sep 15, 2022

The implementation in clj-refactor seems wrong to me, as it relies on what's the preferred build tool, instead of the actual build tool being used at jack-in time. I still think it'd be best if this was handled in a generic manner in CIDER itself by adding some extra logic for Leiningen there which strips deps that also appear as plugins. There's very little point in asking each plugin to do exactly the same thing.

That's a good point about moving this solution to CIDER itself. I can take a crack at making that change, but I'm not familiar enough with lein to know whether to prefer deps or plugins. It sounds like the thing to do is remove any dep from cider-jack-in-dependencies that also shows up in cider-jack-in-lein-plugins; is that all that needs to happen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants