Shared library 'Caffeine' with EcoSkills plugin

lokka30 commented 2 years ago

LibertyBans Version

1.0.2

I have confirmed that ...

[X] LibertyBans is up to date
[X] No similar issue has been reported

Platform

Spigot/Paper

Description

Hi A248 and others, I hope you are all well!

I wish to forward my conversation on Auxilor's Discord server which provides support for his EcoSkills plugin.

Note: this seems to be an issue with their eco library, not EcoSkills specifically (which depends on eco).

lokka30


[LibertyBans] *******************************************
Plugin 'eco 6.38.0' has a critical bug. That plugin has shaded the library 'Caffeine' but did not relocate it, which will pose problems.

LibertyBans is not guaranteed to function if you do not fix this bug in Plugin 'eco 6.38.0'

Contact the author of this plugin and tell them to relocate their dependencies. Unrelocated class detected was com.github.benmanes.caffeine.cache.Caffeine

Note for advanced users: Understanding the consequences, you can disable this check by setting the system property libertybans.relocationbug.disablecheck to 'true'



> **lokka30**
> LibertyBans and eco seem to both not relocate Caffeiene. I'm unsure how this could be resolved without unofficial patches. 
> On another note, wouldn't Gradle allow for the eco suite (and other plugins in this boat) to relocate these dependencies? 
> Could two separate plugins which do not relocate Caffeine possibly cause issues with each other?

> **_OfTeN_**
eco can't relocate kotlin and caffeine because it's a library and eco plugins use those libs from eco
LibertyBans sounds like a standalone plugin so I don't see why can't they relocate caffeine

> **Seitan**
lokka30 what did lite and devs said to you? Because we can like often said, why couldn’t they?

gepron1x commented 2 years ago

LibertyBans does not shade dependencies. Instead, it uses isolated classloader and loads libraries at runtime. Isolated classloader breaks when you have multiple classes with same name at classpath.

A248 commented 2 years ago

As Gepron1x alluded to, we do not relocate dependencies because we rely on an isolated classloader:

Our dependencies are not visible to other plugins, so our use of Caffeine cannot affect other plugins.
However, other plugins' use of Caffeine can affect us. The shaded dependencies inside the eco library are visible to LibertyBans.

Even though eco is a library, it is still required to relocate its dependencies. Dependent plugins would use the relocated classes. It is not too complicated to make this work, and it can even be done without any source code changes -- only the pom.xml/build.gradle needs modification.

If you'd like me to explain this to the EcoSkills developers, I'm more than happy to help.

WillFP commented 2 years ago

Hi - eco dev here

I would relocate caffeine however that breaks backwards-compatibility (originally, it used the spigot libloader which cant relocate)

Since you're loading in dependencies with an isolated classloader at runtime, why not also relocate them? Depending on how your system works you could download remapped jars or remap them at runtime with shadow (maybe).

Theres not much I can do because of compatibility unfortunately, sorry

A248 commented 2 years ago

Since you're loading in dependencies with an isolated classloader at runtime, why not also relocate them? Depending on how your system works you could download remapped jars or remap them at runtime with shadow (maybe).

This is possible, @WillFP , but in my opinion it is unnecessarily complicated. To be frank, the responsibility is rather on the eco library to relocate dependencies, and I'd rather not increase my own maintenance burden merely to workaround other plugins' bugs.

I'm not sure I understand why you can't release a new version of your library. Suppose you create a new version of eco with relocated dependencies and begin depending on this new library for EcoSkills and other eco plugins going forward. Would not this be a seamless transition? This solution needn't break existing deployments of your plugins.

A248 commented 2 years ago

I'm closing this in favor of the linked issue on the tracker for eco.

A248 / LibertyBans