Show available nrepl ports when connecting #3392

ag91 · 2023-08-03T22:02:06Z

Improve cider-connect to show all available ports of running nREPLs.
Fixes #3390
See recording for a peek :)

simplescreenrecorder-2023-08-03_23.00.23.mp4

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (eldev test)
All code passes the linter (eldev lint) which is based on elisp-lint and includes
- byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the user manual (if adding/changing user-visible functionality)

ag91 · 2023-08-03T22:10:27Z

@vemv I seem to have managed to make things work via cider--running-nrepl-paths because nrepl-extract-ports seemed pretty good at finding the correct port.
I could have done through lsof, but things seemed to work fine.
The shell command for finding external nREPL came out a bit long (see 4da151c):

I had to select the right process id via ps + grep + awk and then feed all the ids separated by comma to lsof to extract the current working directory of the processes. It is a mouthful to write down and a pretty long shell command: I would be grateful for an opinion about how to make it more readable :)

vemv

Looks like a great start to me!

Added some suggestions.

I would be grateful for an opinion about how to make it more readable

It looks reasonable to me, perhaps I'm not a fan of awk but I don't have have an immediate suggestion. Can always be refined later.

cider.el

vemv · 2023-08-04T04:05:25Z

cider.el

+(defun cider--running-other-nrepl-paths ()
+  "Retrieve project paths of other running nREPL servers."
+  (let* ((take-non-lein-nrepl-pids
+          "ps u | grep java | grep -v leiningen | grep nrepl.cmdline | awk '{print $2}' | tr '\012' ,")


what does | tr '\012' accomplish?

This works on my macos machine ps u | grep java | grep -v leiningen | awk '{print $2}'

adding | tr '\012' doesn't:

$ ps u | grep java | grep -v leiningen | awk '{print $2}' | tr '\012' usage: tr [-Ccsu] string1 string2 tr [-Ccu] -d string1 tr [-Ccu] -s string1 tr [-Ccu] -ds string1 string2

tr '\012' , is replacing newlines with commas
in linux, running this in the terminal

echo "hi hello" | tr '\012' ,

produces this

hi,hello,

It seems that lsof can take a comma separated list of process ids in the -p option, which was nice for our use case.
Maybe on MacOS the character for new line is not parsed as in Linux, I am substituting that with paste -s -d,, which seems also less cryptic (that changes both newlines or spaces in commas)

cider.el

vemv · 2023-08-04T04:17:26Z

cider.el

+    (lambda (b) (string-prefix-p "*cider-repl" (buffer-name b)))
+    (buffer-list))))
+
+(defun cider--running-nrepl-paths ()


nrepl-extract-ports will be called over each element of cider--running-nrepl-paths.

defun nrepl-extract-ports is based on (expand-file-name "repl-port" dir). That won't work over all possible invocations. It also represents redundant work, to some degree.

I'd suggest that when possible, return a (list dir port) instead of just a dir.

So that we can make this change:

(defun cider-locate-running-nrepl-ports (&optional dir) "Locate ports of running nREPL servers. When DIR is non-nil also look for nREPL port files in DIR. Return a list of list of the form (project-dir port)." (let* ((paths (cider--running-nrepl-paths)) (proj-ports (apply #'append (mapcar (lambda (d) + (if (listp d) + d ;; (directory port) pair was passed, nothing to do + ;; directory was passed, use old behavior: (mapcar (lambda (p) (list (file-name-nondirectory (directory-file-name d)) p)) (and d (nrepl-extract-ports (cider--file-path d)))))) (cons (clojure-project-dir dir) paths))))) (seq-uniq (delq nil proj-ports))))

uhm. I see the optimization here. A note though: we have to return a (list d) and we still have to apply (file-name-nondirectory (directory-file-name d)), to get just the directory name.
This also makes the seq-uniq needing a predicate for handling equality between paths and pairs (path-port).
So it feels we are trading too much on readability for what the optimization gives to us, no?

To get more, I could make also the other nREPL search return ports and I indeed had a thought while implementing that if I use lsof, I could get ports as well but it seemed less readable to further complicate the shell command.
Should this maybe be tackled in another issue to avoid the added code complexity for now?

(here the patch with the change if you want to see how the code would look like
opt.txt
)

Should this maybe be tackled in another issue to avoid the added code complexity for now?

Yes this sounds reasonable, your PR already is a great improvement 👍

vemv

Almost there - thanks for the iteration!

vemv · 2023-08-04T17:10:18Z

cider.el

+(defun cider--running-other-nrepl-paths ()
+  "Retrieve project paths of running nREPL servers other than Lein ones."
+  (let* ((take-non-lein-nrepl-pids
+          "ps u | grep java | grep -v leiningen | grep nrepl.cmdline | awk '{print $2}' | paste -s -d,")


It looks like it almost works on macos - here's a sample session:

~/orchard (master) $ ps u | grep java | grep -v leiningen | grep nrepl.cmdline | awk '{print $2}' 72926 ~/orchard (master) $ ps u | grep java | grep -v leiningen | grep nrepl.cmdline | awk '{print $2}' | paste -s -d, usage: paste [-s] [-d delimiters] file ...

uhm I made the redirection explicit following this https://www.reddit.com/r/linuxquestions/comments/y96jbk/why_cant_i_pipe_awk_into_paste_macos/?rdt=44305

sorry for the multiple attempts

vemv · 2023-08-04T17:12:37Z

cider.el

+          (concat
+           "lsof -a -d cwd -p $("
+           take-non-lein-nrepl-pids
+           ") -n -Fn | awk '/^n/ {print substr($0,2)}'")) )


I can confirm this works on macos:

~/orchard (master) $ lsof -a -d cwd -p $(ps u | grep java | grep -v leiningen | grep nrepl.cmdline | awk '{print $2}') -n -Fn | awk '/^n/ {print substr($0,2)}' /Users/vemv/orchard

It doesn't have the paste -s -d, part, which may have to be added/tweaked.

vemv · 2023-08-04T17:15:03Z

cider.el

+    (lambda (b) (string-prefix-p "*cider-repl" (buffer-name b)))
+    (buffer-list))))
+
+(defun cider--running-nrepl-paths ()


Should this maybe be tackled in another issue to avoid the added code complexity for now?

Yes this sounds reasonable, your PR already is a great improvement 👍

vemv · 2023-08-04T17:18:26Z

cider.el

+
+(defun cider--running-other-nrepl-paths ()
+  "Retrieve project paths of running nREPL servers other than Lein ones."
+  (let* ((take-non-lein-nrepl-pids


Let's guard this and similar code with (when-not (memq system-type '(cygwin windows-nt))

…ing-local-nrepl-paths CIDER misses nREPLs that didn't start via Leiningen. The plan is to enhance cider--running-nrepl-paths to have multiple sources of paths. I found out that given a path, cider-locate-running-nrepl-ports can find the right port via nrepl-extract-ports. This also adds the paths created by running cider-jack-in via Emacs.

This uses lsof in combination with ps and grep to find paths of java processes that may be nREPL. Again we rely on nrepl-extract-ports in cider-locate-running-nrepl-ports to get rid of invalid paths.

fixes clojure-emacs#3390

Co-authored-by: vemv <[email protected]>

…indows Some of the commands we run via shell are incompatible with the Windows prompt, for example there is no lsof command in Windows.

bbatsov · 2023-08-08T05:17:52Z

cider.el

@@ -1810,17 +1810,56 @@ of list of the form (project-dir port)."
                                    (cons (clojure-project-dir dir) paths)))))
    (seq-uniq (delq nil proj-ports))))

-(defun cider--running-nrepl-paths ()
-  "Retrieve project paths of running nREPL servers.
+(defun cider--windows-p ()


Do we even need this helper? I mean - nothing modern is going to return ms-dos and I think cygwin provides the common Unix commands.

I am not sure myself, I wanted to add the guard because we didn't test there and there may be chance the functions don't work there. I can definitely remove ms-dos, I added it only for completeness.

bbatsov · 2023-08-08T05:19:48Z

cider.el

+            (setq paths (cons (match-string 1) paths)))))
+      paths)))
+
+(defun cider--running-other-nrepl-paths ()


I think "other" in this name might be a big confusing, as you won't understand without the docstring that it means "other than Lein". Perhaps "non-lein"?

bbatsov · 2023-08-08T18:40:55Z

CHANGELOG.md

@@ -21,6 +21,7 @@
 - [#3236](https://github.com/clojure-emacs/cider/issues/3236): `cider-repl-set-ns` no longer changes the repl session type from `cljs:shadow` to `clj`.
 - [#3383](https://github.com/clojure-emacs/cider/issues/3383): `cider-connect-clj&cljs`: don't render `"ClojureScript REPL type:" for JVM repls. 
 - [#3331](https://github.com/clojure-emacs/cider/issues/3331): `cider-eval`: never jump to spurious locations, as sometimes conveyed by nREPL.  
+- [#3390](https://github.com/clojure-emacs/cider/issues/3390) Enhance cider-connect to show all nREPLs available ports, instead of only Leinengen ones.


Don't forget the : here. Also - Leiningen. :-)

…non-lein-nrepl-paths

vemv

One last detail, I believe 🙌

vemv · 2023-08-09T07:52:42Z

cider.el

+  "Retrieve project paths of running nREPL servers other than Lein ones."
+  (unless (cider--windows-p)
+    (let* ((take-non-lein-nrepl-pids
+            "ps u | grep java | grep -v leiningen | grep nrepl.cmdline | awk '{print $2}' | paste -s -d, -")


I verified that lsof -a -d cwd -p $(ps u | grep java | grep -v leiningen | grep nrepl.cmdline | awk '{print $2}' | paste -s -d, -) -n -Fn | awk '/^n/ {print substr($0,2)}' can run OK on macOS - it can return either nothing or ports.

However, your code should handle the case where nothing is returned. Else the lsof call will fail.

For that, I'd suggest running take-non-lein-nrepl-pids as an individual shell-command-to-string-call. Abort the defun if it returned nothing / empty string. Else, it's safe to proceed.

Under this approach, $() is no longer needed.

vemv · 2023-08-09T08:03:08Z

cider.el

+  "Retrieve project paths of running nREPL servers.
+Search for lein or java processes including nrepl.command nREPL"
+  (let ((paths (append
+                (cider--running-lein-nrepl-paths)


I wonder if we could have some generic error handling. It wouldn't be too surprising to hear that someone was running a very particular *nix, such that cider-connect is failing for them due to the ps u | grep java | grep -v leiningen | grep nrepl.cmdline | awk | ... call.

Needless to say, cider-connect is too important to fail because of a feature that in the end is a "nice to have".

Maybe, wrap each call in a condition-case, and additionally, filter the results by file-directory-p (to avoid including any error-like messages)?

vemv · 2023-08-09T10:15:52Z

(I wrote something intended for another PR, deleted. Please ignore)

vemv · 2023-08-10T11:46:50Z

I'll reopen this one from scratch, preserving attribution.

After more QAing, I noticed some code fails to handle more than one available port
I'm finding it's simpler / more portable to do things in Elisp (vs awk, etc)

I would not want to create a frustrating "feedback loop" - we all know what it's to be there!

Thanks - V

ag91 · 2023-08-10T16:27:15Z

I would not want to create a frustrating "feedback loop" - we all know what it's to be there!

it was alright, I suspect it would have made sense for me to just add the local dir paths and leave the other bit for a second PR.
Although I hope my work helped threading ideas together to get to your final solution ;)

ag91 force-pushed the show-available-nrepl-ports-when-connecting branch from f4c4d9b to ad5bc9d Compare August 3, 2023 22:03

vemv reviewed Aug 4, 2023

View reviewed changes

ag91 and others added 8 commits August 4, 2023 22:12

get paths of nREPLs by grepping java processes

b058472

This uses lsof in combination with ps and grep to find paths of java processes that may be nREPL. Again we rely on nrepl-extract-ports in cider-locate-running-nrepl-ports to get rid of invalid paths.

add entry to changelog

306b2cf

fixes clojure-emacs#3390

Fix doc string

1386f7f

Co-authored-by: vemv <[email protected]>

use paste instead of tr to replace newlines for commas

28e559c

use thread-last in cider--running-local-nrepl-paths

6e6e2a5

add safety guard against nil values

fdff395

make paste command redirection explicit for MacOS and guard against w…

0ec5789

…indows Some of the commands we run via shell are incompatible with the Windows prompt, for example there is no lsof command in Windows.

ag91 force-pushed the show-available-nrepl-ports-when-connecting branch from 4c0699e to 0ec5789 Compare August 4, 2023 21:13

bbatsov reviewed Aug 8, 2023

View reviewed changes

fix changelog and cider--running-other-nrepl-paths -> cider--running-…

1e18ff8

…non-lein-nrepl-paths

vemv reviewed Aug 9, 2023

View reviewed changes

vemv closed this Aug 10, 2023

vemv mentioned this pull request Aug 10, 2023

Enhance cider-connect to show all nREPLs available ports, instead of only Leiningen ones #3399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show available nrepl ports when connecting #3392

Show available nrepl ports when connecting #3392

ag91 commented Aug 3, 2023 •

edited

Loading

ag91 commented Aug 3, 2023

vemv left a comment

vemv Aug 4, 2023

ag91 Aug 4, 2023

vemv Aug 4, 2023

ag91 Aug 4, 2023

ag91 Aug 4, 2023

vemv Aug 4, 2023

vemv left a comment

vemv Aug 4, 2023

ag91 Aug 4, 2023

vemv Aug 4, 2023

vemv Aug 4, 2023

vemv Aug 4, 2023

bbatsov Aug 8, 2023

ag91 Aug 8, 2023

bbatsov Aug 8, 2023

bbatsov Aug 8, 2023

vemv left a comment

vemv Aug 9, 2023

vemv Aug 9, 2023

vemv commented Aug 9, 2023

vemv commented Aug 10, 2023

ag91 commented Aug 10, 2023

Show available nrepl ports when connecting #3392

Show available nrepl ports when connecting #3392

Conversation

ag91 commented Aug 3, 2023 • edited Loading

ag91 commented Aug 3, 2023

vemv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv commented Aug 9, 2023

vemv commented Aug 10, 2023

ag91 commented Aug 10, 2023

ag91 commented Aug 3, 2023 •

edited

Loading