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 cider-jack-in-with-profile #544

Merged
merged 1 commit into from
May 14, 2014
Merged

Conversation

AdamClements
Copy link
Contributor

I have a cross platform leiningen project with multiple profiles, for which I need to have repls which start with different profiles. At the moment I have to do lein with-profile :local-test repl :headless manually which leaves a terminal floating around separate to my emacs window, and also leaves me not knowing which terminals are linked to which emacs windows.

I've proposed a cider-jack-in-with-profile interactive function, as cider-jack-in already had an optional parameter and so couldn't be overridden. This duplicated most of cider-jack-in and so I have split out the common functionality, cider-jack-in should be functionally unaffected.

Please feel free to advise on style and appropriate locations for functions. I needed to make cider-server-command more flexible and so moved it into a function, which means that the locate-file might need to happen more than once, but a jack-in is a relatively infrequent action anyway, so I don't know how much this matters.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2014

It's definitely a good idea, however, I can certainly imagine someone wanting to specify both a project dir and a profile together, so I think it might be much simpler to just keep a single prefix arg and ask the users to input both params (with some reasonable pre-filled defaults).

@AdamClements
Copy link
Contributor Author

That's true, with the current project was filled out as the default, hitting enter is no great inconvenience, likewise if the default profile was selected. I've never really used prefix arguments before so didn't really think of it.

Would you like me to make the changes and re-submit a pull request with a single commit? Or should I just alter this one?

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2014

Add commits to your current branch, squash them together into one and force push. Best way to update a PR IMO. :-)

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2014

P.S. You should also mention the updated functionality in the changelog.

@hugoduncan
Copy link
Member

This should close #327 too.

@AdamClements
Copy link
Contributor Author

I've made the changes as suggested, and the resulting commit is a lot smaller and neater. I've tested it with all my projects (both with profiles and simpler ones without) and all seems well here.

@AdamClements
Copy link
Contributor Author

Chatting to gtrak and technomancy in IRC, they have raised the possibility of leaving the cider-server-command string where it was (which means that it's amenable to local-dir-vars if you want to set the command for a specific project) and that instead of prompting for a profile and then munging it into the lein repl command, we actually display the whole command string, pre populated with the default, and allow the user to make arbitrary changes, for example 'lein with-profile blah do clean, compile, repl'

@gtrak
Copy link
Contributor

gtrak commented Apr 30, 2014

:-D +1

On Wed, Apr 30, 2014 at 12:42 PM, AdamClements [email protected]:

Chatting to gtrak and technomancy in IRC, they have raised the possibility
of leaving the cider-server-command string where it was (which means that
it's amenable to local-dir-vars if you want to set the command for a
specific project) and that instead of prompting for a profile and then
munging it into the lein repl command, we actually display the whole
command string, pre populated with the default, and allow the user to make
arbitrary changes, for example 'lein with-profile blah do clean, compile,
repl'


Reply to this email directly or view it on GitHubhttps://github.com//pull/544#issuecomment-41819435
.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2014

I guess this makes some sense. I'm not opposed to their idea.

P.S. Why are there no cider discussions every time I'm on #clojure? :-)

@gtrak
Copy link
Contributor

gtrak commented Apr 30, 2014

@bbatsov*, *your time zone must be set incorrectly.

Don't worry, I am double agent, born in Moldova :-). (Hi NSA!)

On Wed, Apr 30, 2014 at 12:47 PM, Bozhidar Batsov
[email protected]:

I guess this makes some sense. I'm not opposed to their idea.

P.S. Why are there no cider discussions every time I'm on #clojure? :-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/544#issuecomment-41820101
.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2014

@AdamClements Let me know when you've updated the PR (again). :-)

@gtrak Not a lot of Clojure hackers in (Eastern) Europe unfortunately. Seems that the party is mostly state-side at this point. :-)

@AdamClements
Copy link
Contributor Author

@bbatsov is that the route you'd prefer then? I was checking for more opinions before I did any work on it.

I'm not too bothered either way myself, the existing pull request scratches my own itch and solves #327, and has the advantage of not requiring any more work ;-). If you think the added flexibility of editing the whole leiningen command is important though, then I'm happy to make the changes

@gtrak
Copy link
Contributor

gtrak commented Apr 30, 2014

I also think there's a slight usability benefit to know that cider is
simply calling lein repl under the hood. I avoided slime-jack-in or
whatever it's called for months of use (3 years ago) because I didn't
understand the difference between that and standard slime-connect. And I
had a lot of uncertainty regarding the implications of changing stuff for
fear of it breaking.

Showing the command will lead one to know the specific dependency on what
leiningen provides, faster.

On Wed, Apr 30, 2014 at 1:13 PM, AdamClements [email protected]:

@bbatsov https://github.com/bbatsov is that the route you'd prefer
then? I was checking for more opinions before I did any work on it.

I'm not too bothered either way myself, the existing pull request
scratches my own itch and solves #327#327,
and has the advantage of not requiring any more work ;-). If you think the
added flexibility of editing the whole leiningen command is important
though, then I'm happy to make the changes


Reply to this email directly or view it on GitHubhttps://github.com//pull/544#issuecomment-41822978
.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2014

@AdamClements I think it'd be nice to keep the command as a defcustom, so people could override it on a per-project basis. Let's go with @gtrak's idea and give users the ability to edit the entire lein command.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2014

Or we can go the extra fancy way and introduce a second prefix argument C-u C-u. With one prefix argument the command can allow to set only the project dir and the profile and with 2 it can allow you to change the project dir and the lein command.

@AdamClements
Copy link
Contributor Author

I think that would be a bit confusing and undiscoverable. I didn't actually know how prefix arguments worked before writing this feature.

Why not have a few extra jack-in commands called something like cider-jack-in-with-profile and cider-jack-in-custom. I don't know about anyone else, but when I want to do something, I hit m-x, enter an appropriate prefix and then tab through the available options to discover new features.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2014

I was actually planning the retire the cider-jack-in name in favour of just cider (to align it more closely with slime and save a few keystrokes). Prefix args are the Emacs way, but I guess some people might find them confusing. I'm certainly not extremely fond of having lots of similar commands.

@AdamClements
Copy link
Contributor Author

Oh, really? I like cider-jack-in. What would happen to the current functionality of just cider?

Anyway, I think the simpler thing to do is just have the one version which allows you to edit the whole command, it's not like the profiles prompt is going to save an awful lot of keystrokes and if it gets very tiresome, you can just set the default for a project.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2014

It was recently renamed to cider-connect.

Let's go with editing the whole command.

@AdamClements
Copy link
Contributor Author

When editing the whole cider-server-command, on some platforms the whole command includes 'echo "lein repl :headless" | eval $SHELL -l'. Is that a detail we don't mind exposing in the prompt for cider-jack-in? It's quite hard to hide it without then taking away the ability to customise it, and I don't know who might be customising that.

I think it's a detail we could reasonably hide, but then the decision about whether to use that or not is based on whether cider-lein-command is visible on the path, so we couldn't offer the string for arbitrary editing and then make that check because the user might change which executable we're looking for!

Do we know what the ramifications would be of changing this to always eval $SHELL -l in which case we could change this to simply lein repl :headless and allow editing, forget all this complication.

@bbatsov
Copy link
Member

bbatsov commented May 6, 2014

@AdamClements I don't even know why the | eval $SHELL -l variant exists (or at least I can't recall that right now). Maybe we can drop this altogether and make our lives (and the code) a bit simpler.

@AdamClements
Copy link
Contributor Author

One of my computers actually uses that variant - when I run emacs from my interactive shell, it hasn't run my .bashrc and so my bin folder containing leiningen isn't on the classpath. I think that's why it pipes it through $SHELL so it runs the appropriate rc file.

I might be wrong though...

@bbatsov
Copy link
Member

bbatsov commented May 6, 2014

Interactive shells would source both .bash_profile and .bashrc, so this doesn't sound right to me. Non-interactive shells source just the .bashrc file. At any rate, I don't think we should care for things like this in cider...

@bbatsov
Copy link
Member

bbatsov commented May 6, 2014

One more thing - the locate-file checks should be replaced with executable-find. Don't know how I hadn't noticed this by now. In case lein doesn't seem to be installed we should simply issue a message saying so.

@AdamClements
Copy link
Contributor Author

So my fairly stock ubuntu, emacs 24 + prelude setup doesn't work without the eval $SHELL -l option. I think it must be whether you have it on your system PATH or your user PATH... not sure. Either way I think perhaps it would be safer to do the eval all the time instead, which simplifies just as much. Can you think of any down sides to that?

The only problem I have now is getting the output of the nrepl to say if the command has failed completely, as it stands it just fails silently and connects to any available repl... not ideal. Any ideas?

@bbatsov
Copy link
Member

bbatsov commented May 6, 2014

How did you install leiningen? What does your exec-path look like in Emacs? In projectile I need to execute a ton of external commands and I've never had to fallback to a clean login shell. On both my computers eval $SHELL -l is not needed (lein is my exec-path).

@AdamClements
Copy link
Contributor Author

As per the readme, I installed it in ~/bin/lein, ~/bin is in my $PATH and my exec-path is only ("/usr/local/sbin" "/usr/local/bin" "/usr/sbin" "/usr/bin" "/sbin" "/bin" "/usr/games" "/usr/local/games" "/usr/lib/emacs/24.3/x86_64-linux-gnu")

@bbatsov
Copy link
Member

bbatsov commented May 6, 2014

This seems related to your problem - http://stackoverflow.com/questions/6411121/how-to-make-emacs-to-use-my-bashrc-file

Btw, Prelude, actually uses the package mentioned in the accepted answer, but enables it only for OS X. I guess it might be useful on Ubuntu as well. In the end the eval ... was probably used just to sidestep such situations in which the exec-path differed from the regular user PATH, which simply hides the problem instead of actually fixing it. I'm now 100% certain we should simply drop this hack.

@gtrak
Copy link
Contributor

gtrak commented May 6, 2014

I agree we should drop this hack, but we should also make it obvious what
people should do to get things to work. I also run from ~/bin. Emacs not
respecting existing environment variables when launching processes is what's
at fault, but I only discovered this when I tried to alter JVM_OPTS for
leiningen, after 3 years of use.

On Tue, May 6, 2014 at 2:28 PM, Bozhidar Batsov [email protected]:

This seems related to your problem -
http://stackoverflow.com/questions/6411121/how-to-make-emacs-to-use-my-bashrc-file

Btw, Prelude, actually uses the package mentioned in the accepted answer,
but enables it only for OS X. I guess it might be useful on Ubuntu as well.
In the end the eval ... was probably used just to sidestep such
situations in which the exec-path differed from the regular user PATH,
which simply hides the problem instead of actually fixing it. I'm now 100%
certain we should simply drop this hack.


Reply to this email directly or view it on GitHubhttps://github.com//pull/544#issuecomment-42340297
.

@gtrak
Copy link
Contributor

gtrak commented May 6, 2014

Come to think of it, I think I'd rather prefer always running through
$SHELL if not doing so meant things would break everyone. That would
follow principle of least surprise, usability vs conceptual integrity.

On Tue, May 6, 2014 at 2:32 PM, Gary Trakhman [email protected]:

I agree we should drop this hack, but we should also make it obvious what
people should do to get things to work. I also run from ~/bin. Emacs not
respecting existing environment variables when launching process is what's
at fault, but I only discovered this when I tried to alter JVM_OPTS for
leiningen, after 3 years of use.

On Tue, May 6, 2014 at 2:28 PM, Bozhidar Batsov [email protected]:

This seems related to your problem -
http://stackoverflow.com/questions/6411121/how-to-make-emacs-to-use-my-bashrc-file

Btw, Prelude, actually uses the package mentioned in the accepted answer,
but enables it only for OS X. I guess it might be useful on Ubuntu as well.
In the end the eval ... was probably used just to sidestep such
situations in which the exec-path differed from the regular user PATH,
which simply hides the problem instead of actually fixing it. I'm now 100%
certain we should simply drop this hack.


Reply to this email directly or view it on GitHubhttps://github.com//pull/544#issuecomment-42340297
.

@gtrak
Copy link
Contributor

gtrak commented May 6, 2014

I've been frustrated at other editors for doing the same thing and making
it needlessly hard to work around (wasted a couple of hours on sublime text
due to this same issue). Just a huge pain. Dev tools should share unix
envs.

On Tue, May 6, 2014 at 2:35 PM, Gary Trakhman [email protected]:

Come to think of it, I think I'd rather prefer always running through
$SHELL if not doing so meant things would break everyone. That would
follow principle of least surprise, usability vs conceptual integrity.

On Tue, May 6, 2014 at 2:32 PM, Gary Trakhman [email protected]:

I agree we should drop this hack, but we should also make it obvious what
people should do to get things to work. I also run from ~/bin. Emacs not
respecting existing environment variables when launching process is what's
at fault, but I only discovered this when I tried to alter JVM_OPTS for
leiningen, after 3 years of use.

On Tue, May 6, 2014 at 2:28 PM, Bozhidar Batsov <[email protected]

wrote:

This seems related to your problem -
http://stackoverflow.com/questions/6411121/how-to-make-emacs-to-use-my-bashrc-file

Btw, Prelude, actually uses the package mentioned in the accepted
answer, but enables it only for OS X. I guess it might be useful on Ubuntu
as well. In the end the eval ... was probably used just to sidestep
such situations in which the exec-path differed from the regular user
PATH, which simply hides the problem instead of actually fixing it. I'm
now 100% certain we should simply drop this hack.


Reply to this email directly or view it on GitHubhttps://github.com//pull/544#issuecomment-42340297
.

@bbatsov
Copy link
Member

bbatsov commented May 6, 2014

@gtrak That's a terrible idea as you'd get a different environment for the process, compared to the one for Emacs. I've been hacking on Emacs extensions for years and (almost) nobody does it this way; we shouldn't either.

@gtrak
Copy link
Contributor

gtrak commented May 6, 2014

Emacs's getenv shows the needed PATH pointing to ~/bin, which occurs
upstream in /etc/profile, but it doesn't respect ~/.bash_profile nor
~/.bashrc. If there's a way to make that work and to pass those along to
the lein process, I'd be a happy man.

Meanwhile, I'm manually running setenv in my init.el.

I had to do this to enable the G1 garbage collector in leiningen's JVM, not just the project JVM, which has real memory space benefits.

On Tue, May 6, 2014 at 2:39 PM, Bozhidar Batsov [email protected]:

@gtrak https://github.com/gtrak That's a terrible idea as you'd get a
different environment for the process, compared to the one for Emacs. I've
been hacking on Emacs extensions for years and (almost) nobody does it this
way; we shouldn't either.


Reply to this email directly or view it on GitHubhttps://github.com//pull/544#issuecomment-42341693
.

@bbatsov
Copy link
Member

bbatsov commented May 6, 2014

Pretty sure this is an OS X/Ubuntu/Debian fault, not Emacs's. Anyways, installing exec-path-from-shell solves this.

@gtrak
Copy link
Contributor

gtrak commented May 6, 2014

@bbatsov, you're right. I verified that emacs has the right environment
when run from a bash terminal, yet 'java -jar envs.jar', which is a small
program I wrote to dump the env variables, does the incorrect thing when run
from KDE's alt-f2 runner.

If emacs passes along its environment to its child processes, lein in this
case, then it's fine by me. I can easily figure out a way to run it with
the correct environment and others should do the same.

On Tue, May 6, 2014 at 2:49 PM, Bozhidar Batsov [email protected]:

Pretty sure this is an OS X/Ubuntu/Debian fault, not Emacs's. Anyways,
installing exec-path-from-shellhttps://github.com/purcell/exec-path-from-shellsolves this.


Reply to this email directly or view it on GitHubhttps://github.com//pull/544#issuecomment-42342949
.

@bbatsov
Copy link
Member

bbatsov commented May 9, 2014

@AdamClements Ping :-)

@AdamClements
Copy link
Contributor Author

Okay then, I'll remove the hack. If we do that then we could also support Lein command separately to the parameters which would be a bit nicer.

I'm a little worried how many people this will break things for, how will we go about informing them? Would it make sense to depend on the package you mentioned to fix the path?

Hope to get a bit of time to do this over the weekend.

@bbatsov
Copy link
Member

bbatsov commented May 9, 2014

They don't really need this package, they just need a properly configured exec-path. The message they'll see will be fairly informative, so I'm not really worried about any fallout from such a change.

@AdamClements
Copy link
Contributor Author

I made the changes, and all the config looks right, it can find lein in the path, but it seems to hang at "Connecting to nREPL" for me now. I know it's running the process, because it shows any errors that come up when starting lein, but the nrepl never seems to show up...

@bbatsov
Copy link
Member

bbatsov commented May 13, 2014

@AdamClements See this issue #561 for tips on debugging the startup sequence.

@AdamClements
Copy link
Contributor Author

Fixed. There was a typo in one of the var names. Now works for me, but please test.

So, in this latest iteration, cider-lein-command does an executable-find of either lein or lein.bat, and sets itself appropriately, but you can of course override it. Separately to this, I have replaced cider-server-command with cider-lein-parameters which is a simple string, defaulting to "repl :headless". Now all it does is cd to the project directory and run the cider-lein-command with the cider-lein-parameters. If you do a prefix argument, you get the option to set the parameters for a given cider-jack-in as well as the project path.

bbatsov added a commit that referenced this pull request May 14, 2014
Add cider-jack-in-with-profile
@bbatsov bbatsov merged commit a9ef03c into clojure-emacs:master May 14, 2014
@Bill
Copy link

Bill commented Mar 19, 2015

To summarize for other noobs like me, who might read this PR and erroneously conclude that cider-jack-in-with-profile is a function available in emacs…if you want to cider-jack-in with a particular profile, do this in emacs:

  • M-x set-variable cider-lein-parameters to e.g. with-profile +my-special-snowflake repl :headless

or to set it interactively (so you can see it's current value before changing it):

  • C-h, v cider-lein-parameters and then click or hit enter on "customize" and set it there to e.g. with-profile +my-special-snowflake repl :headless and apply it

That'll cause your next cider-jack-in to load the my-special-snowflake profile in addition to the base profile (which is needed in order to run the nrepl and hence cider).

@bbatsov
Copy link
Member

bbatsov commented Mar 19, 2015

@Bill You can also invoke cider-jack-in with a prefix argument and you'll get prompted for a few things (like the command used to jack-in).

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.

5 participants