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

Depend on ASM 9.4 #64

Closed
wants to merge 1 commit into from
Closed

Conversation

kwin
Copy link
Contributor

@kwin kwin commented Oct 29, 2022

Remove embedded ASM classes
This closes #63

Remove embedded ASM classes
This closes eclipse-sisu#63
@mcculls
Copy link
Contributor

mcculls commented Oct 29, 2022

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
Copy link
Contributor Author

kwin commented Oct 29, 2022

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
Copy link
Contributor

mcculls commented Oct 29, 2022

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
Copy link
Contributor Author

kwin commented Oct 29, 2022

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
Copy link
Contributor

mcculls commented Oct 29, 2022

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
Copy link
Member

cstamas commented Oct 29, 2022

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
Copy link
Contributor

mcculls commented Nov 13, 2022

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

@mcculls mcculls closed this Nov 13, 2022
cstamas added a commit that referenced this pull request May 17, 2024
And inconsistent, as then why only this one private static
method, why not all?

These methods are fine to remain static as they do not
depend on any instance variable.

Lets undo this change (and have them all static) or,
we can make them all instance methods, but remain
consistent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to ASM 9.4 to support Java 20
3 participants