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

The Gradle plugin shades in a number of libraries that should be repackaged #3896

Closed
ZacSweers opened this issue Feb 16, 2023 · 9 comments · Fixed by #4079
Closed

The Gradle plugin shades in a number of libraries that should be repackaged #3896

ZacSweers opened this issue Feb 16, 2023 · 9 comments · Fixed by #4079
Labels

Comments

@ZacSweers
Copy link
Contributor

SQLDelight Version

2.0.0-alpha05

Operating System

MacOS Ventura

Gradle Version

7.6

Kotlin Version

1.8.10

Dialect

N/A

AGP Version

7.4.1

Describe the Bug

There are a bunch of transitive dependencies from the intelliJ core dependency that are repackaged into the gradle plugin jar but not relocated to another package. This causes classloader conflicts with other plugins and their dependencies, such as JNA. It would be ideal if the plugin could relocate any shaded packages where possible to avoid conflicts.

image

Stacktrace

No response

Gradle Build Script

No response

@ZacSweers ZacSweers added the bug label Feb 16, 2023
@hfhbd
Copy link
Collaborator

hfhbd commented Feb 16, 2023

Do you only have issues with jna (or other dependencies)? I think, we could fixing jna, we only added it to fix some very strange integration tests using android simulators.
Relocation would change the class names, won't it? So com.intellij would move to shade.com.intellij (for example) and we need to change the class names of intellij dependencies used by the dialects, don't we?

@AlecKazakova
Copy link
Collaborator

instead of adding these transitive dependencies directly I wonder if we could add them to the specific task classpath and avoid any potential buildscript classpath conflicts

conceptually that kinda makes sense too because the plugin itself doesnt use it at all, just the compiler / schema generation task

@AlecKazakova
Copy link
Collaborator

and yea i remember now thats why we stopped shading transitive dependencies, because then the dialects would need to also be shaded, and that would make them incompatible with IDE environments

@AlecKazakova
Copy link
Collaborator

i guess the next question is...why did we need to fatjar jna in at all vs just using a normal maven dep

@hfhbd
Copy link
Collaborator

hfhbd commented Feb 16, 2023

Yeah, adding them to the task classpath only sounds the best idea but we still need to shade them and to publish the shaded deps as separate artifact to not add the IntelliJ repos to each consumer project.

@hfhbd
Copy link
Collaborator

hfhbd commented Feb 16, 2023

We don't shade jna explitly, it is a transitive dependency of intellij-core.

@hfhbd
Copy link
Collaborator

hfhbd commented Feb 16, 2023

Another benefit creating a intellij-env module: this would make implementing dialects easier because we would have a supported env now.
"Use this tested artifact which contains all IntelliJ artifacts used by sqldelight" instead looking into the documentation/build script and check the used deps, exclude some modules and add the custom repos by yourself.

@AlecKazakova
Copy link
Collaborator

thats interesting....I wonder if we could accomplish that with a BOM instead of a full module?

I think we can shade all the intellij dependencies, but rely on normal maven resolution for transitive ones like jna? I guess that is why you have it set up the way it is now with an explicit dependency on jna, so it doesn't get shaded in but still gets resolved by gradle?

@hfhbd
Copy link
Collaborator

hfhbd commented Feb 16, 2023

AFAIK you can't shade something with BOM only.

I added the explicit dependency of jna because the one of IntelliJ 5.6 didn't work with the instrumentation tests on the android simulator, so I overwrite the version to 5.9.

So if only JNA is a problem, we could simply shade it too.

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

Successfully merging a pull request may close this issue.

3 participants