ASM in core

[jn...@cloudbees.com]

Hi all,

I have just noticed a few PRs (some merged) to change ASM in core or libraries that core depdns on (stapler).

I think we need to revert these and have a bigger think about ASM.

ASM historically (and I believe still true) evolved in a non compatible way.

In the past I have seen that versions add support for new class opcodes, but this requires the code to implement the new methods to handle them as a no-op, or causes linkage failures depending on how you use ASM.

We also have a complete mix of ASM versions required for Core.  (stapler / BCT) can be aligned quite easily - but Guice also depends on ASM (our current version of guice uses 5.0.3 (unshaded!)) and whilst newer versions of Guice use 9, bumping to that may entail bumping Guava (which Guice also heavily relies upon) and we are working towards that byut are not yet ready for it.

I have not seen the ASM changes discussed before (sorry if I missed it).  Could we at least have a discussion here or via a JEP as I believe there are several moving parts that need to align (and possible several things we currently do that are also broken!).

Thanks

/James

[Matt Sicker]

I don't remember seeing formal docs about it, but I believe it was
changed for two reasons:

1. Dependency simplifications in general. That's been an ongoing
effort by multiple people, particularly in Jenkins Core.
2. ASM needs to be upgraded more often now that Java itself is
released more often. Even if you shade in ASM, you trade one problem
for another in that newer Java releases are unable to run Jenkins (I'd
give a pass for Java 16 since they changed some stuff related to
poking at internals).

I do think a JEP could be useful if it helped figure out a strategy
for packaging and updating ASM to avoid compatibility issues for both
ASM itself and Java.

> --
> You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8558ef90-cf4b-4edb-8c25-a33ab74c5f6bn%40googlegroups.com.

[Basil Crow]

ASM has been shipped by core, unshaded, as a transitive dependency of
JNR (_not_ JNA) since JNR was introduced in 2013. Removing core's
dependency on JNR (and therefore its transitive dependency on unshaded
ASM) is a large and yet unscoped project; similarly, hiding core
dependencies from plugins is another large and yet unscoped project.

JNR was updated in 2.277 in December 2020, which also updated core's
(transitive) ASM dependency. This broke Token Macro. The Token Macro
breakage was resolved by excluding ASM from Token Macro, allowing it
to use the copy provided by core.

In general, when a plugin depends on a library already provided by
core, I have seen three approaches in the short term:

1. Exclude the library on the plugin side (e.g. how Token Macro excludes ASM)
2. Mask the library's classes (e.g. how JaCoCo masks ASM classes)
3. Shade the library into the plugin

None of these are ideal compared to the larger projects of removing
(or hiding) Guice/JNR (and by extension Guava/ASM) from core, but all
three approaches work in the short term.

As long as core continues to expose JNR (and therefore unshaded ASM)
in its public API, plugins that use ASM (directly or indirectly) must
follow one of these three approaches in the short term. Similarly, as
long as core continues to expose Guice (and therefore unshaded Guava)
in its public API, plugins that use Guava (directly or indirectly)
also must follow one of these three approaches in the short term.

Whether we like it or not, core has been in the business of providing
unshaded ASM since 2013, so being explicit about it (by adding ASM to
the list of core dependencies and the core BOM as I did in
jenkinsci/jenkins#5525) at least allows us to manage it carefully.
This is not as good as ripping out JNR (and by extension ASM) from
core, but it is better than having ASM get updated accidentally with
unrelated JNR upgrades, as happened in December 2020.

If a plugin that uses ASM or Guava fails to follow one of these three
approaches (as is currently the case with Subversion), it is going to
have problems in the short term: either with JNR alone (prior to 2.296
thus stapler/stapler#244), or with JNR and Stapler (after 2.296 thus
stapler/stapler#244).

Note that even if stapler/stapler#244 is reverted, a plugin that uses
ASM but fails to follow one of the three approaches outlined above is
going to have problems when invoking a core API that invokes JNR: JNR
will invoke the (recent, unshaded) ASM (just as Stapler is doing
post-stapler/stapler#244), which will fail. In other words, reverting
the Stapler change will reduce the surface area of the problem, but it
will not eliminate the problem.

For this reason, I recommend that all plugins that use Guava or ASM
follow one of the three approaches outlined above in the short term.
This is the only guaranteed way for plugins to avoid Guava and ASM
problems at present.

[jn...@cloudbees.com]

>  . Even if you shade in ASM, you trade one problem
for another in that newer Java releases are unable to run Jenkins (I'd
give a pass for Java 16 since they changed some stuff related to
poking at internals).

How so, you upgrade the shaded library.  You can also keep the original shaded library if you like (that is why these libraries where published under different artifactId), or you can punt that shaded library to a detached plugin extreemly simply.

given the backwards incompatible nature - Guice is compiled against 5.0.3 a far cry from 9.  I strongly believe this is just an accident waiting to happen.
https://abi-laboratory.pro/index.php?view=timeline&lang=java&l=asm is showing a big fat red warning for just version 6....

[jn...@cloudbees.com]

>  1. Exclude the library on the plugin side (e.g. how Token Macro excludes ASM)

2. Mask the library's classes (e.g. how JaCoCo masks ASM classes)2 -> 

3. Shade the library into the plugin

1 -> Which is fine until the library evolves in a binary incompatible way.. and the ASM library is KNOWN to do this.

2-> has many pitfalls and this only works if no one else depends on your plugin otherwise they also get those classes (ref the jacoco guice inject mess)

3 -> requires a larger restructuring of plugins to enable that and I look forward to the PRs.

> Note that even if stapler/stapler#244 is reverted, a plugin that uses

ASM but fails to follow one of the three approaches outlined above is
going to have problems when invoking a core API that invokes JNR: JNR
will invoke the (recent, unshaded) ASM (just as Stapler is doing
post-stapler/stapler#244), which will fail. In other words, reverting
the Stapler change will reduce the surface area of the problem, but it
will not eliminate the problem.

I do not believe this to be correct,  the core class will resolve ASM using the core classes' classloader not the plugins classloader even when invoked from the plugin.   If the core class took an ASM class/object as a parameter then it would blow up but meerly calling `jenkins.foo()`  where `foo` calls some ASM method will work correctly due to the hierachical nature of Jenkins classloaders.

Thus based on the above this really is making things worse.

On Thursday, June 10, 2021 at 5:18:44 PM UTC+1 m...@basilcrow.com wrote:

[Basil Crow]

On Thu, Jun 10, 2021 at 10:03 AM jn...@cloudbees.com
<jn...@cloudbees.com> wrote:
>
> 1 -> Which is fine until the library evolves in a binary incompatible way.. and the ASM library is KNOWN to do this.
> 2-> has many pitfalls and this only works if no one else depends on your plugin otherwise they also get those classes (ref the jacoco guice inject mess)

Agreed. That's why I said the status quo is "not ideal". But by and
large it works well enough to keep the ecosystem stable.

> 3 -> requires a larger restructuring of plugins to enable that and I look forward to the PRs.

I am not planning on opening any such PRs, and I am not sure why you
would think that I am. Please remember that I am a volunteer and am
just trying to help where I can. An encouraging and constructive tone
would be appreciated.

> I do not believe this to be correct, the core class will resolve ASM using the core classes' classloader not the plugins classloader even when invoked from the plugin.

I trust you, but I am having a hard time understanding the behavior
observed empirically here:
https://github.com/jenkinsci/subversion-plugin/pull/254#issuecomment-858288074

Based on your statement, I would expect the core class (Stapler) to

"resolve ASM using the core classes' classloader not the plugins

classloader even when invoked from the plugin". This implies that
whether Subversion bundles ASM or not, Stapler should use the ASM
provided by core. But yet running that test against 2.297 passes when
Subversion doesn't bundle ASM and fails when Subversion does bundle
ASM, implying that the ASM available to Stapler somehow differs based
on the plugin, contradicting the earlier implication. I think we have
not yet gotten to the bottom of this. Perhaps someone like Jesse might
be able to shed some light about what is happening here.

[jn...@cloudbees.com]

Let me try and rephrase this.

We can not be stuck with 10 year old versions of thing, we all agree on that, but we also agree we can not just upgrade something and break plugins[1].  The current semi policy is no breaking changes (which is why we have not even removed deprecated code in Jenkins core)

Currently libraries in Jenkins are exposed and usable in plugins, we have some internal work in progress to try and change this, but it is not here today so for now this is off not part of the discussion.
Thus care needs to be taken before any library is updated (which is exactly what is happening for the Guava work)

As we need to be careful with changes to a library we need to do due diligence.  What that is can depend somewhat on the library (well behaved semver and if it is a major or minor version change, or if it is known to be a risky library not using semver) as well on the number of releases / commits being bumped.

Where we perceive risk I would propose that we should include a binary compatible report (rev-api) and a report of how that impacts plugins (usage in plugins).
Thus could be semi light weight (inside a PR) or via a JEP, I am not worried so long as it is referenceable and the impact of the code change is known about where it is being reviewed.

It could be this particular ASM issue turns out to be a complete non issue in which case great, but it could turn out to be a digester2/3 issue in the waiting...

/James

[1] the exact number of plugins we can break and the nature of them (10 installs vs 1000  etc etc) may vary from person to person

[Basil Crow]

Dear James,

On Thu, Jun 10, 2021 at 10:34 AM jn...@cloudbees.com
<jn...@cloudbees.com> wrote:
>
> Thus care needs to be taken before any library is updated

Of course. Yet such care was not taken when JNR was updated in
December 2020. That is why nobody noticed that the JNR update also
pulled in a new ASM update, and that is why Token Macro broke.

We might not be happy about this, but it is too late to go back in
time. My response to this event was to make core's (de facto) exposure
of ASM explicit. I believe this will help core maintainers be more
aware of these changes in the future and avoid this type of mistake
going forward.

I note that your last message did not contain any specific concrete
action items and did not address my final paragraph. Absent us drawing
any conclusions about what I wrote about in my final paragraph, my
recommendation remains that plugins consistently follow the current
status quo of either excluding or masking Guava/ASM, which works well
enough (but not without the caveats you mentioned). I have provided an
example of how to do this for Subversion in the comments to
jenkinsci/subversion-plugin#254.

If there are any additional concrete action items that I can help with
in the short term, please let me know.

Thanks,
Basil

[jn...@cloudbees.com]

>  am not planning on opening any such PRs, and I am not sure why you

would think that I am. Please remember that I am a volunteer and am
just trying to help where I can. An encouraging and constructive tone
would be appreciated.

Apologies for the offence.

>  I trust you, but I am having a hard time understanding the behavior
observed empirically here:
https://github.com/jenkinsci/subversion-plugin/pull/254#issuecomment-858288074

JenkinsRule /  RestartableJenkinsRule and any other junit test not using `RealisticJenkinsRule` do not use the hierarchical Jenkins classloader and are hence completely unrealistic when it comes to classloading.  They use the classpath provided my maven-surefire which is a flat classpath of all test (runtime, compile etc..) dependencies.
See https://issues.jenkins.io/browse/JENKINS-41827 for some more details.
the referenced test is one of those tests.   The `UpperBounds` enforcer rule in the plugin pom to try and force things to align was an early attempt to ensure that you at least got the version of libraries expected (but it does not correct the cause just tried to hide it somewhat)

If the test was switched to use  https://javadoc.jenkins.io/component/jenkins-test-harness/org/jvnet/hudson/test/RealJenkinsRule.html from  RestartableJenkinsRule I believe the same issues would not be seen.  I am not saying there are not other issues associated with the current state of the Subversion plugin but I think this particular issue would no longer be observed.

[Basil Crow]

On Thu, Jun 10, 2021 at 10:48 AM jn...@cloudbees.com
<jn...@cloudbees.com> wrote:
> JenkinsRule / RestartableJenkinsRule and any other junit test not using `RealisticJenkinsRule` do not use the hierarchical Jenkins classloader and are hence completely unrealistic when it comes to classloading. They use the classpath provided my maven-surefire which is a flat classpath of all test (runtime, compile etc..) dependencies.
> See https://issues.jenkins.io/browse/JENKINS-41827 for some more details.
> the referenced test is one of those tests. The `UpperBounds` enforcer rule in the plugin pom to try and force things to align was an early attempt to ensure that you at least got the version of libraries expected (but it does not correct the cause just tried to hide it somewhat)
>
> If the test was switched to use https://javadoc.jenkins.io/component/jenkins-test-harness/org/jvnet/hudson/test/RealJenkinsRule.html from RestartableJenkinsRule I believe the same issues would not be seen. I am not saying there are not other issues associated with the current state of the Subversion plugin but I think this particular issue would no longer be observed.

Agreed. I guess that implies JENKINS-65862 is a test-only issue rather
than a production issue, which would be a good thing! Still, we need
to solve it, at least by the time we get around to creating a new BOM
line for the next LTS line after 2.289, because PCT tests need to be
stable for the BOM. We could do that, as you mentioned, by rewriting
any tests that use the Subversion plugin so that they use
RealJenkinsRule. But it seems impractical to have a state of affairs
where the Subversion plugin is effectively unusable from regular
JenkinsRule tests. I think a more practical option would be to restore
the Subversion plugin to a state where it can be tested in both
JenkinsRule and RealJenkinsRule, by excluding or masking the ASM
classes.

[Tim Jacomb]

It would be good to see a more recent report given we’re on version 9 in core to see if anything has changed in recent versions 

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/abd68b18-c265-4dca-a5a2-0255c16dfda3n%40googlegroups.com.

[Basil Crow]

On Thu, Jun 10, 2021 at 11:28 AM Tim Jacomb <timja...@gmail.com> wrote:
>
> It would be good to see a more recent report given we’re on version 9 in core to see if anything has changed in recent versions

Great point, Tim. Core 2.273 shipped with ASM 5.0.3, prior to the
upgrade of JNR (and therefore the accidental upgrade of ASM) in 2.274.
Here are the differences between ASM 5.0.3 and ASM 9.1:

https://diff.revapi.org/?groupId=org.ow2.asm&artifactId=asm&old=5.0.3&new=9.1
https://diff.revapi.org/?groupId=org.ow2.asm&artifactId=asm-analysis&old=5.0.3&new=9.1
https://diff.revapi.org/?groupId=org.ow2.asm&artifactId=asm-commons&old=5.0.3&new=9.1
https://diff.revapi.org/?groupId=org.ow2.asm&artifactId=asm-tree&old=5.0.3&new=9.1
https://diff.revapi.org/?groupId=org.ow2.asm&artifactId=asm-util&old=5.0.3&new=9.1

The library seems quite stable between 5.0.3 and 9.1. Apart from new
classes and the addition of generics, nothing seems to have been
removed or renamed. (Similarly, recent versions of Guava are more
stable than older versions.)

In any case, I doubt we would roll back the JNR changes made in 2.274
at this point. For better or for worse, ASM 9.1 is here to stay as
part of the core API, so we might as well own it in the short term.
(In the long term, removing or hiding Guice/JNR and therefore
Guava/ASM would be nice, of course.)

[Ivan Fernandez Calvo]

Hi,

I would like to add an issue related to not bump the ASM library, I recently hit an issue with stapler (see https://groups.google.com/g/jenkinsci-dev/c/JppfFwqIrCU) and it is related to compile the plugin for JDK 11 only,

new plugins use new libraries and more and more libraries use JDK 11 nowadays for obvious reasons. 

So bump the ASM version is something completely necessary. Break thing is fine and more when those things are 10 years old stuff, in general, the Jenkins Core has a few forked libraries that are a pain to maintain updated, most of them are 10 yo pains. Thus, I think it is a matter of making a damage control of the plugins affected by the change and fix those that are still maintained or in use, Do we know which plugins are affected by the change on ASM?

BTW the ASM/JNR/JNA update is part of the JEP-211 and JENKINS-40689 so stuff longtime ago exposed

[Robert Sandell]

Some historical context to know where we "old timers" are coming from :)

https://kohsuke.org/2012/03/03/potd-package-renamed-asm/

There are several times where I've needed to shade asm in plugins because of some transitive dependency. Las time I think it was a dependency on ASM7 that blew up for me and I had to repackage that dependency to not break other plugins that depended on my plugin.

That's why me and James and others are very very wary of bumping the asm dependencies, I think even more wary than for Guava, because there isn't enough code coverage due to how the classpath is during unit testing.

I am not fully read up on the JNI situation so I can't comment on that situation, but it sounds like a damned if we do, damned if we don't kind of situation?

/B

[kuisathaverat]

Robert, Which plugin are you talking about? Which version of the core is that plugin required as minimum? there is another topic opened a few weeks ago about deprecated plugins, a plugin should bump it core minimum required version and update its dependencies often, if not it is not possible to ensure the interoperability between plugins and with the core, to maintain 10 years of backward compatibility is not a good idea and block the project to move forward.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/NhV_o6zxbzw/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/9c2d0cbb-2972-4e88-a51b-c53a5f39f5e4n%40googlegroups.com.

--

Un Saludo

Iván Fernández Calvo

https://www.linkedin.com/in/iv%C3%A1n-fern%C3%A1ndez-calvo-21425033

[Ullrich Hafner]

>
> In general, when a plugin depends on a library already provided by
> core, I have seen three approaches in the short term:
>
> 1. Exclude the library on the plugin side (e.g. how Token Macro excludes ASM)
> 2. Mask the library's classes (e.g. how JaCoCo masks ASM classes)
> 3. Shade the library into the plugin
>

> […]

> For this reason, I recommend that all plugins that use Guava or ASM
> follow one of the three approaches outlined above in the short term.
> This is the only guaranteed way for plugins to avoid Guava and ASM
> problems at present.

Thanks for clarifying these options!

Wouldn’t it be helpful if we would also suggest which option we prefer? Otherwise every plugin author needs to rethink the same options again and again.

E.g., option 1 did break all my integration tests with the token macro plugin since it does not bundle ASM anymore.

[Jim Klimov]

><https://github.com/jenkinsci/jep/blob/a6932225baa1d53224de4989c53b62e72c2e9299/jep/211/README.adoc#library-updates>
>>> and JENKINS-40689 <https://issues.jenkins.io/browse/JENKINS-40689>

><https://groups.google.com/d/msgid/jenkinsci-dev/9c2d0cbb-2972-4e88-a51b-c53a5f39f5e4n%40googlegroups.com?utm_medium=email&utm_source=footer>
>> .
>>

One plugin that I can't locally `mvn package`, not even from its master branch, for several weeks now is github-branch-source-plugin

It complains about
enforcer.RequireUpperBoundDeps
and lists various stacks of dependencies, the best I could make of its output.

Some stacks end with "asm:7.1" (and mention "token-macro:2.15" that was noted elsewhere in this big thread, always from "github:1.33.1"), and others end with "asm:9.0" which I assume is the fatal conflict that breaks my builds.

Jim Klimov

--
Typos courtesy of K-9 Mail on my Android

[Basil Crow]

On Fri, Jun 11, 2021 at 3:45 AM Ullrich Hafner <ullrich...@gmail.com> wrote:
>
> Wouldn’t it be helpful if we would also suggest which option we prefer? Otherwise every plugin author needs to rethink the same options again and again.
>
> E.g., option 1 did break all my integration tests with the token macro plugin since it does not bundle ASM anymore.

As James pointed out, shading isn't a valid option. When I wrote that
I must have been thinking about the shading in Remoting, which isn't a
plugin at all.

Between excluding the library and masking classes, I tend to prefer
excluding the library, as it is less complex than masking classes at
runtime and also results in a leaner plugin. However, it is also more
risky depending on the compatibility level of the library, as James
noted. If the version in core is binary incompatible with the version
your plugin needs, you could have problems. For example, in
Timestamper I depend on OWASP Java HTML Sanitizer, which depends on
Guava 23, which I am excluding because core already provides Guava 11.
But that is risky, because if OWASP Java HTML Sanitizer ever calls a
method not provided by Guava 11 things would break. It happens not to
do that, but the situation should make anybody nervous. (It should
also make anybody nervous that core runs Guice 4.0, which requires
Guava 16, against Guava 11.)

In contrast, JaCoCo and Artifact Manager on S3 are using
"maskClasses", which is more complex as it involves changing runtime
behavior. But on the other hand it does allow the plugin's version of
the library to be used rather than core's version. The developer
documentation for this option reads: "You must manually verify that
the masked classes are complete under the transitive closure of the
Java linker: for example, masking one package but not another from a
library bundled in core could make classes in the masked package
unresolvable. […] If you did not understand any part of this section,
do not use these options. Even if you did, think twice." All of this
complexity should make anybody nervous as well.

As you can see, neither of these are very good options, which is why
removing (or hiding) Guice/JNR (and therefore Guava/ASM) are such
important long-term projects. In the short term, I tend to choose
excluding the library because it seems simpler, but you may need to
choose "maskClasses" if you need the newer version of the library.

[Basil Crow]

On Fri, Jun 11, 2021 at 2:19 AM Robert Sandell <rsan...@cloudbees.com> wrote:
>
> Some historical context to know where we "old timers" are coming from :)
> https://kohsuke.org/2012/03/03/potd-package-renamed-asm/

Thanks for providing this context, Robert! I have a tremendous amount
of respect for all the old-timers in this project. Thanks for having
the patience to explain this to a newcomer like me.

I dug up the original post [1] where Kohsuke laid out his complaints
about ASM. All these complaints seem completely legitimate in the
context of ASM 3 in 2010, which definitely seems like it broke
compatibility with ASM 2.

I know these scars run deep, but I think we should re-examine things
in 2021. As of commit 6a0e7842de436225f3866d3567834c6285107114 in ASM
5.2, ASM added signature tests to avoid breaking backward binary
compatibility with any version >= 4.0. These signature tests are still
present in "src/test/resources" on the latest version of ASM. So ASM
appears to take compatibility seriously these days.

We shouldn't be as nervous about compatibility between ASM 5 and ASM 9
in 2021 as we were about compatibility between ASM 2 and ASM 3 in
2010. Things have changed in the intervening decade.

> That's why me and James and others are very very wary of bumping the asm dependencies, I think even more wary than for Guava, because there isn't enough code coverage due to how the classpath is during unit testing.

Again, I know these scars run deep, but allow me to reiterate that
core has _already_ bumped ASM to latest in December 2020, in my
opinion unintentionally, as a result of bumping JNR to latest. I
didn't review that change, but I am not casting aspersions on those
who reviewed and merged it either. It is a particularly easy mistake
to make. But the fact is, I think we are unlikely to roll it back now.
So whether we like it or not, ASM 9 as a core dependency seems like it
is here to stay.

And since it is here to stay, and since compatibility is taken
seriously in recent versions of ASM, I think it is OK for plugins to
use the version from core by excluding the ASM dependency.

Basil

[1] https://web.archive.org/web/20120701173200/http://weblogs.java.net/blog/kohsuke/archive/2010/02/12/asm-incompatible-changes
[2] https://gitlab.ow2.org/asm/asm/-/commit/6a0e7842de436225f3866d3567834c6285107114

[Jesse Glick]

We should also take a step back and see if these problematic deps really need to exist at all. Most uses of ASM should be deleted if at all possible. I was able to remove Digester and thus, I guess, ASM from the Subversion plugin without much difficulty. I do not understand Token Macro well enough to understand whether it fundamentally needs ASM there (nor do I intend to spend time on that plugin given that Pipeline rendered it obsolete). The Stapler dep on ASM turned out to be just an overengineered way of solving a problem which can be addressed by adding a single flag to javac (though we need to update the parent POMs of plugins to avoid needing ASM at runtime for them).

JNR may be a similar story. I see all of two usages in core—both disabled unless you set a system property. Just deleting it all may be easier than having subtle debates about class loader behavior and compatibility policy.

[Slide]

ASM is a dependency of the parser used (parboiled). People still use token macro a lot even in pipeline even if there are other ways of doing what it does. 

On Mon, Jun 14, 2021 at 7:36 PM Jesse Glick <jgl...@cloudbees.com> wrote:

We should also take a step back and see if these problematic deps really need to exist at all. Most uses of ASM should be deleted if at all possible. I was able to remove Digester and thus, I guess, ASM from the Subversion plugin without much difficulty. I do not understand Token Macro well enough to understand whether it fundamentally needs ASM there (nor do I intend to spend time on that plugin given that Pipeline rendered it obsolete). The Stapler dep on ASM turned out to be just an overengineered way of solving a problem which can be addressed by adding a single flag to javac (though we need to update the parent POMs of plugins to avoid needing ASM at runtime for them).

JNR may be a similar story. I see all of two usages in core—both disabled unless you set a system property. Just deleting it all may be easier than having subtle debates about class loader behavior and compatibility policy.

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr3NPD3b%2BQ2TiQGLyqicuA2-gEby-%2BCJgLGtViXOw1Uh_w%40mail.gmail.com.

--

Website: http://earl-of-code.com

[Basil Crow]

On Mon, Jun 14, 2021 at 7:36 PM Jesse Glick <jgl...@cloudbees.com> wrote:
>

> JNR may be a similar story. I see all of two usages in core—both disabled unless you set a system property. Just deleting it all may be easier

I had already noticed that as well and thought about opening such a PR
to delete all usages of JNR. I decided to settle for removing
`jna-posix` first. :-) But regarding removing JNR, what if someone is
relying on that system property? Unlike API changes, where I can
easily check `usage-in-plugins` to see how many plugins (both open
source and CloudBees-proprietary) are still using a given API, there
is no equivalent for me to check how many installations are relying on
a given system property being said. This makes me a bit nervous to
remove it. How has the Jenkins project handled cases like this
historically?

I can give one example of how I handled a similar situation in Email
Extension. In 2.78 [1] (October 2020) I announced in the release notes
that "integration with the Static Analysis Utilities plugin has been
deprecated in the Email Extension plugin and will be removed in a
future release", linking to the relevant Jira issue. In 2.81 [2]
(January 2021) I again announced in the release notes (more strongly
this time) that "integration with the Static Analysis Utilities plugin
was deprecated in 2.78 and is scheduled to be removed in the first
quarter of 2021", again linking to the relevant Jira issue. In 2.82
[3] (March 2021), I pulled the plug and announced in the release notes
that "as of 2.82, integration with the Static Analysis Utilities
plugin has been removed from the Email Extension plugin".

I gave users a chance to speak up. Nobody spoke up. I gave users a
second chance to speak up. Nobody spoke up. I pulled the plug. Nobody
complained. I think it went pretty well.

Perhaps this might be a pattern worth following in Jenkins core?

[1] https://github.com/jenkinsci/email-ext-plugin/releases/tag/email-ext-2.78
[2] https://github.com/jenkinsci/email-ext-plugin/releases/tag/email-ext-2.81
[3] https://github.com/jenkinsci/email-ext-plugin/releases/tag/email-ext-2.82

[raihaan...@gmail.com]

In the case of system properties https://www.jenkins.io/doc/book/managing/system-properties/

Says:

Compatibility

We do NOT guarantee that system properties will remain unchanged and functional indefinitely. These switches are often experimental in nature, and subject to change without notice. If you find these useful, please file a ticket to promote it to an official feature

So breaking it is not the worst.

[Jesse Glick]

On Mon, Jun 14, 2021 at 10:47 PM Slide <slide...@gmail.com> wrote:

ASM is a dependency of the parser used (parboiled).

What is what I gathered. Probably the fairly simple syntax used by this plugin could be handled with a hand-written parser (regexp?) with some effort. But it seems to also be used by some JSONPath system? 

[Slide]

It originally used regex/hand parsing, but some of the requests for features were not super easy to implement using that method. I can look at it again if we want to remove the ASM dependency for sure. 

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr1acCBAzdOGx_NzQHx3VpP62pd%3Dp%2BuuEK0Ajxo5_kpdtQ%40mail.gmail.com.

--

Website: http://earl-of-code.com

[James Nord]

Thanks for investigating further on this Basil!

/James

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAFwNDjpBuUqLfb7McWCZ6Ue2eMCGDj2vff9atO0_94VLykXCeQ%40mail.gmail.com.

[Basil Crow]

Once JNR is removed from core in jenkinsci/jenkins#5979 (which is
blocked on the merge, release, and widespread adoption of
jenkinsci/pam-auth-plugin#20), then the only remaining consumers of
ASM in core will be Stapler [1] and access-modifier [2]. It would be
nice to eliminate these dependencies from core, which would allow us
to detach ASM to an API plugin. I think we all agree that keeping ASM
in core is a maintenance burden.

I suppose we could shade ASM into Stapler and access-modifier, but the
last time I tried to do this for Guava I ran into weird Maven issues
(MNG-5899?) and have not yet figured out a way to work around them.

If anyone can think of a more clever way to eliminate our dependency
on ASM, I would love to hear it.

[1] https://github.com/jenkinsci/stapler/blob/980e6ca1a29aa31cee1356e2a4e22ac5274a86fa/core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java#L320-L407
[2] https://github.com/jenkinsci/lib-access-modifier/blob/b68c0122e7cfc4772fb2f8ab485b2157fdfa53f8/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/ProtectedExternally.java#L64-L78

[Jesse Glick]

On Tue, Jan 4, 2022 at 3:07 PM Basil Crow <m...@basilcrow.com> wrote:

the only remaining consumers of ASM in core will be Stapler and access-modifier

`access-modifier` is not a big deal since it is used only at compile time—need not be shipped in core.

The usage in Stapler is an obstacle. IIUC until a plugin is built with https://github.com/jenkinsci/plugin-pom/releases/tag/plugin-4.19 or newer (or explicitly specifies a name in `@QueryParameter`), some web methods will fail to receive parameters, producing an error at https://github.com/jenkinsci/stapler/blob/980e6ca1a29aa31cee1356e2a4e22ac5274a86fa/core/src/main/java/org/kohsuke/stapler/QueryParameter.java#L68 [sic] (or, rarely, in `@Header`).

[Basil Crow]

On Tue, Jan 4, 2022 at 1:05 PM 'Jesse Glick' via Jenkins Developers
<jenkin...@googlegroups.com> wrote:
>
> `access-modifier` is not a big deal since it is used only at compile time—need not be shipped in core.

But as of jenkinsci/plugin-pom#480 plugins get their (compile time!)
access-modifier-annotation dependency from core, so even if we didn't
have to ship the ASM JAR in the WAR we'd still have to deal with
awkward Enforcer errors in plugins, which is definitely not ideal.

> The usage in Stapler is an obstacle.

So should I look into the shading approach further? I can't remember
what problems I ran into the last time I tried to do this with Guava,
but I found it quite difficult. I think at one point one of the
CloudBees Maven gurus recommended that I accomplish the shading by
making a dedicated module for the shaded JAR, but I didn't quite
follow the concept and I haven't been able to find any examples of
this online. If you have any pointers to a working example of this
approach that would definitely help.

[Jesse Glick]

On Tue, Jan 4, 2022 at 5:22 PM Basil Crow <m...@basilcrow.com> wrote:

So should I look into the shading approach further?

That basically takes us back to the unfortunate state we were in with Kohsuke’s series of shaded & repackaged ASM libraries, where we could not give a clear answer as to what we were actually bundling, and security scanners complained, etc. Not sure that is an improvement.

accomplish the shading by making a dedicated module for the shaded JAR, but I didn't quite follow the concept

Right, you sometimes get into trouble when you try to use the library from the same Maven module that Shades it. Better to create a separate module in the reactor which solely Shades the library, then depend on that module from Stapler core. https://github.com/jenkinsci/docker-traceability-plugin/pull/18 shows the idea.

Another possibility is to write a minimal bytecode parser that just groks the symbol table, list of methods with their binary signatures, and method parameter metadata.

[Basil Crow]

On Tue, Jan 4, 2022 at 5:09 PM 'Jesse Glick' via Jenkins Developers
<jenkin...@googlegroups.com> wrote:
>
> That basically takes us back to the unfortunate state we were in with Kohsuke’s series of shaded & repackaged ASM libraries, where we could not give a clear answer as to what we were actually bundling, and security scanners complained, etc. Not sure that is an improvement.

Does not seem so bad if limited to Stapler (and a deprecated code path
within Stapler at that). In the old status quo, plugins were also
consuming the shaded copy, which would no longer be necessary. Once
ASM is detached to an API plugin, plugins like SCM: API and Token
Macro can consume the ASM API plugin without shading. The version of
the shaded ASM consumed by Stapler would be updated by Dependabot;
hopefully that addresses the security concern.

> Right, you sometimes get into trouble when you try to use the library from the same Maven module that Shades it. Better to create a separate module in the reactor which solely Shades the library, then depend on that module from Stapler core. https://github.com/jenkinsci/docker-traceability-plugin/pull/18 shows the idea.

Thank you for the pointer. That seems like an example I can follow.

> Another possibility is to write a minimal bytecode parser that just groks the symbol table, list of methods with their binary signatures, and method parameter metadata.

Like https://github.com/paul-hammant/paranamer/blob/master/paranamer/src/java/com/thoughtworks/paranamer/BytecodeReadingParanamer.java
for example (though that is a stripped-down version of ASM, but the
same basic concept). But whether we reuse that or write our own, the
result would still be ~1,000 lines of low-level code for us to
maintain compared to letting the ASM experts do it for us. Does not
seem like a great use of time to me.

[Basil Crow]

Recent weeklies no longer consume ASM. So shall we detach it to a
library plugin?

[Tim Jacomb]

If core no longer uses it the. Yes absolutely +1

On Thu, 18 Aug 2022 at 05:40, Basil Crow <m...@basilcrow.com> wrote:

Recent weeklies no longer consume ASM. So shall we detach it to a
library plugin?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAFwNDjr6H53AJNa9YedKtSUOwNg4B7EayJshWZWW7TC3gucXfw%40mail.gmail.com.

[Alexander Brandes]

+1 for detaching

[Daniel Beck]

On Thu, Aug 18, 2022 at 6:40 AM Basil Crow <m...@basilcrow.com> wrote:

Recent weeklies no longer consume ASM. So shall we detach it to a
library plugin?

I might miss some context, as this thread has been going for a while, but is a clean removal like JNR doable here as well? A quick GH search indicates that only a few plugins use ASM.

[Basil Crow]

On Thu, Aug 18, 2022 at 10:30 AM 'Daniel Beck' via Jenkins Developers
<jenkin...@googlegroups.com> wrote:
> I might miss some context, as this thread has been going for a while, but is a clean removal like JNR doable here as well? A quick GH search indicates that only a few plugins use ASM.

Probably doable with a little more effort I would imagine.

[Basil Crow]

I think detaching is riskier than I expected: a lot of plugins bundle
old copies of ASM (or depend on other plugins that do). With core's
copy no longer taking precedence, I fear that there might be a high
risk of regression with a detached plugin. Seems safer to deal with
each plugin on a case-by-case basis.

I searched for usages of ASM (both direct and transitive) in both
open-source and proprietary plugins. After filtering out plugins that
bundled a recent (9.x) ASM JAR in their JPI, plugins whose only usage
of ASM was through Commons Compress Pack200 support (probably not
used), plugins with fewer than 1,000 installations, and plugins that
have already been deprecated, I came up with the following list:

https://plugins.jenkins.io/analysis-model-api - via cglib (via Commons
Digester) and com.jayway.jsonpath/net.minidev:json-smart
https://plugins.jenkins.io/checkmarx - via xbean-reflect-3.7.jar
https://plugins.jenkins.io/clearcase - via cglib (via Commons Digester)
https://plugins.jenkins.io/clover - via cglib (via Commons Digester)
https://plugins.jenkins.io/cvs - via cglib (via Commons Digester)
https://plugins.jenkins.io/dependency-check-jenkins-plugin - via cglib
(via Commons Digester)
https://plugins.jenkins.io/deploy - via
org.glassfish.main.deployment:deployment-common
https://plugins.jenkins.io/git-changelog - via
com.jayway.jsonpath/net.minidev:json-smart
https://plugins.jenkins.io/github-autostatus - via JNR
https://plugins.jenkins.io/github-pr-coverage-status - via
com.jayway.jsonpath/net.minidev:json-smart
https://plugins.jenkins.io/hp-application-automation-tools-plugin -
com.jayway.jsonpath/net.minidev:json-smart
https://plugins.jenkins.io/jacoco - directly
https://plugins.jenkins.io/jnr-posix-api - via JNR
https://plugins.jenkins.io/kubernetes-pipeline-devops-steps - via JNR
https://plugins.jenkins.io/maven-dependency-update-trigger - via Plexus
https://plugins.jenkins.io/maven-info - via cglib (via Commons Digester)
https://plugins.jenkins.io/multibranch-scan-webhook-trigger -
com.jayway.jsonpath/net.minidev:json-smart
https://plugins.jenkins.io/pipeline-model-definition - directly
https://plugins.jenkins.io/scm-api - directly
https://plugins.jenkins.io/synopsys-coverity - via
com.jayway.jsonpath/net.minidev:json-smart
https://plugins.jenkins.io/teamconcert - via cglib (via Commons Digester)
https://plugins.jenkins.io/token-macro - via
com.jayway.jsonpath/net.minidev:json-smart
https://plugins.jenkins.io/warnings-ng - via
com.jayway.jsonpath/net.minidev:json-smart

These could all be dealt with, but dealing with the long tail
(anything less than a few thousand installations) would be a huge
amount of work. If we expect someone to do that work for all the
plugins listed above, then count me out because the expected value is
just not worth the effort. If, on the other hand, we would be OK with
leaving behind a substantial number of the above plugins (especially
those that have only a few thousand installations and/or have not been
released in years), then I think this could be doable.

[Iván Fernández Calvo]

On previous detaches we faced the same issue 10-15 plugins dependencies and most of them outdated, I remember (tripled-ssh2, sshd), we made a cut on plugins not updated in the last two years (maybe three) that removed most of the problem. Those plugins with more than 2-3 years without updates are difficult to bump the Jenkins core version and dependencies, they do not depend on the BOM and make the update is a nightmare.

Regards
Ivan Fernandez

> El 23 ago 2022, a las 4:22, Basil Crow <m...@basilcrow.com> escribió:
>
> I think detaching is riskier than I expected: a lot of plugins bundle

> --
> You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/NhV_o6zxbzw/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAFwNDjpMgsj%3Dz1ZdF3Uu%3DAex6Nmro3Jm2%3DGzKtpuhTE5S3zcwA%40mail.gmail.com.

[Basil Crow]

Fast forward a little over a year and removing ASM from core in favor
of dynamic linking through a library plugin seems more achievable,
though still quite a bit of work. The following plan seems achievable:

- Create a new ASM library plugin. It would be unused as long as core
keeps shipping ASM, since core's copy would take precedence.
- Make the JNR POSIX library plugin depend on the ASM library plugin.
- Make the JSON Path library plugin depend on the ASM library plugin.
- Make anything that consumes ASM directly depend on the ASM library
plugin (using usage-in-plugins to find all the things).
- Make anything that currently ships the JARs mentioned above depend
on the corresponding library plugin instead (using usage-in-plugins to
find all the things).
- Remove ASM from core.

This is probably a dozen or so PRs to cover plugins with over 10,000
installations, and maybe another dozen or so PRs to cover plugins with
over 1,000 installations, if anyone is interested.

[Valentin Delaye (jonesbusy)]

Jumping into this

- Create a new ASM library plugin. --> Done https://plugins.jenkins.io/asm-api/

  Part of bom since 2718.v7e8a_d43b_3f0b_

- Make the JSON Path library plugin depend on the ASM library plugin --> In progress : https://github.com/jenkinsci/json-path-api-plugin/pull/12

The rest will probably follow step by step, it's a long journey until ASM can be safely removed from core

Regards,

[Basil Crow]

On Sat, Jan 13, 2024 at 5:09 AM Valentin Delaye (jonesbusy)
<jone...@gmail.com> wrote:
>
> Jumping into this

Are you still interested in removing ASM from core? I checked usage in
plugins, and I believe all significant plugins are now linking against
the ASM library plugin. The last major blocker was the JaCoCo plugin,
which was released today. I also checked the CloudBees Update Center
and didn't find any ASM usages there. We are at the beginning of a new
LTS development cycle, so I think now is the ideal time to remove ASM
from core.