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

Fix exclusion for LanguageServer uber jar assembly #3143

Closed
wants to merge 1 commit into from

Conversation

HannesWell
Copy link
Contributor

In the maven-dependency-plugin's 'excludeArtifactIds' the artifactId has to
be specified, not the bundle's Symbolic name.
Also artifacts not present anway.

Also exclude all *.java files in the shade plugin configuration to ensure no java source-files are included in any case (the maven-dependencies-plugin also copies some eclipse source-jars).

In the maven-dependency-plugin's 'excludeArtifactIds' the artifactId has
to
be specified, not the bundle's Symbolic name.
Also artifacts not present anway.

Also exclude all *.java files in the shade plugin configuration to
ensure no java source-files are included in any case (the
maven-dependencies-plugin also copies some eclipse source-jars).
@HannesWell
Copy link
Contributor Author

I wonder if we can replace the usage of the com.googlecode.addjars-maven-plugin:addjars-maven-plugin with something that has not been last released over 10years ago and is more known (I personally never saw that plugin before and it's hard to find any doc)?

Maybe we can use the antrun plugin to generate a list of copied dependencies and feed that to the shade's plugin extraJars configuration instead?.

@cdietrich
Copy link
Contributor

I have no internas on how this is working. And how a state of the art solution would look like

org.apache.commons.lang,
org.apache.commons.logging,
icu4j,
commons-logging,
Copy link
Contributor

Choose a reason for hiding this comment

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

hmmmm in my tests this was not sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From this comment I assume you only checked the ls jar didn't you?
#3137 (comment)

But to me it looks like the maven-dependencies-plugin only copies to the target/libs folder whoose content is then installed as dependency and then shaded into the uber jar.
And the shading seems to consider all dependencies (I'm not on expert on this), which again includes e.g. commons-logging.
I have to admit I don't know what the ultimate goal was with the exclusion, but it definitively prevents the copying to the target/libs folder. It currently does not prevent the inclusion into the ls jar.

Copy link
Contributor

@cdietrich cdietrich Aug 11, 2024

Choose a reason for hiding this comment

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

yes (jar tf), but on the base of your xtext/commons-logging branch

Copy link
Contributor

Choose a reason for hiding this comment

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

(start runtime eclipse, create new xtext project)

@cdietrich
Copy link
Contributor

cdietrich commented Aug 11, 2024

in general i am not sure how properly this was ever tested.
e.g. running the jar gets

java -jar ./org.xtext.example.mydsl.ide/target/org.xtext.example.mydsl.ide-1.0.0-SNAPSHOT-ls.jar
Exception in thread "main" java.lang.NoClassDefFoundError: org/eclipse/xtext/xbase/lib/Pair
	at java.base/java.lang.Class.getDeclaredMethods0(Native Method)
	at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3402)
	at java.base/java.lang.Class.getDeclaredMethods(Class.java:2504)
	at com.google.inject.internal.DeclaredMembers.getDeclaredMethods(DeclaredMembers.java:48)
	at com.google.inject.spi.InjectionPoint.getDeclaredMethods(InjectionPoint.java:813)
	at com.google.inject.spi.InjectionPoint.getInjectionPoints(InjectionPoint.java:732)
	at com.google.inject.spi.InjectionPoint.forInstanceMethodsAndFields(InjectionPoint.java:432)
	at com.google.inject.internal.ConstructorBindingImpl.getInternalDependencies(ConstructorBindingImpl.java:177)
	at com.google.inject.internal.InjectorImpl.getInternalDependencies(InjectorImpl.java:686)
	at com.google.inject.internal.InjectorImpl.cleanup(InjectorImpl.java:629)
	at com.google.inject.internal.InjectorImpl.initializeJitBinding(InjectorImpl.java:615)
	at com.google.inject.internal.InjectorImpl.createJustInTimeBinding(InjectorImpl.java:990)
	at com.google.inject.internal.InjectorImpl.createJustInTimeBindingRecursive(InjectorImpl.java:902)
	at com.google.inject.internal.InjectorImpl.getJustInTimeBinding(InjectorImpl.java:302)
	at com.google.inject.internal.InjectorImpl.getBindingOrThrow(InjectorImpl.java:225)
	at com.google.inject.internal.InjectorImpl.getInternalFactory(InjectorImpl.java:996)
	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 org.eclipse.xtext.ide.server.ServerLauncher.launch(ServerLauncher.java:47)
	at org.eclipse.xtext.ide.server.ServerLauncher.main(ServerLauncher.java:42)
Caused by: java.lang.ClassNotFoundException: org.eclipse.xtext.xbase.lib.Pair
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
	... 25 more

it is here
jar tf org.xtext.example.mydsl.ide/target/libs/org.eclipse.xtext.xbase.lib-2.36.0.v20240811-0651.jar | grep Pair
org/eclipse/xtext/xbase/lib/Pair.class

but not here
jar tf ./org.xtext.example.mydsl.ide/target/org.xtext.example.mydsl.ide-1.0.0-SNAPSHOT-ls.jar | grep lib/Pair

unclear what jar comes from the addjars and what from the copy deps

@cdietrich
Copy link
Contributor

i assume this is cause its no longer a transitive dependency of lsp4j

@cdietrich
Copy link
Contributor

created #3145
but we might really look into a better solution
#3146

@HannesWell
Copy link
Contributor Author

created #3145
but we might really look into a better solution
#3146

Absolutely. But first it would be good to understand the full intend. Also for the app setup.
Can anyone of the original creators explain that?
As far as I can tell, this change at least fixes the copy-dependencies goal.

@cdietrich
Copy link
Contributor

@kthoms added that there is two usecases

  • build an uber jar with all dependencies
  • build something like a gradle application you can run via a "mydsl-ls"

@kthoms do you remember the background on the addjars plugin?

@cdietrich
Copy link
Contributor

With the other pr this one is obsolete. Thx @HannesWell

@cdietrich cdietrich closed this Aug 11, 2024
@HannesWell HannesWell deleted the fix-uber-jar-build branch August 11, 2024 20:31
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.

2 participants