-
Notifications
You must be signed in to change notification settings - Fork 105
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
Stop using repackaged ASM; upgrade ASM to 9.1 #225
Conversation
I would actually love to get rid of JNR (and JNA and JNI) in core, but that is a longer-term project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the dep could be removed entirely. https://docs.oracle.com/javase/8/docs/api/java/lang/reflect/Parameter.html#getName-- (“new” in Java 8) should cover the functionality we seemed to be using ASM for.
Hard pass. If you are going to insist on that, feel free to close this PR. |
Fair enough! Let me take a stab at it to see if it is practical. |
Does not currently look like Parameter.getName
is going to work for us.
Do not forget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not terribly happy with this but the status quo seems worse and I see no straightforward way to remove this use of ASM altogether.
I was planning on tackling that after I killed ASM 5. Slow and steady wins the race and all. |
I Strongly beleive this should be reverted. |
That is why KK created the shaded versions of asm so they wouldn't collide. |
jenkinsci/scm-api-plugin#88 (comment) claims to debunk that. |
In commit b3f1bcb Kohsuke adopted a repackaged ASM to avoid conflicts. While valid at the time, I don't think it makes much sense to continue to do this. First, it is a maintenance burden on the Jenkins project to release repackaged versions of ASM every time upstream releases a new version. Second, recent versions of Jenkins core already include the upstream version of ASM transitively via JNR:
So the upstream version of ASM has become part of the Jenkins API whether we like it or not. And so far this has not caused too much trouble, other than breaking Token Macro (via Parboiled). That issue was resolved in jenkinsci/token-macro-plugin#70, which updated Token Macro's POM to read:
This clearly indicates that the ecosystem is expecting ASM to be bundled in core. Yet we still continue to bundle Kohsuke's version of ASM 5 in core via Stapler:
This just bloats the WAR by bundling two versions of ASM. One of these two versions only exists to avoid exposing ASM in the public API, but it is already exposed so this is pointless.
In this change I am proposing to upgrade Stapler to the latest upstream version of ASM (the same version already shipped in Jenkins core transitively through JNR). I would then propose that we formalize the de facto bundling of ASM in Jenkins core by adding ASM as an explicit version in the Jenkins core BOM. This will allow us to manage upgrades carefully and avoid a repeat of the Token Macro incident in the future.