Illegal reflective access in Java 9 build

[ssoloff]

The Java 9 build on Travis is reporting a few illegal reflective access violations when running the tests. I may have missed some, but the ones I saw are reproduced below:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.mockito.internal.util.reflection.AccessibilityChanger (file:/home/travis/.gradle/caches/modules-2/files-2.1/org.mockito/mockito-core/2.13.0/8e372943974e4a121fb8617baced8ebfe46d54f0/mockito-core-2.13.0.jar) to field java.util.logging.LogManager.props
WARNING: Please consider reporting this to the maintainers of org.mockito.internal.util.reflection.AccessibilityChanger
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.postgresql.jdbc.TimestampUtils (file:/home/travis/.gradle/caches/modules-2/files-2.1/org.postgresql/postgresql/42.1.4/1c7788d16b67d51f2f38ae99e474ece968bf715a/postgresql-42.1.4.jar) to field java.util.TimeZone.defaultTimeZone
WARNING: Please consider reporting this to the maintainers of org.postgresql.jdbc.TimestampUtils
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

We should investigate if these issues have already been reported to the Mockito and Postgres projects, and, if not, open new issues, as necessary.

The suggestion to use --illegal-access=warn may mean that only the first such violation is reported in each process. (The Postgres warning is from the integration tests, while the Mockito warning is from the unit tests, so they are indeed run in separate processes.) We may want to enable that option to see if there are any others.

---

External dependencies with illegal reflective access:

• Gradle [GROOVY-8339]
• GuidTest (EqualsVerifier) [Illegal reflective access on Java 9/10 jqno/equalsverifier#207]
• LoggingConfigurationTest [no longer exists]
• PointFileReaderWriterTest (Mockito) [Fix illegal reflective access in PointFileReaderWriterTest #4027]
• Postgres JDBC driver [Update postgres driver version #2866]
• TripleAApplicationTest (TestFX) [ApplicationLauncherImpl: Illegal reflective access TestFX/TestFX#553]

[RoiEXLab]

@ssoloff Already mentioned this in the java 9 PR
pgjdbc/pgjdbc#986

[ssoloff]

@RoiEXLab Derp, should have read that closer. 😄 Thanks for noting that the Postgres violation has already been reported.

[RoiEXLab]

@ssoloff #2866

[RoiEXLab]

@ssoloff Dug into this a little bit deeper.
The gradle issue is actually a groovy issue being tracked here: https://issues.apache.org/jira/browse/GROOVY-8339 but it doesn't seem like it's going to get fixed soon as long as everything is still working fine.
And the mockito issue is actually not a mockito issue at all. Haven't tested this but I'm pretty sure the illegal reflective acces comes from our usage of the spying mechanism in LoggingConfigurationTest on LogManager. I'll try to simply mock an instance and see if that works as well.

[ssoloff]

This is one of several issues blocking us from moving to Java 9+. (Well, this one is probably more of a Java 11+ blocker, because the reflective operations are only deprecated in Java 9 and 10.) I added a new label for these types of issues.

As @RoiEXLab pointed out elsewhere, we probably need to start jumping on these issues. Java 11 LTS is supposed to be released this month, and Java 8 LTS his end-of-life in December 2020 for personal use (commercial use end-of-life is January 2019, but no one plays TripleA at work, right? 😄).

We continue to have users reporting the inability to run TripleA using Java 9 and 10. Presumably, once Java 11 is released, we're going to see an uptick in various distros pushing Java 11 as the default over Java 8, which is going to cause even more problems. New players may be dissuaded from playing if they have to go out of their way to install Java 8.

Given these considerations, I'm going to re-open these Java 9+ issues and label them appropriately.

[ssoloff]

I re-ran all unit and integration tests under Java 9 with --illegal-access=warn set. Based on the results, I updated the list of current illegal accesses in the issue description.

[RoiEXLab]

I'd like to point out that this is not a real blocker.
Regardless of the warning everything works up to JDK 11 AFAIK.
I don't think that oracle set a fixed version when the warning turns into an actual exception.

[ssoloff]

Yeah, I added a parenthetical to that effect above, but didn't think to create a separate label at that time. Should we have a separate label for "Java Module System Incompatibility" (or something like that)? Or just drop the label altogether?

[ssoloff]

I changed the label from "Java 9+ Blocker" to "Java 9+ Deprecation". The description of the latter indicates it does not currently block migration to Java 9+.

[DanVanAtta]

@ssoloff is this actionable? What remains to be done? I'll note too that tickets out of SLA should not be revived simply because we want them to be open. Tracking action items that have been opened for over half a year seems to indicate something is wrong.

Maybe instead we should have a Java9 project? That project can have a list of things that need to be done, and actionable requests of the TripleA team could then be created as new issue tickets.

[ssoloff]

@DanVanAtta It is currently actionable. The EqualsVerifier issue is in the process of being fixed by the maintainer. When that's complete, we can upgrade the dependency in our build.

The Gradle and TestFX issues will not be actionable until their respective maintainers have addressed the underlying issue (again, our action will be to upgrade the dependency).

Maybe instead we should have a Java9 project?

Yeah, that could work.

[DanVanAtta]

@ssoloff this is in danger of getting ice-boxed.

Is there any chance we will pick this issue up soon to complete it?

May I recommend we create a new issue summarizing the remaining 2 tasks and reference this one. The hope/intent there is the tracking would be lighter-weight. Let me know if that just seems like paper-shuffling. FWIW the lengthy history here I think means only the original participants are sufficiently over the entry-barrier that they would be the only ones to complete this. Should we just assign this to the conversation participants thus far?

[ssoloff]

Is there any chance we will pick this issue up soon to complete it?

I'm monitoring the two open third-party issues. Once they are resolved, we can fix our own code.

FWIW the lengthy history here I think means only the original participants are sufficiently over the entry-barrier that they would be the only ones to complete this.

I think all the relevant information, including what's left to do, is summarized concisely in the description. Thus, a new participant should not have to read the 12+ comments to get up to speed. Not sure what would be gained by closing this issue and opening a new one.

[DanVanAtta]

@ssoloff any updates on this issue?

[ssoloff]

Both GROOVY-8339 and TestFX/TestFX#553 appear to still be unresolved.

[DanVanAtta]

@ssoloff I'm trying to close any old tickets that do not have a 'problem' label, and I have trouble really calling this a problem. If the error message is potentially going to be a point of confusion for dev's, could we document it and then remove the documentation when it's fixed?