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

script to inject enrich-classpath for deps.edn projects #8

Closed

Conversation

wandersoncferreira
Copy link

@wandersoncferreira wandersoncferreira commented Sep 7, 2021

Proposal to fix #2 .

A problem: cider-jack-in adds middleware on-the-fly using -Sdeps EDN Deps data to use as the last deps file to be merged however -Scp does not work together with -Sdeps.

I need to extend this code to understand the -Sdeps option and include it in the classpath too, I am pushing this code for review anyway.

@vemv
Copy link
Member

vemv commented Sep 7, 2021

Looking promising! Thanks for this one, receiving a strong contribution feels like magic.

A problem: cider-jack-in adds middleware on-the-fly using -Sdeps EDN Deps data to use as the last deps file to be merged however -Scp does not work together with -Sdeps.

Got it. It's worth noting that enrich-classpath, being a clojure-emacs project, can certainly justify a modification in the cider-jack-in implementation.

I think that a non-obstrusive first step would be to make cider-jack-in use -Scp instead of -Sdeps (without enrich-classpath being involved). In the end -Scp is a "superset" of -Sdeps, sounds correct?

(also please link to the cider-jack-in code in question)

@wandersoncferreira
Copy link
Author

wandersoncferreira commented Sep 7, 2021

I think that a non-obstrusive first step would be to make cider-jack-in use -Scp instead of -Sdeps (without enrich-classpath being involved). In the end -Scp is a "superset" of -Sdeps, sounds correct?

I don't think so, we are doing a somehow convoluted thing to make this work:

bash process -> becomes a clojure process -> calls shell/sh to run a clojure process ^^

The thing is that inside our script we are using -Scp and seems like this flag do not work together with any other and something like clojure -Scp <our-enriched-classpath> -Scp <cider classpath> should not work from my interpretation of the help docs -Scp CP Do NOT compute or cache classpath, use this one instead.

A more descriptive picture of this issue is this:

  1. Call cider-jack-in in Emacs
  2. Emacs will launch

[nREPL] Starting server via /Users/wferreir/clojure-deps -Sdeps '{:deps {nrepl/nrepl {:mvn/version "0.9.0-beta1"} refactor-nrepl/refactor-nrepl {:mvn/version "2.5.1"} cider/cider-nrepl {:mvn/version "0.26.0"}} :aliases {:cider/nrepl {:main-opts ["-m" "nrepl.cmdline" "--middleware" "[cider.nrepl/cider-middleware]"]}}}' -M:cider/nrepl

  1. Our bash script clojure-deps will receive -Sdeps options and all the rest...
  2. In the end of our code we say "use this classpath and this classpath only" and then we emit

clojure -Scp <enriched-classpath> -Sdeps '{:deps {nrepl/nrepl {:mvn/version "0.9.0-beta1"} refactor-nrepl/refactor-nrepl {:mvn/version "2.5.1"} cider/cider-nrepl {:mvn/version "0.26.0"}} :aliases {:cider/nrepl {:main-opts ["-m" "nrepl.cmdline" "--middleware" "[cider.nrepl/cider-middleware]"]}}}' -M:cider/nrepl

which should ignore all the -Sdeps options afaict. Seems to me that the best (only?) option would be to handle -Sdeps inside our bash script to include these libraries provided in our -Scp

leiningen-deps-structure
->lein-project-map
enrich/middleware)
enriched-classpath (string/join ":" (classpath/get-classpath project))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While probably it's useful at a POC stage, leiningen-core seems a dubious dependency to have.

Arguably deps.edn users can have (or even want) a Lein-free, fast-starting environment. Large dependencies add startup time.

In the end building a classpath is relatively trivial. Might warrant changes/additions to enrich-classpath's API though.

...in particular, I'm thinking of making enrich-classpath optionally return only the .jars to be added instead of appending them to an existing presumed project.

@vemv
Copy link
Member

vemv commented Sep 7, 2021

which should ignore all the -Sdeps options afaict. Seems to me that the best (only?) option would be to handle -Sdeps inside our bash script to include these libraries provided in our -Scp

Yes we're on the same track, we both want to stop using -Sdeps, consolidating all things to be added in a single emitted -Scp.

At this stage that seems OK to do in the proposed script. Maybe for the 'final product' we'll need to move some (all?) logic to Elisp. Not sure though, I'm not familiar with cider-jack-in internals.

@vemv
Copy link
Member

vemv commented Sep 7, 2021

At this stage that seems OK to do in the proposed script. Maybe for the 'final product' we'll need to move some (all?) logic to Elisp

Scratch that, the logic can't reside 100% in Elisp as there are editors other than Emacs 😇

Still, there's a point that stands: we shouldn't necessarily work around cider-jack-in's impl details. We can always propose a change upstream if necessary, keeping things simple for both parts.

vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
@vemv vemv mentioned this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 14, 2022
vemv added a commit that referenced this pull request Jan 15, 2022
vemv added a commit that referenced this pull request Jan 15, 2022
vemv added a commit that referenced this pull request Jan 15, 2022
vemv added a commit that referenced this pull request Jan 15, 2022
vemv added a commit that referenced this pull request Jan 15, 2022
vemv added a commit that referenced this pull request Jan 15, 2022
@vemv vemv closed this in #14 Jan 15, 2022
vemv added a commit that referenced this pull request Jan 15, 2022
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.

deps.edn story
2 participants