Support launching server preinstalled by build tools #354
Conversation
| if not bootstrap_server and no_classpath: | ||
|
|
||
| installed = self.launcher.strategy.isinstalled() | ||
| if not installed and not bootstrap_server: |
There was a problem hiding this comment.
This little strategy.isinstalled() Law of Demeter violation felt dirty for a moment, but I feel the right solution is probably not a delegating method on the launcher itself, but getting this logic entirely out of EnsimeClient as per #325. And likely extending to some of the work toward server lifecycle management.
| @@ -7,7 +7,7 @@ | |||
|
|
|||
| from ensime_shared.util import Util | |||
|
|
|||
| BOOTSTRAPS_ROOT = os.path.join(os.environ['HOME'], '.config/ensime-vim/') | |||
| BOOTSTRAPS_ROOT = os.path.join(os.environ['HOME'], '.config', 'ensime-vim') | |||
There was a problem hiding this comment.
Unrelated, but the config folder location could be improved by using XDG for Linux and AppData for Windows.
| def using_server_v2(self): | ||
| """Whether user has configured the plugin to use ENSIME v2 protocol.""" | ||
| """bool: Whether user has configured the plugin to use ENSIME v2 protocol.""" |
There was a problem hiding this comment.
I'm a bit confused, keeping this setting just to choose the client version can't lead to a client and server version mismatch?
There was a problem hiding this comment.
Nvm, it's an undocumented setting so only people knowing what they do should be using it.
There was a problem hiding this comment.
Sorry about the noise, I think we should get rid of this setting and ask the launcher which version we should use for the client.
| config = ProjectConfig(config_path) | ||
| launcher = EnsimeLauncher(self._vim, config) | ||
|
|
||
| if self.using_server_v2: |
There was a problem hiding this comment.
I think there is a bit of space for improvement here, I think it's not ideal to pick the version of the server and the client separately, so we could ask the launcher which version of the server it is gonna launch. After applying the heuristic to pick the strategy it has all the needed information to answer that question. For assembly jar it can extract the server version from the jar name, for dotensime it has the sbt setting to specify the server version and for bootstrap it's always 1.0 I guess.
What you reckon?
There was a problem hiding this comment.
You're right, it isn't ideal to keep a client config setting for this if it's now configurable from the build tools, I hadn't really thought about removing the client setting altogether.
I'm divided about how to go about this, though. Right now the launcher doesn't actually care what version it's launching, and it's kind of nice that it doesn't need to. Parsing it out of jar file names in :ensime-server-jars feels icky to me; it's not otherwise directly available as a field in the .ensime file, and introducing coupling to particular build tools to get it (e.g. asking sbt for ensimeVersion) seems wrong too, at least for this purpose.
IMO what feels right is to get the server version strictly from the handshake messages (ConnectionInfo or whatever might possibly come out of ensime/ensime-server#1552 in the future, it appears that hasn't been resolved yet). That's cleanly agnostic to the launcher strategy.
This implies that handshake is completed before an EnsimeClient gets instantiated for the appropriate protocol version. That would be a change (subject of a new enhancement, if we agree on this design), but it sounds like a sensible one to me. It would get trickier if this becomes async, but how much so is hard to say since those specifics aren't decided/implemented.
Maybe I'm making too big a deal out of avoiding stuff like parsing version out of the jars… clearly it isn't hard to do that, I'm just afraid it leads to multiple launch strategy solutions that are somewhat fragile, and internal coupling that can be avoided—I think there might be conscious reasoning in the fact that server version isn't expressly encoded in .ensime, like that a running server should be the authoritative source of truth for protocol version, not .ensime, nor the build tool, nor a launcher that's finished its job of opaquely invoking java with a classpath.
Thoughts?
If this sounds like the way to go, then I'll create a new ticket for making client protocol selection automatic based on handshake, and g:ensime_server_v2 will be removed once that gets done.
There was a problem hiding this comment.
Hmm, you have all valid points, getting the version from the ConnectionInfo sound perfect, even more, we need the protocol version no the server version, it's only that this time internal change to the server were the perfect excuse to also change the protocol, but that could not be the case in the future.
As you said, the problem I see is that handling websocket messages is currently the main responsibility of the client, so we have a chicken and egg problem here due to the current implementation of the client relying on inheritance to pick the protocol version. IMHO the client should be only one, having all the WS machinery and after shacking hands with the server it should instantiate the proper version of protocol handlers.
Go ahead and create the ticket, the change looks big enough to be out of scope of this PR.
There was a problem hiding this comment.
Sorry for the delay, couldn't spare time during the week—I created #355 and I'm going to try to wrap this PR up with some tests today.
the problem I see is that handling websocket messages is currently the main responsibility of the client, so we have a chicken and egg problem here due to the current implementation of the client relying on inheritance to pick the protocol version. IMHO the client should be only one, having all the WS machinery and after shacking hands with the server it should instantiate the proper version of protocol handlers.
Yep, I'm not sure about what's going to work best here yet either. It'd be nice if responsibility for dealing with connection/messages didn't spread, but it still seems to me like mixins are the most manageable way to deal with protocol versions (open to ideas though). From my POV, I think it will be easier to decide what feels right after knocking out #325 and other server lifecycle aspects that will achieve some clean-up refactoring around these parts of the code.
In my separate ensime-py client library effort I began modeling a Connection as an explicit object—that could end up being something that knows the protocol version for the connection it represents, so maybe that could be a clean way for connections to be started and then passed into the constructor of client instances for subsequent communication. That is a long way from being ready or fully thought out, though 😣
|
Hey @ches I've been trying to give this PR a shot but something is preventing the rplugin from being registered in neovim, I'm on 0.2-dev installed via PPA. |
ches
force-pushed
the
dotensime-launcher
branch
from
c208f87 to
c8b442e
Compare
|
@ktonga Okay I reproduced that, thanks for the report. It looks like it's similar to the problem before where using the I've just pushed an update that reverts that change. Maybe it would be better for avoiding surprises to make |
ches
force-pushed
the
dotensime-launcher
branch
from
c8b442e to
fefc881
Compare
ches
changed the title
|
I finally got around to adding some initial unit tests that cover at least the new stuff on this PR (updated on the branch now). As we all know the installation part of the sbt bootstrap strategy is unpleasant to unit test so I leave that matter for #312 and/or the day we remove support for it altogether… I'd like to give |
Types > strings. Minor aesthetic renaming, bump bootstrap sbt-coursier.
ensime-sbt now installs the server and puts all the relevant jar paths for launching into .ensime, so we don't have to be bothered with doing the installation. This adds support for launching this way, keeping the bootstrap server installation around for an upgrade period. Closes #353.
ches
force-pushed
the
dotensime-launcher
branch
from
fefc881 to
272ba49
Compare
CI infrastructure has been under maintenance so #359 didn't get checked.
Since #354 it was possible that we checked for the presence of an assembly jar in the config directory before ever creating that dir ourselves. This should fix #363.
This updates
EnsimeLauncherto use the server preinstalled by ensime-sbt 1.12+ if available, as per #353. This should be pretty much ready, just want to get feedback/testing and possibly add some unit tests in the meantime now that the launcher doesn't have to do a bunch of slow side-effecty installation stuff that is obnoxious to test or stub.I've refactored the launcher to use a strategy pattern for launching/installing with three strategies, in order of precedence:
.ensimetells us how.SbtBootstrap😄One change to note: I removed the ability to specify that a bootstrap installation should use server v2. With the new ensime-sbt an
ensimeVersionsetting can be given in anensime.sbtto achieve this much more conveniently. I assume anyone who wants to use server v2 at this point is an ensime-vim developer and you don't mind using sbt and the latest ensime-sbt on your plugin testing project, so it's nice to get rid of this special-case handling.I think it would be nice to integrate a bit of feedback in the launcher for the user to know what's being used (if their assembly jar is picked up, for instance), either via a logger or in the Vim messages that already announce the server is starting, etc., but IMO that's out of scope for this PR.