[JENKINS-34064] Groovy 2.x fixes

[abayer]

JENKINS-34064

Subsumes #24

I did a local build of core with Groovy 2.4.11, modified workflow-cps to roll back jenkinsci/workflow-cps-plugin@efeee2b, and ran the workflow-cps tests against that tweaked core and this snapshot...and everything passed.

I know DGMPatcher is not the ideal way to do this, but given that CategorySupport isn't viable due to it affecting the whole thread, I'd say this is worth doing as is. I'd really like to get this in so that I can work on adding more patched methods for things like .collect, .collectEntries, .find, etc...

cc @reviewbybees

[jglick]

given that CategorySupport isn't viable due to it affecting the whole thread

FTR I never proposed using CategorySupport itself, only the internal APIs it in turn calls.

[jglick]

Looks promising. I need to look at this in detail. Agreed that we would like to get this in ASAP.

[jglick]

modified workflow-cps to roll back jenkinsci/workflow-cps-plugin@efeee2b

I.e., part of jenkinsci/workflow-cps-plugin#15.

Probably we would actually want to maintain a list of methods which were successfully patched, at least until we get automatic DGMPatcher source code generation to work. So I think what you actually meant to do was update, and then test, jenkinsci/workflow-cps-plugin#14.

ran the workflow-cps tests against that tweaked core and this snapshot...and everything passed

Does not prove that much since you were presumably not running SerializationTest.eachClosure, as above, which is the integration test demonstrating the patching actually works in a somewhat more realistic environment. (Which is by no means obvious, since there are a lot of nasty ordering effects with the DGMPatcher trick: it is mutating the behavior of the groovy.jar bundled in Jenkins core, yet some Groovy code is already run by the time we do so—such as CpsTransformer itself—and Groovy tends to keep various metadata caches which you may or may not be clearing or updating consistently.)

[abayer]

@jglick Nope, I was running SerializationTest.eachClosure specifically.

I'm 100% sure there's problems with this still in cases we don't have test coverage for due to real-world things, and am still entirely open to revisiting it and actually trying to use the APIs CategorySupport uses (though I looked into that a while back and my brain hurt, so that's probably more involved than I can do quickly). But what I did was as follows:

• Build Jenkins 2.60-SNAPSHOT (after Upgrade to Groovy 2.4.11 jenkinsci/jenkins#2814 was merged) locally.
• Build this SNAPSHOT of groovy-cps as 1.13-SNAPSHOT, again locally.
• Change workflow-cps to have <jenkins.version>2.60-SNAPSHOT</jenkins.version> and groovy-cps version 1.13-SNAPSHOT.
• Also set <java.level>8</java.level> in workflow-cps because core 2.60-SNAPSHOT requires 8.
• Manually roll back some of https://github.com/jenkinsci/workflow-cps-plugin/pull/15/files - specifically, replacing return checkJenkins26481(args, method, method.getParameterTypes()) || delegate.permitsMethod(method, receiver, args) with return method.getDeclaringClass() || delegate.permitsMethod(method, receiver, args) (and equivalent for the other invocations of checkJenkins26481). No other changes, just cutting checkJenkins26481 out of the loop.
• Removed @Ignore("TODO backed out for JENKINS-34064") from SerializationTest#eachClosure.
• Ran mvn clean install
• Smiled happily as everything passed, including SerializationTest#eachClosure. =)

[jglick]

Got it. Yeah that looks right. Will be more reproducible if we deploy snapshots.

In the meantime I am taking another look at GroovyCategorySupport. While I had earlier been drawn to the idea of using its internal calls like

DefaultMetaClassInfo.setCategoryUsed(true);
VMPluginFactory.getPlugin().invalidateCallSites();

I now see that this is likely not going to work, as these are basically markers, while tons of other Groovy core code calls back into GroovyCategorySupport. IOW categories are baked into method delegation logic rather than a feature added modularly using lower-level facilities as I had hoped.

OTOH my original reasons for rejecting GroovyCategorySupport itself may not actually be blockers:

• Yes it uses a ThreadLocal, and so we cannot make [Object.]use from CPS-transformed code work. But we might be able to call GroovyCategorySupport.use from Java code in workflow-cps or groovy-cps, such as in CpsThreadGroup.run or Continuable.run0 or even CpsVmExecutorService.wrap. The reason is that all the CPS-transformed methods are in fact called from a predictable Thread, even though this dynamic scope has nothing to do with the block structure of the Groovy code.
• I just assumed that a category could only add methods, not replace methods available elsewhere, particularly DefaultGroovyMethods. But in fact

class XTest {
    @Test
    public void x() {
        Base x = new Base()
        use(Cat) {
            println "x.m1()=${x.m1()} x.m2()=${x.m2()} x.asBoolean()=${x.asBoolean()}"
        }
    }
    public static class Base {
        public String m1() {"base-m1"}
    }
    public static class Cat {
        public static String m1(Base o) {"cat-m1"}
        public static String m2(Base o) {"cat-m2"}
        public static String asBoolean(Base o) {"cat-asBoolean"}
    }
}

prints what we would want:

x.m1()=cat-m1 x.m2()=cat-m2 x.asBoolean()=cat-asBoolean

Note that the category is replacing Base.m1 and DefaultGroovyMethods.asBoolean.

[abayer]

Oh yeah, categories can replace methods for sure.

So you're thinking that we can do GroovyCategorySupport without the CpsDefaultGroovyMethods versions replacing the DefaultGroovyMethods in @NonCPS code called from CPS-transformed code?

[jglick]

Superseded by #56, we hope.

[abayer]

Yup, closing.