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

plugin-gradle: Build shadow jar and relocate jgit #1991

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rickard-von-essen
Copy link

@rickard-von-essen rickard-von-essen commented Jan 10, 2024

This builds a shadow jar with all dependencies bundled in the plugin and relocates most dependencies jgit under shadow. This will fix issues where other Gradle plugins uses incompatible versions of the same dependencies.

Fixes #587

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
@rickard-von-essen rickard-von-essen marked this pull request as draft January 10, 2024 14:44
@rickard-von-essen
Copy link
Author

I'll do some further testing before this is ready.

Comment on lines 40 to 57
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'
Copy link
Contributor

@Goooler Goooler Jan 11, 2024

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?

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor

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
@rickard-von-essen rickard-von-essen changed the title plugin-gradle: Build shadow jar and relocate most dependencies plugin-gradle: Build shadow jar and relocate jgit Jan 11, 2024
@rickard-von-essen rickard-von-essen marked this pull request as ready for review January 11, 2024 13:13
@rickard-von-essen
Copy link
Author

This works for me™️

@nedtwigg
Copy link
Member

I think that everything that gets embedded should have a prefix, relocationPrefix 'spotless.shadow'.

If JGit is the problematic dep, it could be just these that get embedded

  • spotless-lib
  • spotless-lib-extra
  • jgit

Or include everything except the org.eclipse & equo, and leave the others out?

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
@rickard-von-essen
Copy link
Author

So I updated this to now:

  • Bundle JGit and it's deps
  • Bundle com.diffplug.durian:* and it's deps
  • Relocate org.eclipse.jgit into spotless.shadow.org.eclipse.jgit
  • Include spotless-lib and spotless-lib-extra normal deps, since they include all deps that are problematic to bundle.

The built plugin works and resolves the issues with clashing JGit versions between plugins, but this breaks almost all tests in plugin-gradle with this error:

java.lang.NoClassDefFoundError: com/diffplug/spotless/Jvm

And I couldn't figure out how to resolve that 😞

@nedtwigg
Copy link
Member

If we shade a transitive (like JGit), we have to shade all the code which calls into it (spotless-lib-extra) so that lib-extra can updated to look for shadow.jgit.

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 spotless-lib-extra and jgit without shading all of the others, we would be in business. One hacky way I can imagine doing something like this:

  • shade spotless-lib-extra and all its dependencies
  • prefix for spotless-lib-extra is shadow.spotlessextra
  • prefix for jgit is shadow.jgit
  • no prefix for everything else
  • then, in the classfile, remove every class except shadow.*
  • then, in the pom, add back the jars we need that got removed by shadow

@nedtwigg
Copy link
Member

@nedtwigg nedtwigg added the pr-archive PRs which are still valid but have gotten stuck for some reason label Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-archive PRs which are still valid but have gotten stuck for some reason
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider shading the JGit dependency in Grade plugin
3 participants