search

eclipse-sisu / sisu-project

Sisu Inject
https://eclipse.dev/sisu/
Eclipse Public License 2.0
18 stars 15 forks source link

Depend on ASM 9.4 #64

Closed kwin closed 1 year ago

kwin commented 1 year ago

Remove embedded ASM classes This closes #63

mcculls commented 1 year ago

I'm fine with upgrading to a more recent version of ASM but we cannot depend on it directly without repackaging. If we don't change the package name then there's a high risk of conflicts on the classpath if something else brings in a different version of ASM.

Historically we've done this repackaging manually (albeit via a simple script) because we rarely need to upgrade ASM, rather than overcomplicate the build with shading/jarjar.

kwin commented 1 year ago

I not only see the risk of conflicting versions on the classpath but rather the chance for consumers to upgrade the dependency individually (given the few releases sisu had in the past). Where exactly do you fear conflicts? In Eclipse OSGi manages that due to the import package version range. If you talk about Maven then it is easy to ship it with that or a newer version of asm (although it slightly blows up the core classloader)

mcculls commented 1 year ago

It's not theoretical - in the early days before repackaging it we had a number of hard to debug issues caused by having multiple versions of ASM on the classpath. It is also an internal dependency that I prefer not to have as an external dependency.

kwin commented 1 year ago

Ok, but manually embedding does no longer scale given the 6 month release cycle of Java. It also requires a new Sisu release for each new java version having a new binary class version. Do you think you can manage that @mcculls or find more committers who can do that?

mcculls commented 1 year ago

I think it's manageable and I've been talking with @cstamas about doing another release soon which can include ASM 9.4

BTW I believe that this is only an issue if you want to write components that compile to Java 20 bytecode - if you're running on Java 20 but consuming components that were compiled for earlier versions then that should still work without changing the version of ASM.

(separately I would like to grow the committership now we're getting a steady stream of suggestions/tasks)

cstamas commented 1 year ago

but @kwin is right re scaling... this would mean we need (well, those who want to be "on the edge") sisu release each 6 months as well... (current releases does not support java 17, master does, but 19 is already there, out, etc....)....

mcculls commented 1 year ago

Closing as I really want to keep ASM embedded to avoid known class-path issues - we now have a script to make upgrading it easier which I used to upgrade to ASM 9.4