Skip to content

cider-connect: auto-detect ssh hosts and all running repls on local and remote hosts #808

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 21, 2014
Merged

cider-connect: auto-detect ssh hosts and all running repls on local and remote hosts #808

merged 1 commit into from
Sep 21, 2014
+162 −98

Conversation

@vspinu
Copy link
Contributor

@vspinu vspinu commented Sep 17, 2014

Two main changes:

  • cider-connect now interactively asks for host from a combined list of
    machine-wide ssh configuration and cider-known-endpoints.
    cider4
  • Once the host has been selected cider-connect locates all running instances
    of lein servers. The machine must have ps and grep programs in the bin
    path.
    cider5

The algorithm lists user processes with ps and retrives lein paths , then searches for nrepl-port files. This detection works fine if you run one server per project. If you run more servers at the same time lein will overwrite that file and will eventually delete it on first server exit. Hopefully this issue will be sorted soon in technomancy/leiningen#1686.

It will probably not work on windows out of the box, but should not break anything.

bbatsov
bbatsov reviewed Sep 17, 2014
View reviewed changes
cider.el Outdated
Copy link
Member

@bbatsov bbatsov Sep 17, 2014

Choose a reason for hiding this comment

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

This function is way too complex IMO and should be broken up into several functions.

@bbatsov
Copy link
Member

bbatsov commented Sep 17, 2014

It will probably not work on windows out of the box, but should not break anything.

It certainly won't. We have plenty of Windows users and should avoid Unix-specific stuff. There has to be a more portable way to do this.

@bbatsov
Copy link
Member

bbatsov commented Sep 17, 2014

Also - when this is broken down into smaller units of work at least some of them should be unit tested.

@vspinu
Copy link
Contributor Author

vspinu commented Sep 17, 2014

Well it should work on Linux and Mac and we have plenty of those. Windows users will have the same functionality as they had before. This is uniformly better.

To adapt that to Windows one would need 1) process listing command 2) regexp change. That should be an easy task for any Windows user. I expect someone to contribute this very soon. I am not a Windows user and have no capability to figure that out.

Moreover, for remote functionality we already rely on ssh tunneling. I would guess that doesn't work on Windows either.

@vspinu
Copy link
Contributor Author

vspinu commented Sep 17, 2014

Also note that once technomancy/leiningen#1686 is fixed this hack won't be necessary. So I would better focus on an user interaction side right now.

@bbatsov
Copy link
Member

bbatsov commented Sep 18, 2014

Well it should work on Linux and Mac and we have plenty of those. Windows users will have the same functionality as they had before. This is uniformly better.

Well, I don't use Windows, so I can live without support for it. :-)

Moreover, for remote functionality we already rely on ssh tunneling. I would guess that doesn't work on Windows either.

Hadn't thought about this. Guess this tunnelling support should be documented somewhere, because likely few people know it exists.

Break up the code into smaller bits and I'll have a look at it.

bbatsov
bbatsov reviewed Sep 18, 2014
View reviewed changes
cider.el Outdated
Copy link
Member

@bbatsov bbatsov Sep 18, 2014

Choose a reason for hiding this comment

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

leinengen -> leiningen

@vspinu
Copy link
Contributor Author

vspinu commented Sep 19, 2014

Break up the code into smaller bits and I'll have a look at it.

I have looked into that and it's not easy. Everything is interconnected. So I have simplified it as mush as I could, gave self-documenting names to all locals and inline-documented the code. Should be pretty clear what it does right now.

I have also extracted the ps command and leiningen path regexps into separate variables so that people on different machines can adjust them as needed.

@vspinu vspinu force-pushed the cider-connect branch 2 times, most recently from 45dbe00 to e47ed82 Compare September 19, 2014 06:22
bbatsov
bbatsov reviewed Sep 19, 2014
View reviewed changes
cider.el Outdated
Copy link
Member

@bbatsov bbatsov Sep 19, 2014

Choose a reason for hiding this comment

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

Inlining code that complex in a let reduces readability a lot. Extracting this would be easy.

@bbatsov
Copy link
Member

bbatsov commented Sep 19, 2014

have looked into that and it's not easy. Everything is interconnected. So I have simplified it as mush as I could, gave self-documenting names to all locals and inline-documented the code. Should be pretty clear what it does right now.

I've always understood what the code does, but that's not the point. I'm certain in can be simplified by extracting portions of that monstrous let* form. Smaller bits of this will also be testable to some extent, which is always a good thing.

@vspinu vspinu force-pushed the cider-connect branch 2 times, most recently from ba907ca to f394489 Compare September 20, 2014 01:12
@vspinu
Copy link
Contributor Author

vspinu commented Sep 20, 2014

Ok, I have extracted the pieces out.

@bbatsov
Copy link
Member

bbatsov commented Sep 20, 2014

In the commit message:

  • start it with a capitalised word
  • repls -> nREPL servers
  • instances of lein servers -> instances of nREPL servers started with lein repl.

bbatsov
bbatsov reviewed Sep 20, 2014
View reviewed changes
cider.el Outdated
Copy link
Member

@bbatsov bbatsov Sep 20, 2014

Choose a reason for hiding this comment

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

Btw, leiningen is not the only tool that writes a .nrepl-port file. Immutant also does so, maybe others as well.

Copy link
Contributor Author

@vspinu vspinu Sep 20, 2014

Choose a reason for hiding this comment

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

Btw, leiningen is not the only tool that writes a .nrepl-port file. Immutant also does so, maybe others as well.

Be it cider-locate-running-nrepl-ports then.

Copy link
Member

@bbatsov bbatsov Sep 20, 2014

Choose a reason for hiding this comment

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

Yeah, that's an OK name.

@vspinu vspinu force-pushed the cider-connect branch 2 times, most recently from 44e396d to e4e8dd5 Compare September 21, 2014 19:36
@vspinu
Copy link
Contributor Author

vspinu commented Sep 21, 2014

Done. I gave generic running-nrepl names to all involved variables.

bbatsov
bbatsov reviewed Sep 21, 2014
View reviewed changes
temp.el1 Outdated
Copy link
Member

@bbatsov bbatsov Sep 21, 2014

Choose a reason for hiding this comment

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

You should remove this file.

Copy link
Contributor Author

@vspinu vspinu Sep 21, 2014

Choose a reason for hiding this comment

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

Fixed. Thanks.

@vspinu
Enhance cider-connect to auto-detect running nREPL servers
57ac604 
 - `cider-connect` now interactively asks for host from a combined list of hosts
   from machine-wide ssh configuration and `cider-known-endpoints`.

 - Once the host has been selected `cider-connect` locates ports of the running
   instances of nREPL servers on the corresponding host (currently only those
   started with lein repl). The machine must have `ps` and `grep` programs in
   the bin path.
bbatsov added a commit that referenced this pull request Sep 21, 2014
@bbatsov
Merge pull request #808 from vitoshka/cider-connect
34ba845 
`cider-connect`: auto-detect ssh hosts and all running repls on local and remote hosts
@bbatsov bbatsov merged commit 34ba845 into clojure-emacs:master Sep 21, 2014
@vspinu vspinu deleted the cider-connect branch September 21, 2014 21:08
@vspinu vspinu mentioned this pull request Sep 25, 2014
Closed
@vspinu vspinu mentioned this pull request Dec 11, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vspinu @bbatsov