Skip to content

GROOVY-10730: Bump Gradle to 7.5.1#1771

Merged
daniellansun merged 1 commit into
apache:masterfrom
paulk-asert:groovy10730
Aug 28, 2022
Merged

GROOVY-10730: Bump Gradle to 7.5.1#1771
daniellansun merged 1 commit into
apache:masterfrom
paulk-asert:groovy10730

Conversation

@paulk-asert
Copy link
Copy Markdown
Contributor

No description provided.

// def jdk9 = ['-Djava.locale.providers=COMPAT,SPI', '--illegal-access=debug']
if (JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_16)) {
jdk9 += ["--add-opens=java.base/java.lang=ALL-UNNAMED",
"--add-opens=java.base/java.util=ALL-UNNAMED"]
Copy link
Copy Markdown
Contributor

@daniellansun daniellansun Aug 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the options --add-opens added, we may not be able to find Groovy compiler issues related to JPMS in time.

P.S. Is this a bug of new version of Gradle? We don't need the two option when using the old version of gradle.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old version of gradle always added those two add-opens implicitly for test workers:
https://docs.gradle.org/7.5/userguide/upgrading_version_7.html#removes_implicit_add_opens_for_test_workers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, we want to eventually remove them. They are hiding a couple of bugs for us which we hadn't noticed before. Once we fix those bugs, we should remove.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old version of gradle always added those two add-opens implicitly for test workers: https://docs.gradle.org/7.5/userguide/upgrading_version_7.html#removes_implicit_add_opens_for_test_workers

Got it ;-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remaining issue seems just related to accessing clone illegally, e.g. AutoClone.
( https://github.com/apache/groovy/runs/8015134704?check_suite_focus=true )

org.codehaus.groovy.transform.ImmutableTransformTest > testCloneableField FAILED
    java.lang.IllegalAccessException: class org.codehaus.groovy.reflection.CachedMethod cannot access a member of class java.lang.Object (in module java.base) with modifiers "protected native"
        at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:392)
        at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:674)
        at java.base/java.lang.reflect.Method.invoke(Method.java:560)
        at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:343)
        at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:328)
        at groovy.lang.MetaClassImpl.doInvokeMethod(MetaClassImpl.java:1372)
        at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1104)
        at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1010)
        at org.codehaus.groovy.runtime.InvokerHelper.invokePogoMethod(InvokerHelper.java:610)
        at org.codehaus.groovy.runtime.InvokerHelper.invokeMethod(InvokerHelper.java:[593](https://github.com/apache/groovy/runs/8015134704?check_suite_focus=true#step:5:594))
        at org.codehaus.groovy.runtime.ReflectionMethodInvoker.invoke(ReflectionMethodInvoker.java:50)
        at org.codehaus.groovy.vmplugin.v8.IndyInterface.fromCache(IndyInterface.java:318)
        at Lab.<init>(Script1.groovy)
        at org.codehaus.groovy.vmplugin.v8.IndyInterface.fromCache(IndyInterface.java:318)
        at Script1.run(Script1.groovy:14)
        at groovy.lang.GroovyShell.evaluate(GroovyShell.java:460)
        at groovy.lang.GroovyShell.evaluate(GroovyShell.java:495)
        at groovy.lang.GroovyShell.evaluate(GroovyShell.java:469)
        at groovy.lang.GroovyShell$evaluate$0.call(Unknown Source)
        at groovy.test.GroovyShellTestCase.evaluate(GroovyShellTestCase.groovy:26)
        at groovy.test.GroovyShellTestCase$evaluate.callCurrent(Unknown Source)
        at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:49)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:171)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:185)
        at org.codehaus.groovy.transform.ImmutableTransformTest.testCloneableField(ImmutableTransformTest.groovy:113)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And from memory, a few tests in groovy-templates. Plus legal usage of clone seems broken too:

int[] nums = [1, 2, 3]
nums.clone()

==>
BUG! Unknown transformation for argument [I@503a20ea at position 0 with class [I for parameter of type class

Copy link
Copy Markdown
Contributor

@daniellansun daniellansun Aug 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And from memory, a few tests in groovy-templates. Plus legal usage of clone seems broken too:

int[] nums = [1, 2, 3]
nums.clone()

==> BUG! Unknown transformation for argument [I@503a20ea at position 0 with class [I for parameter of type class

I added a test case for the above code, fails with gradle 7.5.1
ce15eae

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is the same with the groovyConsole. Passes if you add the --add-open params but fails otherwise.

Copy link
Copy Markdown
Contributor

@daniellansun daniellansun Aug 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG... I had thought I could created a new branch based on the PR... but I added the experimental commits to the PR at last...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniellansun daniellansun merged commit 69b88f0 into apache:master Aug 28, 2022
@daniellansun
Copy link
Copy Markdown
Contributor

Merged. Thanks!

@paulk-asert paulk-asert deleted the groovy10730 branch August 30, 2022 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants