RFC: Fixes for recent JDK support (eg JDK 17)

[ian-arkver]

Feature Request / Request for Comment

Update dependencies and other fixes to allow OpenPnP to run with a recent Java VM.

I didn't find an online list, or any open Issues describing the current blockers for running with a more recent JDK,
so thought I'd create this as a place to collect the information and discussion.

Currently OpenPnP requires an old, insecure JDK version to work properly due to illegal reflective access failures in various places, for example when trying to write java.awt.Color to machine.xml. There are probably other places, possibly within package dependencies. These may be harder to work around than the java.awt.Color one.

Since JEP403 was implemented in Java 17, there's now no command line option to re-enable illegal reflective accesses.

How Will It Look?

There should hopefully be no changes to the user experience, unless some UI element dependencies are badly broken by Java 17.

Why Do We Need It?

1. What problem does it solve?

Fixing this would avoid a common problem where new users try to run OpenPnP on a too recent Java version and report that it doesn't work.

Presumably the driver behind JEP403 and its predecessors was to improve security of the Java VM.

2. Is it useful for everyone, or does it only solve a problem for your machine?

This could be useful to many people, especially new users installing it on a new machine.

Provide Examples

N/A

[markmaker]

Hi @ian-arkver

I just wanted to make it clear that while I've found my way around OpenPnP, I'm in no way a Java expert. I'm clueless as to how one would identify and resolve these issues. So help is greatly appreciated.

To make it clear: Don't hold your breath for me to fix this.

_Mark

[ian-arkver]

Thanks Mark, understood. Me neither. However if we have a place where we can collate what's broken then those with some coding time and the necessary knowhow will have a place to start.

I don't know whether it's just a matter of a few missing accessors or an explicit serialiser/deserialiser for some data type or whether something more fundamental will break like maybe "beans bindings won't work and the whole subsystem needs replaced" [Not saying beans bindings are broken, just an example].

For java.awt.Color there's some information here and some maybe relevant code here, though we'd need to make sure backward compatability with existing machine.xml files was preserved, i.e. the color was serialised to/from the existing format.

[vonnieda]

I would be happy to help with this. @ian-arkver if you are willing to start tracking down some of the broken things and noting them here than I can jump into the code and try to get them fixed.

[ian-arkver]

Thanks Jason, I'll see what I can do.

[ian-arkver]

Interestingly there's already a custom serialiser in the code for awt.Color, eg. in CvStage/DrawCircles:

    @Element(required = false)
    @Convert(ColorConverter.class)
    private Color color = null;

And this ColorConverter class uses getRed etc. as the example code I linked to does, so we end up with

                  <cv-stage class="org.openpnp.vision.pipeline.stages.DrawCircles" name="display" enabled="true" circles-stage-name="results" thickness="1">
                     <color r="255" g="0" b="0" a="255"/>
                  </cv-stage>

So either the @Convert attribute isn't working, or there are some Color class members that don't have it.

[ian-arkver]

The only Color I could see that might end up in XML that didn't have the @Convert was the Issue class in Solutions.java. I added it but the tests still failed in the same way (failure to write machine.xml). Maybe there's another one.

I fired the UI up under the debugger in VSCode and it works since VSCode uses the JDK that's specified in the pom.xml for the maven compiler plugin. This was set to 1.8 so I changed it to 17 (requires this without the 1. now) and rebuilt the package (by disabling the tests). Now both openpnp.sh and VSCode launcher give the same error, but it's a different error...

Exception in thread "main" java.lang.reflect.InaccessibleObjectException: Unable to make protected final java.lang.Class java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain) throws java.lang.ClassFormatError accessible: module java.base does not "opens java.lang" to unnamed module @45820e51
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(Unknown Source)
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(Unknown Source)
        at java.base/java.lang.reflect.Method.checkCanSetAccessible(Unknown Source)
        at java.base/java.lang.reflect.Method.setAccessible(Unknown Source)
        at javassist.util.proxy.SecurityActions.setAccessible(SecurityActions.java:159)
        at javassist.util.proxy.DefineClassHelper$JavaOther.defineClass(DefineClassHelper.java:213)
        at javassist.util.proxy.DefineClassHelper$Java11.defineClass(DefineClassHelper.java:52)
        at javassist.util.proxy.DefineClassHelper.toClass(DefineClassHelper.java:260)
        at javassist.ClassPool.toClass(ClassPool.java:1240)
        at javassist.ClassPool.toClass(ClassPool.java:1098)
        at javassist.ClassPool.toClass(ClassPool.java:1056)
        at javassist.CtClass.toClass(CtClass.java:1298)
        at org.openpnp.Main.monkeyPatchBeansBinding(Main.java:101)
        at org.openpnp.Main.main(Main.java:121)

The tests still fail with the same problem writing machine.xml, so I guess they're run without invoking the monkeyPatch. Maybe they don't call Main. For example BasicJobTest can be run:
mvn -e -Dtest=BasicJobTest -DskipTests=false test
and the stack trace shows it failing in the xml serialiser:

java.lang.Exception: Error while saving machine.xml (Unable to make field int java.awt.Color.value accessible: module java.desktop does not "opens java.awt" to unnamed module @1d76aeea)
	at org.openpnp.model.Configuration.save(Configuration.java:409)
	at BasicJobTest.testSimpleJob(BasicJobTest.java:61)
... lots of maven surefire trace ...
Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make field int java.awt.Color.value accessible: module java.desktop does not "opens java.awt" to unnamed module @1d76aeea
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:178)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:172)
	at org.simpleframework.xml.core.FieldScanner.process(FieldScanner.java:247)
... lots of xml serialiser trace...
	at org.simpleframework.xml.core.Persister.write(Persister.java:1222)
	at org.openpnp.model.Configuration.serializeObject(Configuration.java:519)
	at org.openpnp.model.Configuration.saveMachine(Configuration.java:535)
	at org.openpnp.model.Configuration.save(Configuration.java:406)

VSCode shows 18 errors during compile, all in EagleLoader.java for some reason. It also shows > 1300 warnings and 5 critical vulnerabilities in packages in pom.xml. I've not looked at any of these yet.

I've pushed the few changes I made so far to the jdk17_hack branch in my repo.

[ian-arkver]

I commented out the test serialization before the machine.xml file write so we get the partially written file before the exception is thrown.

Comparing the Java 17 version with the Java 8 version we can see that the next element to be written would have been:

                  <cv-stage class="org.openpnp.vision.pipeline.stages.DrawContours" name="7" enabled="true" contours-stage-name="9" thickness="2" index="-1">
                     <color r="255" g="255" b="255" a="255"/> 
                  </cv-stage>

which is an element with the custom serializer. So I guess this isn't working with 17, at least when run by the testbench.

[vonnieda]

I'm re-reading through this and the other related tickets and one thing occoured to me: We do already support Java up to 15. We're not limited to 8. The only reason we ship the installers with 8 is because I've been lazy about updating those. We could update them to Java 15 today with no changes.

You can see the list of versions officially supported here: https://github.com/openpnp/openpnp/blob/develop/.github/workflows/build_and_deploy.yml#L13

So, if the goal is just to get on a more recent version of the bundled JRE in the installers, that can be done now just by updating the installer.

[ian-arkver]

Defaulting to 15 in the pom and installer would be an improvement in my opinion, assuming that 15 is a more up-to-date and secure JRE than 8. I'm not sure whether this would break upgrades for people with only JRE 8 installed. I don't use the installers though so maybe they sidestep this problem for most people.

Note also comments by Niclas here which imply getting rid of all illegal reflective accesses may be non-trivial.

[ian-arkver]

Recent discussion in the group...
https://groups.google.com/g/openpnp/c/2cUIHXxVpXQ
(Thanks "tmasti..." whoever you are)

This brought to my attention --add-opens java commandline flag which can be used to open up the private internals of selected modules, thereby defeating the illegal reflective access restrictions.

If you add the following two flags:
--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.desktop/java.awt=ALL-UNNAMED
then all the builtin maven tests pass when compiled and tested using OpenJDK 17.

Perhaps this is a solution to getting JDK17+ supported, though disabling the new restrictions in this way seems a little dubious.

If there's interest I can submit a PR with my pom changes. The flags would need added to the shell and .bat launchers too.

[markmaker]

@ian-arkver, thanks for keeping an eye on this!

This is all way above my Java knowledge.

The question is: What's the harm? I see no harm.

1. OpenPnP is not an internet fronting application.
2. The command-line is just effective for that one process, i.e. we are not subverting Java security in any global way.
3. We can always take it back.
4. We can always find a better solution.
5. It would (it seems) remove the increasingly frequent Java 17 support case.

So IMHO, I would happily accept a PR.

Perhaps @vonnieda can assess this too?

_Mark

[vonnieda]

It's fine with me. Eventually we will need to modernize the codebase a bit but I see no problem with kicking the can down the road a little further.

[ian-arkver]

I think the --add-opens option was added in Java 9, so we can't add it for the default packaged Java 8. We'd need to change the target version in the pom, the bundled JRE version in the installers and probably the launch scripts - all at the same time.

I'm also not sure how this would work with the installer and upgrading from a previous bundled JRE. I'd hope Install4j would handle it.

I also don't really know how the installer launches it? Does it use the sh/bat or does it create its own launchers? I've never personally used a prebuilt version.

A last bit of paranoia... is JRE 17 even available for all the supported platforms and OSes? OpenPnP users run it on quite a range of kit.

[vonnieda]

The installer launches it directly, and you can specify additional command line arguments to it, so we can change it there eventually too, if we need to. But for now we could just leave the bundled version at JVM 8 and we don't need to worry about it.

In other words, I'd focus on addressing specifically the users that are having the issue, and not worry about the ones who aren't. The people using the bundled JVM should be fine.

Alternately, if you really want to dig into this, I'd support updating the installers and everything else to the latest JVM, and making whatever changes we need to support it first class.

[ian-arkver]

ok, well maybe a 2 phase approach then?

• Update the pom maven-compiler target and the test runner args and the sh and bat scripts and try it out on "test" to see what else is broken
• Switch the bundled JRE in the prebuilt packages once the dust has settled

I'll do a PR for the first phase shortly.

[ian-arkver]

See #1398 though it clearly needs some more work.

I'm unsure how the GH actions set up their JRE's. Perhaps there should only be one Java 17 one? Did this only work in the past because the target was lower than all the tested JRE versions?

[gxurma]

Hi Jason, Hi Mark!
Is this still not working correctly?
I just cloned onto new windows 11 machine with JDK 21, and still get this compile problem, that it can not alter the machine.xml etc.
any suggestions where to start the bugfix?

[ian-arkver]

"Works For Me" with the latest test branch and OpenJDK 21 on Linux...

[INFO] Results:
[INFO]
[INFO] Tests run: 235, Failures: 0, Errors: 0, Skipped: 0

java --version
openjdk 21 2023-09-19
OpenJDK Runtime Environment (build 21+35)
OpenJDK 64-Bit Server VM (build 21+35, mixed mode, sharing)

Which branch did you check out and build?

[gxurma]

latest:

git status
On branch develop
Your branch is up to date with 'origin/develop'.
nothing to commit, working tree clean

java --version
java 21 2023-09-19 LTS
Java(TM) SE Runtime Environment (build 21+35-LTS-2513)
Java HotSpot(TM) 64-Bit Server VM (build 21+35-LTS-2513, mixed mode, sharing)

[gxurma]

Oh and it is Windows 11

[ian-arkver]

I don't think the JDK fixes are on develop yet. Try the test branch.

[gxurma]

oh, test branch works

[Team1Electronics]

Just wanted to reconfirm: developer branch does not work (yet) when using openjdk 17.0.10 2024-01-16 (debian 12) but test branch does indeed seem to work.

[ian-arkver]

@vonnieda @markmaker The "add-opens" workarounds have been on the test branch for a while and people seem to be using them successfully. These are just a sticky-plaster over actually fixing the library dependencies to work without the "opening". However they're most likely the best we're going to get any time soon.

I think it might be time to merge them onto the develop branch. Are you planning another test->develop merge Mark?

[markmaker]

We could, after properly announcing it and calling for extended testing. We would have to freeze features, though, and only allow for fixing bugs.

[ian-arkver]

Absolutely, bringing test down onto develop is a lot of work, and I don't think it's worth that just for these JDK workarounds. I was really only asking to see if you were planning one anyway for other reasons. I haven't been following closely what all has landed in test recently. In the meantime anyone needing new JDK can use test.