org.eclipse.lsp4j should not be optional

[stbischof]

While starting org.eclipse.xtext.web.servlet.XtextServlet in my OSGi Application i got a

java.lang.ClassNotFoundException: org.eclipse.lsp4j.WorkspaceEdit cannot be found by org.eclipse.xtext.ide_2.28.0.v20220829-0438

Stack:

com.google.inject.internal.DeclaredMembers.getDeclaredMethods(DeclaredMembers.java:48)                                                 
    at com.google.inject.spi.InjectionPoint.getDeclaredMethods(InjectionPoint.java:804)                                                       
    at com.google.inject.spi.InjectionPoint.getInjectionPoints(InjectionPoint.java:723)                                                       
    at com.google.inject.spi.InjectionPoint.forInstanceMethodsAndFields(InjectionPoint.java:423)                                              
    at com.google.inject.internal.ConstructorBindingImpl.getInternalDependencies(ConstructorBindingImpl.java:173)                             
    at com.google.inject.internal.InjectorImpl.getInternalDependencies(InjectorImpl.java:671)                                                 
    at com.google.inject.internal.InjectorImpl.cleanup(InjectorImpl.java:628)                                                                 
    at com.google.inject.internal.InjectorImpl.initializeJitBinding(InjectorImpl.java:614)                                                    
    at com.google.inject.internal.InjectorImpl.createJustInTimeBinding(InjectorImpl.java:943)                                                 
    at com.google.inject.internal.InjectorImpl.createJustInTimeBindingRecursive(InjectorImpl.java:863)                                        
    at com.google.inject.internal.InjectorImpl.getJustInTimeBinding(InjectorImpl.java:301)                                                    
    at com.google.inject.internal.InjectorImpl.getBindingOrThrow(InjectorImpl.java:224)                                                       
    at com.google.inject.internal.InjectorImpl.getInternalFactory(InjectorImpl.java:949)                                                      
    at com.google.inject.internal.FactoryProxy.notify(FactoryProxy.java:48)                                                                   
    at com.google.inject.internal.ProcessedBindingData.runCreationListeners(ProcessedBindingData.java:60)                                     
    at com.google.inject.internal.InternalInjectorCreator.initializeStatically(InternalInjectorCreator.java:137)                              
    at com.google.inject.internal.InternalInjectorCreator.build(InternalInjectorCreator.java:110)                                             
    at com.google.inject.Guice.createInjector(Guice.java:87)                                                                                  
    at com.google.inject.Guice.createInjector(Guice.java:69)                                                                                  
    at com.google.inject.Guice.createInjector(Guice.java:59)                                                                                  
    at my.project.dsl.xtext.web.MyDslWebSetup.createInjector(MyDslWebSetup.java:20)                                                       
    at my.project.dsl.xtext.MyDslStandaloneSetupGenerated.createInjectorAndDoEMFRegistration(MyDslStandaloneSetupGenerated.java:23)       
    at my.project.mydsl.xtext.web.MyDslServlet.activate(MyDslServlet.java:39)                                                               
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)                                                         
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)                                       
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)                               
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)                                                                             
    at org.apache.felix.scr.impl.inject.methods.BaseMethod.invokeMethod(BaseMethod.java:245)                                                  
    at org.apache.felix.scr.impl.inject.methods.BaseMethod.access$500(BaseMethod.java:41) 

But while resolving all dependencies -no bundle is missing. This is because of the optional resolution. org.eclipse.lsp4j;bundle-version="[0.15.0,0.16.0)";resolution:=optional

https://github.com/eclipse/xtext-core/blob/896f7136d8cfedf0daeec2fae671fb14a4b81da1/org.eclipse.xtext.ide/META-INF/MANIFEST.MF#L14

I think optional is not used here correctly. Would be better to extract the lsp4j related parts into extra bundle to avoid this.

[cdietrich]

The dependency is optional as the ide project is also used by the ui project and thus in eclipse where lsp is not in use Have no clue why this not applying in the web project

do you see why it tries to load workspace edit at all

i dont think we have the capacity to split up the code even more to have an lsp project too

[stbischof]

Thank you for the fast response. I see your point. but i think this is an mis-use of the optional.

As described in https://docs.osgi.org/specification/osgi.core/8.0.0/framework.module.html#i2548181

A bundle can indicate that it does not require a package to resolve correctly, but it may use the package if it is available. For example, logging is important, but the absence of a log service should not prevent a bundle from running. 

in your case the absense of lsp4j prevents the bundle from running.

This is why i think it might not live in the ide bundle.

I will have i look where i tries to load workspaceedit.

[stbischof]

public class MyDslWebSetup extends MyDslStandaloneSetup {

    @Override
    public Injector createInjector() {
        return Guice.createInjector(Modules2.mixin(new MyDslRuntimeModule(), new MyDslIdeModule(), new MyDslWebModule()));
    }

}

then: class MyDslIdeModule extends AbstractMyDslIdeModule

then an import of RenameService2

import org.eclipse.xtext.ide.server.rename.RenameService2;
public abstract class  AbstractMyDslIdeModule {

RenameService2 imports a lot of lsp4j: org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/rename/RenameService2.java

[stbischof]

i dont think we have the capacity to split up the code even more to have an lsp project too

and if others have the capability would you be open to PR?

• split lsp bundles
• xtext-osgi-example
• Manifest enhancements

[cdietrich]

as i am not sure how dead the web part is anyway i dont know. i also dont know if splitting up the code even on more plugins will confuse users even more. i am also not sure if RenameService2 is meant to be used inside web

@szarnekow @kthoms what do you think

[szarnekow]

@stbischof The offer is greatly appreciated but similar to what @cdietrich said, I've doubts that this is time well spent.

Splitting up the bundles is unlikely to be fully backwards compatible for existing users. It'd be not desirable to change the bundles in a way that users would be confronted with a more involving dependency tree.

As you can see from the plenty of usages of the ide bundle, the lsp4j dependency is not strictly mandatory. If you decide to use it in a context where lsp4j dependent classes are referenced, you'll have to make sure that your bundle cannot be installed without lsp4j being present. That's the state of affairs and I've low hopes that this will change soonish.

There might be a middle ground here with proper export-package and uses clauses but these cannot be applied throughout the codebase due to some unfortunate design decisions that date back to 2008.

TLDR; If you see a strictly backwards compatible way (as in: existing languages continue to work and new language are still easy to develop), please share your ideas. For now, having a dependency to lsp4j in your own bundles will ensure that ide is correctly resolved.

[stbischof]

If you decide to use it in a context where lsp4j dependent classes are referenced, you'll have to make sure that your bundle cannot be installed without lsp4j being present.

You are the bundle deployer. I will not take cake in my bundles for thinks you have to care.

There might be a middle ground here with proper export-package and uses clauses but these cannot be applied throughout the codebase due to some unfortunate design decisions that date back to 2008.

Could you please explain why it could not be applied and which design decisions?

Will see where the project brings me. Thank you for the current state of xtext insides.

[szarnekow]

Coming back to the issue at hand: TIL that OSGI wiring in the context of optional dependencies is not as predictable as I thought it is. The mere presence of org.eclipse.lsp4j does not mean that xtext.ide will actually be wired to it. We'll have to revisit the approach being taken here.

[stbischof]

if you need support for using bnd to create proper bundle-metadata (with and without tycho) just ping me.

[szarnekow]

The most challenging task is to make this backwards compatible. In case you do already have an idea, please let me know.

[stbischof]

My first thought was to split the jars in a way that it solves multiple issues

• just interfaces
• abstract Implementation without injection
• injection with guice
• injection with osgi declarative services
• Eclipse Support
• language server protocol Support

Have a project for each of this. And build it with bnd.

And the current legacy jars are then just aggregations. Interfaces + abstract impl + guice + Eclipse +lsp(optional) build with tycho

And new projects could use the separated jars.

Split packages are the major issue of this approach

[cdietrich]

i am not sure if ending up with additional 200 projects will be helpful from a maintenance perspective

[stbischof]

Guten Morgen Christian,

Yep. 200 would break your neck.

But maybe this could be handled in < 10.

[cdietrich]

a rough grep gives 63 different MANIFEST files that uses the optional directive

[szarnekow]

Would it suffice if we'd have proper uses clauses on the exports of the packages? I guess it would be a first big step forward.

[szarnekow]

Would it suffice if we'd have proper uses clauses on the exports of the packages? I guess it would be a first big step forward.

Scratch that. As far as I understand from the spec, it wouldn't impact the wiring.

I'd still be an improvement for all bundles that use import-package, though, if they consume xtext artifacts.

[cdietrich]

this also affects non osgi case, but maven (SNAPSHOTS?!?) only

and not renameservice2 with Either3 only but java.lang.NoClassDefFoundError: org/eclipse/lsp4j/WorkspaceEdit at java.base/java.lang.Class.getDeclaredMethods0(Native Method) too