-
Notifications
You must be signed in to change notification settings - Fork 400
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
Do not require o.e.xtend.lib #2416
Conversation
Maybe it was our intention to support the compilation of the xtend language but never go around to doing it ? I don't see us having used this. |
From another language server that's pure Java.
lsp4j depends on lsp4j-generator which brings in xtend according to the pom file metadata, https://repo1.maven.org/maven2/org/eclipse/lsp4j/org.eclipse.lsp4j/0.19.0/org.eclipse.lsp4j-0.19.0.pom . This isn't expressed on the OSGi metadata though. |
Does it mean that this change is effectively no-op as it keeps being transitively installed as a dependency? If that's the case feel free to merge or abandon as you see fit. |
Well, this change will remove xtend from the product, but only because lsp4j doesn't have a dependency to lsp4j-generator in OSGi. However in the pom files, this dependency exists (compile-time). I'm just not sure why one has it, and not the other. Maybe it is only a compile-time dep and can be safely left out at runtime ? @jonahgraham , would you know if lsp4j-generator is only needed at compile-time and safe to remove from runtime ? If so that would probably imply we could remove xtend.lib. |
AFAIU lsp4j-generator is only needed at compile time. @cdietrich has been working on removing xtend.lib at runtime, but for now it is still needed. Follow progress on that in eclipse-lsp4j/lsp4j#529 It may be the pom.xml is missing xbase from its dependency list (or relying on getting it transitively). The MANIFEST.MF for org.eclipse.lsp4j has an import package for org.eclipse.xtext.xbase.lib |
It's not really a no-op from dep management perspective. Removing direct the OSGi requirement wire from org.eclipse.jdt.ls.core to org.eclipse.xtend.lib is a small optimization and simplification; it probably won't change much per se, but it's still a win. |
@jonahgraham , just to clarify, if all we're using is Even a simple p2 installation leaves
|
I think I am in a muddle between xtend.lib and xbase.lib. Both are needed for the generator, but at runtime it is just xbase.lib that is needed. Further to that it seems that xbase.lib dependency may not be properly reported in the pom dependencies (it is correct in the OSGi deps). And Christian is working on removing the dependency to xbase.lib in eclipse-lsp4j/lsp4j#529 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this change is valid after all. CC'ing @testforstephen for awareness in case some other language server depending on JDT-LS may take advantage of this, but we really should aim to keep JDT-LS as slim as possible. @akurtakov , I would also remove the xtend.lib references in :
It's not actually used thus only causes couple more bundles ending in the product for nothing.
All requested changes done. |
Had a search in java extensions we owned, we didn't explicitly use this lib. Feel free to merge. |
i am reviving my patch would be nice to get some feedback |
It's not actually used thus only causes couple more bundles ending in the product for nothing.