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

Stop using repackaged ASM; upgrade ASM to 9.1 #225

Merged
merged 1 commit into from
May 24, 2021
Merged

Conversation

basil
Copy link
Member

@basil basil commented May 24, 2021

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:

+- com.github.jnr:jnr-posix:jar:3.1.5:compile
|  +- com.github.jnr:jnr-ffi:jar:2.2.2:compile
|  |  +- com.github.jnr:jffi:jar:1.3.1:compile
|  |  +- com.github.jnr:jffi:jar:native:1.3.1:runtime
|  |  +- org.ow2.asm:asm:jar:9.1:compile
|  |  +- org.ow2.asm:asm-commons:jar:9.1:compile
|  |  +- org.ow2.asm:asm-analysis:jar:9.1:compile
|  |  +- org.ow2.asm:asm-tree:jar:9.1:compile
|  |  +- org.ow2.asm:asm-util:jar:9.1:compile

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:

    <dependency>
      <groupId>org.parboiled</groupId>
      <artifactId>parboiled-java</artifactId>
      <version>1.3.1</version>
      <exclusions>
        <!-- asm is provided by Jenkins core -->
        <exclusion>
          <groupId>org.ow2.asm</groupId>
          <artifactId>*</artifactId>
        </exclusion>
      </exclusions>
    </dependency>

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:

+- org.kohsuke.stapler:stapler:jar:1532.vfcf95addcb5f:compile
|  +- javax.annotation:javax.annotation-api:jar:1.2:compile
|  +- commons-discovery:commons-discovery:jar:0.4:compile
|  +- commons-logging:commons-logging:jar:1.2:provided
|  +- org.jvnet:tiger-types:jar:2.2:compile
|  \- org.kohsuke:asm5:jar:5.0.1:compile

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.

@timja timja requested a review from jglick May 24, 2021 19:11
@jglick
Copy link
Member

jglick commented May 24, 2021

I would actually love to get rid of JNR (and JNA and JNI) in core, but that is a longer-term project.

jglick
jglick previously requested changes May 24, 2021
Copy link
Member

@jglick jglick left a 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.

@basil
Copy link
Member Author

basil commented May 24, 2021

I think the dep could be removed entirely.

Hard pass. If you are going to insist on that, feel free to close this PR.

@jglick
Copy link
Member

jglick commented May 24, 2021

Fair enough! Let me take a stab at it to see if it is practical.

@jglick jglick dismissed their stale review May 24, 2021 21:38

Does not currently look like Parameter.getName is going to work for us.

@jglick
Copy link
Member

jglick commented May 24, 2021

Do not forget bytecode-compatibility-transformer (ick), pulling in org.kohsuke:asm6:6.2.

Copy link
Member

@jglick jglick left a 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.

@basil
Copy link
Member Author

basil commented May 24, 2021

Do not forget bytecode-compatibility-transformer (ick), pulling in org.kohsuke:asm6:6.2.

I was planning on tackling that after I killed ASM 5. Slow and steady wins the race and all.

@jtnord
Copy link
Member

jtnord commented Jun 10, 2021

I Strongly beleive this should be reverted.
ASM historically has not evolved in a compatbible manner meaning that this will expose ASM to jenkins core and all libraries which can cause plugins that use ASM to break at a moments notice.
Keeping ASM aligned across all the things that need in in Jenkins core wil be a PITA,

@rsandell
Copy link
Member

That is why KK created the shaded versions of asm so they wouldn't collide.

@jglick
Copy link
Member

jglick commented Jun 14, 2021

ASM historically has not evolved in a compatbible manner

jenkinsci/scm-api-plugin#88 (comment) claims to debunk that.

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

Successfully merging this pull request may close these issues.

None yet

5 participants