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

When using cider-connect with localhost nrepls running, those ports are not shown among the available ones #3390

Closed
ag91 opened this issue Aug 1, 2023 · 6 comments · Fixed by #3399

Comments

@ag91
Copy link
Contributor

ag91 commented Aug 1, 2023

Expected behavior

For a running nrepl on localhost, I expected (cider-select-endpoint) to allow me to select ports on which nrepl is running.
(I use this to connect ob-clojure buffers to cider when doing literate programming and a bit of completion comes handy)

Fixes #3390

Actual behavior

It requires me to input the port manually.

Steps to reproduce the problem

  1. cider-jack-in without a project
  2. cider-connect from another clj buffer
  3. follow instructions to get to the cider session you opened before
  4. see that there are no available ports

or more specifically:

  1. eval in emacs (cider--running-nrepl-paths)
  2. see an empty output sigh!

My solution

Just for curiosity, I redefined cider--running-nrepl-paths (which seems to be the culprit) as follows:

(defun cider--running-nrepl-paths ()
  "Retrieve project paths of running nREPL servers.
Use `cider-ps-running-nrepls-command' and
`cider-ps-running-nrepl-path-regexp-list'."
  (seq-uniq
   (seq-map
    (lambda (b)
      (with-current-buffer b default-directory))
    (seq-filter
     (lambda (b) (string-prefix-p "*cider-repl" (buffer-name b)))
     (buffer-list)))))

That way things seem to work fine (for localhost).
I opened an issue because I think cider-ps-running-nrepls-command and cider-ps-running-nrepl-path-regexp-list are somewhat outdated. I replaced the behavior going via Emacs active buffers.
Any chance you can clarify why cider is looking for active nrepl via a terminal command (greping for leiningen -- I didn't find anything using that command manually)?

If this solution seems useful, I could look into opening a PR :)

CIDER version information

1.8.0-snapshot

Lein / Clojure CLI version

lein 2.9.1 / 1.10.3

Emacs version

28.1

Operating system

Ubuntu 22.04

@vemv
Copy link
Member

vemv commented Aug 2, 2023

Hi!

Thanks for the detailed issue.

Any chance you can clarify why cider is looking for active nrepl via a terminal command (greping for leiningen -- I didn't find anything using that command manually)?

This very likely aims to make it possible to connect to repls that were created outside of CIDER, in the terminal, especially if their pwd is not the current-directory of any Emacs buffer.

I think cider-ps-running-nrepls-command and cider-ps-running-nrepl-path-regexp-list are somewhat outdated.

I agree. However I think we could take the current cider-locate-running-nrepl-ports impl from:

  • greps Leiningen processes for a path

to:

  • greps Leiningen for a path,
  • greps java processes that don't contain lein in it
    • this covers Clojure CLI and direct java invocations
    • gets port from either simple --port argument parsing (this is a nrepl.cmdline flag), or fallback to lsof
  • iterates though all active *cider-repls, call cider--gather-connect-params on each, extract the port param from there
  • concatenates and seq-uniqs all of them

WDYT?

@bbatsov
Copy link
Member

bbatsov commented Aug 2, 2023

Sounds reasonable.

@ag91
Copy link
Contributor Author

ag91 commented Aug 2, 2023

I agree. However I think we could take the current cider-locate-running-nrepl-ports impl from:

I looked into it a bit and a thing to note: cider-locate-running-nrepl-ports returns pairs path-port (e.g., '(("~" "41655"))).
Due to that, for my use case I changed cider--running-nrepl-paths instead.
To make things work in cider-locate-running-nrepl-ports I may use a terminal command like pwdx to capture the working dir of those process identifiers I find.

Also would it be wrong to look for processes that mention nrepl (I have -m nrepl.cmdline in the processes I spin via cider, but i guess maybe that is unique to Clj Cli)? Otherwise I think I may also catch java processes that Cider cannot connect to.

And how do you usually test these kind of changes? I am using a Linux machine, so I will be confident for those bash commands, but MacOS (and Windows?!).

@vemv
Copy link
Member

vemv commented Aug 3, 2023

I looked into it a bit and a thing to note: cider-locate-running-nrepl-ports returns pairs path-port (e.g., '(("~" "41655"))).

Yes, I had the impression that that's for better usability (this way one can present the "context" for each port in the completing-read)

Also would it be wrong to look for processes that mention nrepl (I have -m nrepl.cmdline in the processes I spin via cider, but i guess maybe that is unique to Clj Cli)? Otherwise I think I may also catch java processes that Cider cannot connect to.

"nrepl.cmdline" can be present in Clojure CLI and vanilla java invocations (the latter might become increasingly common because of #3364)

I'm not sure about inspecting java processes that match nrepl but not nrepl.cmdline. I think it can be done, provided that we present a short, informative context in the completing-read (like the pwd, suffixed by " other" if there was another java process in the same pwd)

...If including such processes excessively complicated things, I'd be willing to exclude them.

And how do you usually test these kind of changes? I am using a Linux machine, so I will be confident for those bash commands, but MacOS (and Windows?!).

Currently defvar cider-ps-running-nrepls-command has the fixed "ps u | grep leiningen" value. It's not even a defcustom 😅 so it doesn't matter much if it doesn't work on Windows.

As long as we stick to simple primitives like ps/grep, we can be fine with no testing / just a one-off round of QAing. I can try it on my macbook while reviewing a PR.

Cheers - V

ag91 added a commit to ag91/cider that referenced this issue Aug 3, 2023
ag91 added a commit to ag91/cider that referenced this issue Aug 4, 2023
vemv pushed a commit that referenced this issue Aug 10, 2023
vemv pushed a commit that referenced this issue Aug 11, 2023
vemv added a commit that referenced this issue Aug 11, 2023
…f only Leiningen ones (#3399)

Fixes #3390

Co-authored-by: ag91 <ag91@users.noreply.github.com>
@ag91
Copy link
Contributor Author

ag91 commented Aug 11, 2023

thank you @vemv and @bbatsov, it is easier to connect now :D

@vemv
Copy link
Member

vemv commented Aug 12, 2023

thank you @vemv and @bbatsov, it is easier to connect now :D

🍻

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