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

add defcustom cider-use-ssh-to-infer-remote-ports #2506

Merged

Conversation

aiba
Copy link
Contributor

@aiba aiba commented Nov 2, 2018

This is just an idea to address #1544. If the cider maintainers want it, I can update this PR to also include documentation of the new defcustom.

@bbatsov
Copy link
Member

bbatsov commented Nov 2, 2018

I don't use tramp, so I don't really care about this much. With me it's perfectly fine to just delete everything tramp-related, as it has been nothing but a pain in the ass for me. At this point I hate tramp (both because of CIDER and Projectile). :-)

On the other hand a lot of people seem to be using it for some weird reason... In principle I'm fine with the suggested change.

(with-current-buffer (tramp-get-connection-buffer (cider--tramp-file-name vec))
(cider-locate-running-nrepl-ports dir))))))
(when cider-use-ssh-to-infer-remote-nrepl-ports
(let ((vec (vector "sshx" nil host "" nil))
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we shouldn't move the remote logic to a separate function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, but it didn't seem like a big win, especially since this function is already entangled with an ssh-hosts arg.

cider.el Outdated
@@ -1327,6 +1327,11 @@ Tramp version starting 26.1 is using a `cl-defstruct' rather than vanilla VEC."
(make-tramp-file-name :method (elt vec 0)
:host (elt vec 2)))))

(defcustom cider-use-ssh-to-infer-remote-nrepl-ports t
Copy link
Member

Choose a reason for hiding this comment

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

Probably we can shorten this simply to cider-infer-remote-nrepl-ports.
The defcustom should be moved to the top of the file and annotated accordingly.

@aiba
Copy link
Contributor Author

aiba commented Nov 3, 2018

With me it's perfectly fine to just delete everything tramp-related, as it has been nothing but a pain in the ass for me.

Yeah, same with me. My workflow is that I always edit files locally. When I connect to a remote repl, it's always a direct connection and I use git to sync files, rather than editing files remotely with tramp.

But I'm sympathetic to the workflow of editing files remotely with tramp.

Another strategy would be to have a master var, cider-enable-ssh-integration or cider-enable-tramp-integration, and then hide everything behind this var so non-tramp users (like us) can disable all those code paths with one var. Would you prefer that strategy?

@bbatsov
Copy link
Member

bbatsov commented Nov 5, 2018

It's a good suggestion, but ideally the nrepl-client should be completely detached from CIDER, so doing this properly will be trickier than it sounds. I think it's fine for now to have two variables, both of which should be nil by default and should be well documented.

@aiba
Copy link
Contributor Author

aiba commented Nov 13, 2018

I think it's fine for now to have two variables, both of which should be nil by default and should be well documented

OK, i'll clean this up and add docs for it.

@aiba aiba force-pushed the cider-use-ssh-to-infer-remote-ports branch from 42d4b50 to fd2855b Compare November 13, 2018 18:57
@aiba
Copy link
Contributor Author

aiba commented Nov 13, 2018

OK, I think this PR is ready for another look. I considered adding documentation to doc/configuration.md but I'm not sure it's worth it given how infrequently we expect people to turn this variable on, and that we might want to phase it out in the future.

@bbatsov
Copy link
Member

bbatsov commented Nov 15, 2018

I think it might actually be nice to add some section on connecting over ssh, but that's not that big of a deal.

CHANGELOG.md Outdated
@@ -25,6 +25,7 @@
### Changes

* [#2482](https://github.com/clojure-emacs/cider/issues/2482): Don't bind nREPL server started by `cider-jack-in` to `::` (use `localhost` instead).
* [#1544](https://github.com/clojure-emacs/cider/issues/1544): Add a new defcustom `cider-infer-remote-nrepl-ports` to control whether we use tramp/ssh to infer remote ports. Now defaulting to `nil` (previously it alwasy tried to infer).
Copy link
Member

@bbatsov bbatsov Nov 16, 2018

Choose a reason for hiding this comment

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

alwasy -> always

cider.el Outdated
(defcustom cider-infer-remote-nrepl-ports nil
"When true, cider will use ssh to try to infer nREPL ports on remote hosts."
:type 'boolean
:safe #'booleanp)
Copy link
Member

Choose a reason for hiding this comment

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

You should also add a :package-version property (0.19.0).

@bbatsov
Copy link
Member

bbatsov commented Nov 16, 2018

Btw, it seems to me that the two ssh options should probably be mentioned in http://www.cider.mx/en/latest/up_and_running/, as this makes more sense than the generic configuration section.

@aiba
Copy link
Contributor Author

aiba commented Nov 24, 2018

Btw, it seems to me that the two ssh options should probably be mentioned in http://www.cider.mx/en/latest/up_and_running/, as this makes more sense than the generic configuration section.

I can take a stab at docs for this. To me it makes sense to have a section somewhere (perhaps up-and-running, or perhaps a new page) called "Connecting over SSH". And I'm not sure I understand all of cider's behavior wrt ssh connections well enough to write this section, but I'll give it a try.

@aiba aiba force-pushed the cider-use-ssh-to-infer-remote-ports branch from fd2855b to fe81bd7 Compare November 24, 2018 15:39
@aiba
Copy link
Contributor Author

aiba commented Nov 24, 2018

I think this is ready for another look.

@bbatsov bbatsov merged commit 13256ec into clojure-emacs:master Dec 21, 2018
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.

2 participants