> 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. On Thursday, June 10, 2021 at 6:34:29 PM UTC+1 jn...@cloudbees.com wrote: > 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 > On Thursday, June 10, 2021 at 6:03:41 PM UTC+1 jn...@cloudbees.com wrote: > >> > 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: >> >>> 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. >>> >> -- 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-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/d0706fe3-c623-4e6b-9a84-25db0d6c6924n%40googlegroups.com.
