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

Embed language server instead of starting new JVM #6

Closed
wants to merge 6 commits into from

Conversation

ddekany
Copy link
Contributor

@ddekany ddekany commented Mar 27, 2018

Only look at the last commit. The ones before it belong to the earlier PR (random small things).

This way we can avoid the mess which is starting a new JVM... Also it's more logical IMO.

The hard part was really to pull in the dependency, as I'm new to Tycho... but see commit message. I guess the CI will fail, as now it needs to see the Maven artifacts from the freemarker-languageserver. That's something I can't solve of course.

ddekany added 5 commits March 27, 2018 22:15
…ally amateurish, but that's how I managed with it so far.
…that doesn't start a separate OS process (a separate JVM), but runs the language server embedded. This in itself was simple. But, because of this, I had to pull in the org.freemarker:freemarker-languageserver dependency somehow (without including binaries in the source code). So I made an OSGi über-bundle over there, then made Tycho pull it in from the standard Maven (m2) repo mechanism. I'm not sure what's the best way of pulling in that dependency is, maybe not this... but for now I just wanted to try if an embedded language server works quickly.
@angelozerr
Copy link
Owner

@ddekany I love your idea with LocalStreamConnectionProvider (if it works well you could create a gerrit patch to lsp4e) but the part with target/target-platform-pde.target seems a little complex.

I tell me that if /freemarker-languageserver-cli, freemarker-languageserver-core will contains a META-INF/MANISFEST.MF it could work, no?

@ddekany
Copy link
Contributor Author

ddekany commented Mar 28, 2018

/freemarker-languageserver-cli is not needed for the Eclipse pugin, only /freemarker-languageserver-core. And /freemarker-languageserver-bundle does contain MANIFEST.MF (and all the dependencies inside that OSGi bundle). So the question is not that, but if where does lsp4e-freemarker get that bundle. I did it through Maven dependency, but maybe I should instead deploy to some local p2 repository, and then target/target-platform-pde.target (which is only needed for development, not for the maven build) is not needed. Any help with that is welcome. I really just wanted the embedded server to work first, so maybe I haven't yet found the best approach. What's the best practice on this field?

BTW, if this comes to Apache (not sure if you want that), then freemarker-languageserver and the lsp4e-freemarker part most certainly would be in the same Git repository, to simplify the release process. They would still produce separate artifacts, so if someone just needs the CLI for example, he can grab only that. It also make sense because for now the only consumer of freemarker-languageserver is lsp4e-freemarker, so versioning them together makes sense. I'm saying this here as this might mean that we can just put all these under a single big Maven project (i.e., they have common parent), which can mean that the CI can just verify that project, and then it's easier to organize that lsp4e-freemarker finds freemarker-languageserver.

@angelozerr
Copy link
Owner

@ddekany after thinking about using LocalStreamConnectionProvider, I'm not sure it's a good idea. The LocalStreamConnectionProvider fixes the problem with JVM but we must keep in mind that Freemarker Language Server must work on VSCode too.

More with LocalStreamConnectionProvider you are linked to a freemarker version. With a cli, you can set in the classpath the freemarker jar that you wish to use. It means that with the cli solution you could use the freemarker.jar from the user Eclipse Project and have validation with the well version of FreeMarker.

/freemarker-languageserver-cli is not needed for the Eclipse pugin, only /freemarker-languageserver-core.

I tell me if it's really interesting to have several JARs for that. Having a jar (cli) with just a simple main class is shame I find. IMHO I think it should better to have one JAR with different package.

What's the best practice on this field?

I think the 3 projects freemarker-languageserver* must be an OSGi bundle (just contains a MANIFEST.MF). If we have one project, I think it should be enough.

BTW, if this comes to Apache (not sure if you want that)

I'm aware with anything if it will motivate other people to work on Freemarker Language Server. Today the main work is in the Freemarker Language Server.

freemarker-languageserver and the lsp4e-freemarker part most certainly would be in the same Git repository

I have not done that for the moment because I would like to VSCode consume it dcortes92/vs-freemarker#15

@ddekany here my proposition. I think we should not consume our time to organize projects but consume our time with angelozerr/freemarker-languageserver#1

Our PR brings advantage like fix problem with java version, could consume JDT in the language server, but loose features like using custom freemarker.jar. We will have too some work with your PR to fix the update site.

In otherwise, my idea is to keep the lsp4e-freemarker like today and will see in the future if we need this split and using the freemarker language server in the same JVM.

Are you OK with that? The angelozerr/freemarker-languageserver#1 is the most important issue I think.

Hope you will understand what I mean and you will keep motivation to help me.

@ddekany
Copy link
Contributor Author

ddekany commented Mar 28, 2018

LocalStreamConnectionProvider fixes the problem with JVM but we must keep in mind that Freemarker Language Server must work on VSCode too.

That's why freemarker-languageserver-cli exists. You haven't lost any functionality, it was just split up to two parts (plus the OSGi über-bundle).

More with LocalStreamConnectionProvider you are linked to a freemarker version.

If that's a problem, then the earlier server/freemarker-languageserver-all.jar solution had the same problem. You have to decide if it's the plugin that contains and launches the language server, or the user is expected to set up and start the language server outside our plugin, and the plugin just connects to that. Only in the last case can the user replace freemarker.jar in it, as far as I see. Actually, we can support both cases on the long run, but certainly almost all users will want the solution that "just works", that is, use an embedded language server. As far as the user is concerned, both server/freemarker-languageserver-all.jar and LocalStreamConnectionProvider is a such solution (and in neither case can they replace freemarker.jar, at least not without doing dirty things inside the plugin directory). But I believe that among this two embedded solutions, LocalStreamConnectionProvider is certainly more reliable, and simpler for the plugin developers as well. Again, a non-embedded solution (which is when the user starts the freemarker-languageserver-cli jar directly) is a different topic, and is still possible, just as it was earlier.

I tell me if it's really interesting to have several JARs for that. Having a jar (cli) with just a simple main class is shame I find.

It's not relevant to our current problems as far as I see (ie., how is it best to make the language server a dependency of the lsp4e-freemarker). And freemarker-languageserver-cli will do more that's not relevant for the plugin, like will have command line parameters, a different logging implementation (logback most certainly), and will have to generate an executable über-jar (assembly). I think whenever you have opportunity to separate specialized functionality like that (CLI), it helps keeping the code base clean.

I think the 3 projects freemarker-languageserver* must be an OSGi bundle

So, I believe that doesn't help us. We already have an OSGi bundle (freemarker-languageserver-bundle), which furthermore packs all its dependencies (which a normal jar that's also an OSGi bundle, like freemarker.jar, doesn't and shouldn't do). My question was what's the best practice to depend on something that's not in the Orbit or similar well-known p2 repos, because you are building it. As I don't know, and wanted a quick solution, I have just used the local m2 repo instead. But the problem I have faced is that while Tycho can deal with that, the Eclipse IDE Tycho plugin can't. That can only use p2 repos... so I have this hack where we generate that outher traget file just for Eclipse. I want to avoid that. This is really a totally generic Tycho + Eclipse IDE question, not specific to our project.

I have not done that for the moment because I would like to VSCode consume it dcortes92/vs-freemarker#15

As far as I see, that works fine even if you have a single Git repo that generates a dozen of artifacts. Because all they you need (I guess...) is an artifact from the CLI module that's an über jar as well.

BTW, I suspect that uniting lsp4-freemarker with freemarker-languageserver under the same repo and common Maven parent would simplify development and releasing, unlike uniting the freemarker-languageserver modules.

Our PR brings advantage like fix problem with java version, could consume JDT in the language server, but loose features like using custom freemarker.jar.

I think that will be a key misunderstanding here. Some of us don't understand what's the other is saying. Because as far as I see, the old solution is as bad as the new one when it comes to using a custom freemarker.jar. But see earlier (both solutions embed the language server and FreeMarker). BTW, if we expose freemarker.jar as a dependency on its own (similarly as our other dependencies), doesn't Eclipse allow the user to install a different freemarker.jar first (mind you, freemarker.jar is an OSGi bundle in itself), and the user can chose not to upgrade/downgrade that when he installs the lsp4e-freemarker plugin?

We will have too some work with your PR to fix the update site.

Here again, I believe there's no difference between the old and the new solution in this. True, in the old solution you include a binary copy of the freemarker-languageserver-all.jar in the source code of lsp4e-freemarker, so you don't have to pull it as a dependency. But I assume that's just a temporary solution, and sooner or later you will have to pull that jar from some repo as well. (Or said differently, I could copy freemarker-languageserver-bundle.jar into the lsp4e-freemarker source code as well.) It's also against Apache policy to embed such binaries into the source code. So if we want to keep the CI part simple, and still have real dependencies instead copying binaries over manually, I guess we should just unite the two repo-s under a single Maven parent module, but see earlier.

Are you OK with that?

So, not really, because I think there are misunderstandings regarding the pros/cons of the two solutions.

The angelozerr/freemarker-languageserver#1 is the most important issue I think.

I have explained my standpoint over there. You see, I want to avoid bringing the old plugin to the ASF (also maintaining it in my personal fork indefinitely). Instead, I want to bring yours there. Now, the old pluging has stopped at the first error as well (furthermore, I have used it like that for ages, and while it's lame, it wasn't a significant problem in practice, because it's not Java, and an error is just typo that you can fix immediately). I only need the new plugin to be able to do what the old one did (no exactly of course, but roughly), ASAP, and without it not working for a lot of users because of dependency issues and other environment differences (the old plugin seems to be quite resilient in this regard). That's the top priority from my point of view. But, everyone works on what he is interested in, so, if you want #1, go ahead of course, it's still very useful.

@angelozerr
Copy link
Owner

If that's a problem, then the earlier server/freemarker-languageserver-all.jar solution had the same problem.

At first let's me explain how I would like to manage this freemarker.jar even if it's not possible with lsp4e for the moment. My idea is to consume the freemarker.jar from the project and otherwise consume a default freemarker.jar.

To do that we could have lsp4e-freemarker which contains 2 JARs:

  • server/freemarker-languageserver-all.jar
  • server/freemarker.jar

When we start the server/freemarker language server we will set the classpath with the 2 JARs.

Now when a project has a freemarker.jar we start the freeamrker server by setting sclasspath with server/freemarker language and $rproject/lib/freemarker.jar

With LocalStreamConnectionProvider, I'm afraid that is not possible.

My question was what's the best practice to depend on something that's not in the Orbit or similar well-known p2 repos, because you are building it.

I have already done that:

It's a long work and IMHO I think we should consume our time to support completion outline, hyperlink with language server once angelozerr/freemarker-languageserver#1 will be fixed.

Here again, I believe there's no difference between the old and the new solution in this. True, in the old solution you include a binary copy of the freemarker-languageserver-all.jar in the source code of lsp4e-freemarker, so you don't have to pull it as a dependency. But I assume that's just a temporary solution

Yes sure. My idea is to build this jar with maven and copy it when plugin must be built. If we have a JAR we can create server/freemarker language server.jar without a given freemarker.jar. So after we can choose easily the version of Freemarker to use. With LocalStreamConnectionProvider you will be linked to a freemarker.jar version (you wil lnot have the capibility to choose a freemarker.jar to use it)

But, everyone works on what he is interested in, so, if you want #1, go ahead of course, it's still very useful.

Without a FM tolerant parser we cannot manage other features like mark occurences, completion, outline, etc. IMHO it's a very important, no?

@ddekany ddekany force-pushed the pr-embedded-lsp-server branch from a266b41 to 24fe3af Compare March 28, 2018 22:32
@ddekany
Copy link
Contributor Author

ddekany commented Mar 28, 2018

Now when a project has a freemarker.jar we start the freeamrker server by setting sclasspath with server/freemarker language and $rproject/lib/freemarker.jar.

Aha, I see. That's a new challenge, and an interesting extra. Though at least before a certain FreeMarker 2 version doing this will be certainly fragile, as you keep adding features to freemarker.jar that's needed for the language server, so, we have to keep track of what the minimum FreeMarker version is.

With LocalStreamConnectionProvider, I'm afraid that is not possible.

I don't see problems there (yet?). If you want to re-load a jar (or a system of 2 dependent jar-s in this case), you need a new ClassLoader and Java reflection to call what you have loaded, but not a new JVM. So in this case, that über jar need not be an OSGi bundle after all, and we just load it with a new ClassLoader, together with the freemarker.jar, and call the single static method needed to launch the language service with Java Reflection. I'm not sure if I'm missing something. (A possibly interesting thing you gain here, apart from not launching a new JVM, is that you can pass along a log service object or such, not just JSON.)

It's a long work and

Well, the solution I have used seems to work OK as far bridging the p2 and m2 world together goes, even if it also adds some complexity to the build. Though this jar magic now gives a twist to the story, like we don't have an OSGi bundle dependency after all. Anyway, let me repeat my earlier question. If just put these into the same repo and common parent... wouldn't that simplify life? Then the freemarker-languageserver artifacts are already there in the local repo of the CI when the Eclipse plugin part is built. Also, given that these two will be certainly developed together, we could also avoid the problem where the two builds are technically independent, yet for lsp4e-freemarker to work, I need to wait for freemarker-languageserver to be deployed.

Without a FM tolerant parser we cannot manage other features like mark occurences, completion, outline, etc. IMHO it's a very important, no?

Yes, but it's just that until there's a syntax error, those features are off. Where that's truly a problem is auto-completion, because when you need it, your template is usually incomplete, and hence certainly very often has a syntax error. So the "FreeMarker IDE" plugin did not need the AST for auto-completion, and you know the core directives and built-ins without that, which is mostly why people want to auto-completion. You just look back at the text, and guess what it is (we are after a <#, so certainly it's a core directive).

@angelozerr
Copy link
Owner

Aha, I see. That's a new challenge, and an interesting extra.

Glad it pleases you.

Though at least before a certain FreeMarker 2 version doing this will be certainly fragile, as you keep adding features to freemarker.jar that's needed for the language server, so, we have to keep track of what the minimum FreeMarker version is.

Yes sure. It will certainly not work with old version of FreeMarker, but for current version of FreeMarker I think it's a feature which is very important. It's an another topic, but I think freemarker.jar should embed the language server. TypeScript works like this for instance, when you install it, it provides a tsserver.js (TypeScript server) that IDE can consume it. So your sure that your language server and version of your language are synchronyzed.

don't see problems there (yet?). If you want to re-load a jar (or a system of 2 dependent jar-s in this case), you need a new ClassLoader and Java reflection to call what you have loaded, but not a new JVM. So in this case, that über jar need not be an OSGi bundle after all, and we just load it with a new ClassLoader, together with the freemarker.jar, and call the single static method needed to launch the language service with Java Reflection.

In theory yes, but we are in OSGi context here. If I understand your idea you want to update the ClassLoader of the OSGi container to use the well freemarker.jar. I think it's an hard work and what about having several version of freemarker (for each project). IMHO I think it's a more simply to update the java command line like this:

java -jar server/freemarker-language-service.jar;${PROJECT_HOME}/lib/freemarker.jar

And that's all!

Well, the solution I have used seems to work OK as far bridging the p2 and m2 world together goes, even if it also adds some complexity to the build.

I would like just to import projects and not build a P2.

Anyway, let me repeat my earlier question. If just put these into the same repo and common parent... wouldn't that simplify life?

It will simplify, indeed. But I have done that because I hope VSCode, IJ, etc will consume it. I would like to keep that. For dev, you should just import in your workspace the freemarker-languageserver projects without creating a P2. We could even add a git submodule to the project freemarker-languageserver.

@pascalleclercq could perhaps help us to build those freemarker-languageserver project and consume it in the update site. But before that I would like just clarify the structure of the project which must be simply I think. I'm really sorry, but today I don't see the benfit to have 3 bundles. It's more clean sure, but more complex for the build (create an uber Jar etc).

What is the problem if we have just one OSGi bundle (we could be a maven project too):

  • freemarker-languageserver
  • src/ext
    * languageserver
    * cli
    META-INF/MANIFEST.MF (export languageserver and languageserver/cli)

Then the freemarker-languageserver artifacts are already there in the local repo of the CI when the Eclipse plugin part is built.

IMHO, I think Freemarker Language Server must be hosted in a another project because VSCode, IJ which could contribute doesn't want to have Eclipse code.

So the "FreeMarker IDE" plugin did not need the AST for auto-completion, and you know the core directives and built-ins without that

"FreeMarker IDE" create a custom AST (see org.jboss.ide.eclipse.freemarker.model.ItemSet) which is consumed to manage completion, outline, etc. So we need an AST and the question is how to create a tolerant AST (like ItemSet):

  • create a custom parser like ItemSet which create lsp4j structure
  • extend FMParser to have a factory system and have a FMParser#setTolerant(true)

I have experimented this feature with interpolation which is not closed by hacking FMParser like this:

/**
 * A production representing the ${...}
 * that outputs a variable.
 */
  final public DollarVariable StringOutput() throws ParseException {Expression exp;
    Token begin, end = null;
    begin = jj_consume_token(DOLLAR_INTERPOLATION_OPENING);
    exp = Expression();
notHashLiteral(exp, NonStringException.STRING_COERCABLE_TYPES_DESC);
        notListLiteral(exp, NonStringException.STRING_COERCABLE_TYPES_DESC); 
		if (!tolerant) {
			end = jj_consume_token(CLOSING_CURLY_BRACKET);
		} else {
			try {
				end = jj_consume_token(CLOSING_CURLY_BRACKET);
			} catch (Exception e) {
				// Here "}" cannot be found
				// The end token is the token which has thrown the error
				Token next = token.next;
				end = token;
				end.next = null;
				// marke this end token as end interpolation
				token_source.endInterpolation(end);
				if (next != null) {
					// move the character where token has throw an error
					token_source.input_stream.backup(next.endColumn - end.endColumn);
				}
			}
		}
...
}

As you can see it's a lot of work to manage a full tolerant parser. It seems that it is not in your priority so I think more and more to develop my custom tolerant parser (like Freemarker IDE does too). So in this case the custom use of freemarker.jar will be less important (just for validation).

@ddekany
Copy link
Contributor Author

ddekany commented Mar 29, 2018

freemarker.jar should embed the language server

The language server has a lot of dependencies, so we really can't do that with a small library like FreeMarker. Besides, monoliths are considered ugly in the Java world since Maven has become wide spread (while it still has no feature to keep the versions of the related modules in sync... oh well). We have received several complains that freemarker.jar is too monolithic (hence FM3 is more modular).

If I understand your idea you want to update the ClassLoader of the OSGi container to use the well freemarker.jar.

No, I don't care about the OSGi context here. I just plan to load the jars on the most basic way, similarly to as java -classpath would. (Unless, OSGi can actually help us, because OSGi apps used to be able to reload bundles without restart, but I don't know it that well.)

And that's all!

My main motivation was exactly that I don't expect launching a proper JVM becoming problem free (for all random users), or at least that we don't spend much more time in the future with hacking/maintaining that code even more. I should have come up with this local server POC much earlier, before you have invested into that earlier issue of mine... but don't let it becoming a lost investment fallacy. Loading and re-loading jar-s dynamically is a quite common thing on the JVM, so I guess if once I get it right (which is hopefully simple), it will keep working everywhere.

BTW, I still don't know what the problem was in my case, and thus if it won't resurface with an Eclipse-provided Java version too, for someone. Also, the current version needs more investment to find the properly high Java version. I'm not sure what Xmx should be either. A new JVM won't share the heap with the parent, has no idea how many cores it should use when it uses the fork-join pool, and so on. JVM-s are antisocial. OK, these are mostly only practical concerns on a server (I mean, as opposed to a desktop), but still, not nice. Also, with a separate JVM, you have to monitor a separate OS process (like did it crash and why), which is also adds some complexity.

Anyway, the bottom line: If I implement this with LocalStreamConnectionProvider, for now by putting back /server/*.jar, will you like it? See, I just wan to load those jar-s into the current JVM, instead of into a new JVM, so it's one less problem.

As you can see it's a lot of work to manage a full tolerant parser.

Oh, I do know... I have fixed the FreeMarker IDE parser a few years ago, rewriting quite much of it along the way (but I still kept most of the confusing nomenclature used there, so that it's easier to get the PR accepted by JBoss... maybe I shouldn't have). Plus FM2 syntax is hard to parse properly. It's really a 15 year old monstrosity (not to point a fingers at anyone... :) ), only humans don't care or notice it much. But when you load bigger templates, sometimes your parser will be derailed somewhere, where the real parser has no problem.

Anyway, what to do with that... Have you tried the approach shown in Issue #1? Could the real parser be auto-generated from the tolerant one? Because what we should avoid is maintaining two FM2 parsers. If not, then it has to be done on the hard way, and we better get that rights for the FreeMarker IDE source code.

@angelozerr
Copy link
Owner

We have received several complains that freemarker.jar is too monolithic (hence FM3 is more modular).

Ok thanks for the clarification. But I think Freemarker Language Server (without dependencies of lsp4j will be little).

Unless, OSGi can actually help us, because OSGi apps used to be able to reload bundles without restart, but I don't know it that well.

Yes sure, but never played with that in Eclipse word. To study...

My main motivation was exactly that I don't expect launching a proper JVM becoming problem free

Ok let me see how to create a clean FreeMarker Language server (to avoid using P2). I will merge all bundle in one for the moment and we will discuss after about splitting.

Could the real parser be auto-generated from the tolerant one?

Yes, there is just one FTL.jjs which have condition with "tolerant", but I'm a little discouraged with that and I think more and more developping my own parser.

Because what we should avoid is maintaining two FM2 parsers. If not, then it has to be done on the hard way, and we better get that rights for the FreeMarker IDE source code.

I will prefer developping my own parser which will work directly with lsp4j structure and will not use Eclipse structure like IRegion, etc

@ddekany
Copy link
Contributor Author

ddekany commented Mar 29, 2018

But I think Freemarker Language Server (without dependencies of lsp4j will be little).

But without the dependencies it doesn't work. So how would it look?

Ok let me see how to create a clean FreeMarker Language server (to avoid using P2). I will merge all bundle in one for the moment and we will discuss after about splitting.

Note that splitting is still orthogonal to the dependency problem. Simply you copy the über jar of freemareker-languageserver-core into server/ in lsp4e-freemarker.

BTW my main motivation for the splitting was logging (next to clarity). Not sure how that will work out with or without it, but generally if your environment-specific launcher is separate, it's easier. (It's like when you develop a some web service, and then there's a separate module that packs it into a war, and another that packs it into a Spring Boot application.)

@angelozerr
Copy link
Owner

@ddekany could you please create a new PR with only LocalStreamConnectionProvider Thanks!

@ddekany
Copy link
Contributor Author

ddekany commented Apr 6, 2018

OK, I will do a such PR today.

@angelozerr
Copy link
Owner

OK, I will do a such PR today.

Thanks @ddekany !

@mickaelistria I think the class LocalStreamConnectionProvider.java created by @ddekany https://github.com/angelozerr/lsp4e-freemarker/pull/6/files#diff-1f24cd4cadfe3ad304f0ecb34565e743 could be interesting to be hosted in lsp4e. What do you think about that?

@mickaelistria
Copy link

mickaelistria commented Apr 6, 2018 via email

@ddekany
Copy link
Contributor Author

ddekany commented Apr 7, 2018

So I assume I should create a new PR that builds on the freemarker-server-all.jar. (I couldn't do it yesterday... will do it today.) Hence I close this one.

@ddekany ddekany closed this Apr 7, 2018
@ddekany
Copy link
Contributor Author

ddekany commented Apr 7, 2018

@mickaelistria Please see the one in #7 instead. Also note that I have no idea what I'm doing (not an Eclipse plugin developer)... like if Future.cancel(...)-ing what the LSP4J launcher has returned will actually stop the background message processor. It's more like a POC really.

@ddekany ddekany deleted the pr-embedded-lsp-server branch April 8, 2018 13:33
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.

3 participants