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

Do not require o.e.xtend.lib #2416

Merged
merged 1 commit into from
Feb 1, 2023
Merged

Conversation

akurtakov
Copy link
Contributor

It's not actually used thus only causes couple more bundles ending in the product for nothing.

@rgrunber
Copy link
Contributor

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.

@rgrunber
Copy link
Contributor

From another language server that's pure Java.

[INFO] +- org.eclipse.lsp4j:org.eclipse.lsp4j:jar:0.14.0:compile
[INFO] |  \- org.eclipse.lsp4j:org.eclipse.lsp4j.generator:jar:0.14.0:compile
[INFO] |     \- org.eclipse.xtend:org.eclipse.xtend.lib:jar:2.16.0.M1:compile
[INFO] |        +- org.eclipse.xtext:org.eclipse.xtext.xbase.lib:jar:2.16.0.M1:compile
[INFO] |        \- org.eclipse.xtend:org.eclipse.xtend.lib.macro:jar:2.16.0.M1:compile
[INFO] +- org.eclipse.lsp4j:org.eclipse.lsp4j.jsonrpc:jar:0.14.0:compile

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.

@akurtakov
Copy link
Contributor Author

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.

@rgrunber
Copy link
Contributor

rgrunber commented Jan 24, 2023

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.

@jonahgraham
Copy link

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

@rgrunber rgrunber removed this from the End February 2023 milestone Jan 24, 2023
@mickaelistria
Copy link
Contributor

Does it mean that this change is effectively no-op as it keeps being transitively installed as a dependency?

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.

@rgrunber
Copy link
Contributor

rgrunber commented Jan 31, 2023

@jonahgraham , just to clarify, if all we're using is org.eclipse.lsp4j & org.eclipse.lsp4j.jsonrpc (say 0.12.0, but eventually 0.19.0), is there any reason for us to keep a dependency to org.eclipse.xtend.lib ? As far as I can tell, neither the above lsp4j jars use xtend.lib. The language server seems to respond to requests without any issues/failures when it's left out of the runtime, and even jdeps doesn't show any imports to it.

Even a simple p2 installation leaves xtend.lib out :

$ p2iu https://download.eclipse.org/lsp4j/updates/releases/0.12.0/,https://download.eclipse.org/releases/2022-06 org.eclipse.lsp4j.jsonrpc/0.12.0.v20210402-1305,org.eclipse.lsp4j/0.12.0.v20210402-1305
Installing org.eclipse.lsp4j.jsonrpc 0.12.0.v20210402-1305.
Installing org.eclipse.lsp4j 0.12.0.v20210402-1305.
Operation completed in 8756 ms.
$ ls -1
com.google.gson_2.8.9.v20220111-1409.jar
com.google.guava_30.1.0.v20210127-2300.jar
org.eclipse.lsp4j_0.12.0.v20210402-1305.jar
org.eclipse.lsp4j.jsonrpc_0.12.0.v20210402-1305.jar
org.eclipse.xtext.xbase.lib_2.27.0.v20220530-0353.jar

@jonahgraham
Copy link

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

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgrunber rgrunber added this to the Mid February 2023 milestone Jan 31, 2023
It's not actually used thus only causes couple more bundles ending in
the product for nothing.
@testforstephen
Copy link
Contributor

CC'ing @testforstephen for awareness in case some other language server depending on JDT-LS may take advantage of this

Had a search in java extensions we owned, we didn't explicitly use this lib. Feel free to merge.

@rgrunber rgrunber merged commit 5091b3c into eclipse-jdtls:master Feb 1, 2023
@cdietrich
Copy link

cdietrich commented Feb 13, 2023

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

Successfully merging this pull request may close these issues.

6 participants