Leiningen self-install for nrepl #300

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

phillord commented Mar 9, 2013

Adds a command to enable self-installation of leiningen. This also requires of nrepl-server-command variable, which now searches for leiningen every invocation. This should ease the installation of a working clojure environment, as lein2 current involves installing by hand on many platforms.

phillord added some commits Feb 16, 2013

Automatically make lein available.
nrepl-server-command is now modified to take either 'search
or a string. Access is now via nrepl-fetch-server-command. If,
nrepl-server-command is a string, use this. If search, look for the
nrepl executable. If it is not found an error message is signalled.
As nrepl-fetch-server-command returns full paths, launch through a
login shell is no longer necessary.

A new command, nrepl-lein-self-install, is provided which fetches the
lein script (shell or batch file as appropriate) and installs it in
user-emacs-directory.
Require executable.
lein download uses this, but it wasn't required.
Search for .bat first on windows.
A file called "lein" gets found before the .bat file regardless of
whether Emacs is running cygwin internally.
Install lein jar in USERPROFILEDIRECTORY.
This is the location that the lein.bat file looks.

@phillord phillord closed this Mar 9, 2013

@phillord phillord reopened this Mar 9, 2013

@phillord phillord closed this Mar 9, 2013

@phillord phillord reopened this Mar 9, 2013

@phillord phillord closed this Mar 9, 2013

@phillord phillord reopened this Mar 11, 2013

Owner

bbatsov commented Mar 11, 2013

Do we need this at all in nrepl.el? It's present in lein.el anyways.

Contributor

phillord commented Mar 11, 2013

If everyone who was running nrepl.el had lein.el then perhaps not. But they don't. I think it would ease the task of getting a working clojure environment working and that is no bad thing. Particularly when lein2 is currently not the main version packaged in most repos, and/or requires wget on windows. It also effectively places the version dependency of lein under the control of nrepl, which is no bad thing.

Of course, it would mean effectively some code duplication between lein.el and nrepl.el. It's not too large though. And if everybody does start to use lein.el, then it could be backed out again.

Either way, the code is there, and it works in the environments that I have been able to test. Do we need it? It's not essential, but it might be useful. But then, I don't think it will cause problems either. It does need testing on at least one other Mac though.

Parts of the change could do with percolating anyway. nrepl.el currently behaves badly when lein is not installed. It gives a very opaque error message, and requires a restart of Emacs after installation.

Contributor

technomancy commented Mar 11, 2013

I'd rather point people to lein.el, but it still has problems for long-running tasks. It's definitely a better solution in the long run, but I'm not sure when I'll have time to fix it.

I don't know what Tim's policy is for backwards-compatibility, but adding functionality you plan on removing is usually not a good idea unless you have plans for a major version bump which signals to users the fact that they shouldn't expect to be able to upgrade and have everything still work.

Owner

bbatsov commented Mar 11, 2013

@phillord I'm not saying it will cause problems, I'm just wary of adding code that's been duplicated somewhere else. Maybe we should just encourage people to install lein.el if we find lein2 is not installed? We can even run (package-install 'lein) automatically for them.

Contributor

phillord commented Mar 11, 2013

I agree with you both that lein.el is nicer in the abstract. But as Phil H. points out, while the self-install part works, the package itself is not complete. Hence, the code base here.

In terms of removing code, what I would say is that, I think, my changes to nrepl-server-command, and addition of nrepl-fetch-server-command stand in their own right (although they are necessary for nrepl-lein-self-install to work without restart). They are probably worth incorporating anyway, again with the proviso that I have only limited ability to test on Mac's.

If the self-install code were removed at a later date, in favour of lein.el, then these changes could stay. Any lein installation from lein.el would work.

If lein.el were at a point where Phil H. thinks it's fixed, then nrepl-lein-self-install could just call the same code in lein.el, and lein.el could be added as a dependency. (package-install 'lein) would sort of come with package dependency management I think. So, removing the code duplication would not involve removing functionality. nrepl-lein-self-install could stay where it is.

At least this makes sense to me. I was moving an Emacs from Slime/swank to nrepl last week, and having nrepl self-install would have made the whole process very slick. It could even be added to the package install.

Owner

bbatsov commented Mar 11, 2013

At any rate - you should really squash those 13 commits into 1 :-)

Contributor

phillord commented Mar 11, 2013

I'm a git newbie, and this is how it turned out. Took me 13 commits to get it right. That's also why I opened and closed the PR several times. How do I squash commits?

Owner

bbatsov commented Mar 11, 2013

At your feature branch run git rebase master -i.

An editor will open with the list of commits and you'll have to select which to keep (in your case the first one), which to squash(all the rest) and what the combined commit message for all of them should be. Afterwards do a git push -f in your feature branch to update it upstream.

Contributor

technomancy commented Mar 11, 2013

Making nrepl.el depend on lein.el will never happen, (package.el does not support circular dependencies) but one compromise could be that nrepl.el could interactively ask whether the user would like lein.el installed when it detects Leiningen is not found. As long as there's a clear prompt explaining what's going on it should be fine.

Contributor

phillord commented Mar 12, 2013

Oh, yes, I remember now, we discussed this before. Personally, I'd leave the code duplication in place. It's probably less lines that we have spent discussing how bad it is:-)

Owner

bbatsov commented Jul 24, 2013

I'm with @technomancy on this one - we should simply prompt the user to install lein.el.

Contributor

phillord commented Jul 24, 2013

I may have a go at that, when I get time. I've screwed up the PR anyway, so I'll close this and leave it.

@phillord phillord closed this Jul 24, 2013

Owner

bbatsov commented Jul 24, 2013

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment