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

Clojure CLI tools - optional jack-in dependencies and params #2922

Closed
practicalli-johnny opened this issue Oct 25, 2020 · 5 comments
Closed

Comments

@practicalli-johnny
Copy link
Contributor

practicalli-johnny commented Oct 25, 2020

Evolving the Cider support for Clojure CLI tools by providing a means to switch off the auto-injected dependencies.

Using Clojure CLI tools, the auto-injected dependencies and parameters can be replaced by a single alias (or combined aliases), simplifying the command line. This also allows for greater flexibility in how cider is run, whilst still providing the highly convenient jack-in experience.

Related to #2916

This is not meant to be a final design, but the next useful change without breaking the existing experience.

Use cases

  1. Additional command line options
    Add an alias to include extra paths and/or libraries to the Clojure repl (eg. :env/test which adds the :extra-paths ["test"] value so that the Cider test runner can find the unit test code in a project (without it having to be added to the main paths and ending up with test code in production).
    The latest release of Cider already allows such an alias to be injected correctly along with the standard Clojure libraries and parameters

  2. Custom command line options
    Use custom command line options that replace the auto-injected dependencies and params

Set cider-clojure-cli-global-options (EDIT: this variable has been replaced by cider-clojure-cli-aliases) value via .dir-locals.el and use only that value as an argument to the clojure command

The cider-clojure-cli-global-options would provide an alias that included the relevant nrepl, nrepl-refator & cider-nrepl libraries.

Using the universal argument, this would also present a simplified command line, avoiding the need to delete the injected cider options

Suggested Code change
Update the cider-inject-jack-in-dependencies function to include a check of the new variable called cider-clojure-cli-jack-in-options, which should be set to true by default to retain the current behaviour of Cider.

Create a defcustom called cider-clojure-cli-jack-in-options and set to true by default.

If cider-clojure-cli-jack-in-options is set to nil then only the value of cider-clojure-cli-global-options is used with the clojure command

Code change from line 635 of cider.el

('clojure-cli
     (if cider-clojure-cli-jack-in-options
         (cider-clojure-cli-jack-in-dependencies
          global-opts
          params
          (cider-add-clojure-dependencies-maybe
           cider-jack-in-dependencies))
       global-opts))

A video of my experiments around this issue is here: https://youtu.be/XuquYgOSOnc?t=6879 (skipped to the conclusion as its about 2 hrs long)

@bbatsov if this approach seems sensible, I'll raise a PR with this change. Happy to discuss further if you wish.

@practicalli-johnny
Copy link
Contributor Author

From the previous discussion, I did consider the implications of merging the global opts, params and even dependencies for Clojure CLI tools. However, this always seemed to lead to lots of potential conflicts with provided global-ops and the Cider auto-injected configuration.

As the command line is very flexible for Clojure CLI tools, it would seem that some kind of parsing would be required of any supplied global-opts to ensure a main namespace was not already included.

The parsing would need to check the project deps.edn file as well as the user level config in ~/.clojure/deps.edn, to extract the details of the alias and see if it included an main namespace. The -M flag may include just dependencies or it may also include a main namespace.

I am assuming the new -X option wouldn't be used with cider, unless nrepl changes to use Clojure exec rather than Clojure main.

@practicalli-johnny
Copy link
Contributor Author

@bbatsov would adding a new defcustom cider-clojure-cli-jack-in-options and updating cider-inject-jack-in-dependencies with a check be an acceptable enhancement to the way Clojure currently used Clojure CLI tools alases?

If so, I will create a pull request with this change.
Thank you.

@dpsutton
Copy link
Contributor

dpsutton commented Nov 12, 2020

at this point i wonder if the best option might be a var that the user can set which contains the whole jack in string? maybe its just far easier to have a self-contained "don't resolve anything, add middleware, whatever. just start a process with this and connect to it" type var.

;; .dir-locals.el
((nil . ((cider-full-jack-in-command . "clojure -X whatever"))

@practicalli-johnny
Copy link
Contributor Author

practicalli-johnny commented Nov 13, 2020

So the concept of adding a var seems agreed, just different scope of detail for that var.

In the slack discussion you mention a single string called cmd is created to start the process, in the function cider--update-jack-in-cmd and just need to skip the code that normally creates the jack-in command with its dependencies.

https://github.com/clojure-emacs/cider/blob/master/cider.el#L1279-L1285 is where the string is built up and here the string is just passed into nrepl-start-server-process https://github.com/clojure-emacs/cider/blob/master/cider.el#L1007-L1011
cider.el:1007-1011

I tried wrapping an if statement around the call (cider--update-jack-in-cmd), which builds the full jack-in command, at https://github.com/clojure-emacs/cider/blob/master/cider.el#L1006

 (let ((params (thread-first params
                  (cider--update-project-dir)
                  (cider--check-existing-session)
                  (if cider-full-jack-in-command
                     :jack-in-cmd cider-full-jack-in-command
                    (cider--update-jack-in-cmd))))

That code generates

error in process sentinel: Could not start nREPL server:

So I probably have the :jack-in-cmd cider-full-jack-in-command wrong...
Or is it because I'm using an if in a thread-first macro?
Yes, something different about thread-first in elisp, as this code doesnt work either

    (let ((params (thread-first params
                  (cider--update-project-dir)
                  (cider--check-existing-session)
                  (plist-put :jack-in-cmd cider-custom-jack-in-command))))
    (print cider-custom-jack-in-command)
    (print params)
    (nrepl-start-server-process
     (plist-get params :project-dir)
     (plist-get params :jack-in-cmd)
     (lambda (server-buffer)
       (cider-connect-sibling-clj params server-buffer))))

seems thread-first doesnt put params where I want it to be

Actually, looking through some of the other code, editing the cider-jack-in-clj function doesn't make sense, as a similar change would need to be made to the other cider-jack-in-* functions

Although I generally understand what the cider--update-jack-in-cmd code is doing, I am confused about the details of how its doing it. I'm afraid I am stuck, without further suggestions. My only though is to wrap an if after the let* and wrap the rest of the body of that function, which feels like it makes the code harder to understand.

TODO: Read the following article to see if it helps with a solution https://metaredux.com/posts/2019/11/02/hard-cider-understanding-the-jack-in-process.html

@practicalli-johnny
Copy link
Contributor Author

practicalli-johnny commented Dec 10, 2020

I am no longer using cider-jack-in with Clojure CLI tools so will close this issue

EDIT: use cider-clojure-cli-aliases variable to include project and user aliases in the cider-jack-in command. Do not use the deprecated cider-clojure-global-options variable as this will cause the REPL process to fail.

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

No branches or pull requests

2 participants