-
Notifications
You must be signed in to change notification settings - Fork 458
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
plugin-gradle: Build shadow jar and relocate jgit #1991
Conversation
This builds a shadow jar with all dependencies bundled in the plugin and relocates most dependencies under shadow. This will fix issues where other Gradle plugins uses incompatible versions of the same dependencies. Fixes diffplug#587
I'll do some further testing before this is ready. |
plugin-gradle/build.gradle
Outdated
relocate 'org.eclipse.jgit', 'shadow.eclipse.jgit' | ||
relocate 'org.eclipse.osgi', 'shadow.eclipse.osgi' | ||
relocate 'org.eclipse.equinox', 'shadow.eclipse.equinox' | ||
relocate 'org.eclipse.core', 'shadow.eclipse.core' | ||
relocate 'org.osgi', 'shadow.org.osgi' | ||
relocate 'com.googlecode.concurrenttrees', 'shadow.googlecode.concurrenttrees' | ||
relocate 'com.googlecode.javaewah', 'shadow.googlecode.javaewah' | ||
relocate 'com.googlecode.javaewah32', 'shadow.googlecode.javaewah32' | ||
relocate 'dev.equo.solstice', 'shadow.equo.solstice' | ||
relocate 'dev.equo.ide', 'shadow.equo.ide' | ||
relocate 'org.apache.commons', 'shadow.apache.commons' | ||
relocate 'org.apache.felix', 'shadow.apache.felix' | ||
relocate 'org.tukaani.xz', 'shadow.tukaani.xz' | ||
relocate 'okhttp3', 'shadow.okhttp3' | ||
relocate 'okio', 'shadow.okio' | ||
relocate 'org.intellij', 'shadow.intellij' | ||
relocate 'org.jetbrains', 'shadow.jetbrains' | ||
relocate 'org.slf4j', 'shadow.slf4j' |
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.
Using relocationPrefix "shadow"
might be enough?
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.
(together with enableRelocation = true
)
I got some issues with loading some of the dependencies, like this:
Execution failed for task ':tasks'.
> Could not create task ':spotlessGroovyGradle'.
> java.io.IOException: Failed to load eclipse groovy formatter: java.lang.RuntimeException: java.lang.ClassNotFoundException: shadow.equo.solstice.p2.P2QueryResult
So I backed out of relocating most deps and instead focused on JGit only.
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.
I tried this on my fork, it works well. if I missing something? See Goooler#15.
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.
Did you test the Greclipse formatter? I would expect the Eclipse-based formatters to fail.
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.
That should be included in Maven tests on CI? Do I need some extra running?
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.
I think the Gradle and Maven integration tests are not testing against the shadowed jar. I think you have to publish to maven local to test that. Changing the PR so that at least one of the Gradle or Maven integration tests is testing using the shadowed jar would be a great way to prove me wrong.
I'm willing to merge this without such a test, but only if I can understand how it is working. I would expect the eclipse-based formatters should be "unshadeable", and shading them without changing their packages creates really painful bugs.
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.
Yeah, will try to add integration tests for using shadow jar.
Some dependencies fails to load if they are relocated. Example: Execution failed for task ':tasks'. > Could not create task ':spotlessGroovyGradle'. > java.io.IOException: Failed to load eclipse groovy formatter: java.lang.RuntimeException: java.lang.ClassNotFoundException: shadow.equo.solstice.p2.P2QueryResult
This works for me™️ |
I think that everything that gets embedded should have a prefix, If JGit is the problematic dep, it could be just these that get embedded
Or include everything except the |
Only bundle com.diffplug.durian:* and org.eclipse.jgit:org.eclipse.jgit Relocate JGit with spotless.shadow prefix. This breaks most tests in plugin-gradle with: java.lang.NoClassDefFoundError: com/diffplug/spotless/Jvm
So I updated this to now:
The built plugin works and resolves the issues with clashing
And I couldn't figure out how to resolve that 😞 |
If we shade a transitive (like JGit), we have to shade all the code which calls into it (spotless-lib-extra) so that The problem is that there are some dependencies (like everything related to Eclipse) which use reflection, so those cannot be shaded because it breaks the reflection. A quick workaround is to shade eclipse/equo stuff, but have them not have prefixes. That will blow up fast, because if the "real" jars are on the classpath from another plugin, we will get truly horrific errors, because it's random which classfiles will end up getting used. If you can shade
|
|
This builds a shadow jar with all dependencies bundled in the plugin and relocates
most dependenciesjgit
under shadow. This will fix issues where other Gradle plugins uses incompatible versions of the same dependencies.Fixes #587