Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add openJDK 20 compatibility #6758

Closed
wants to merge 1 commit into from
Closed

Add openJDK 20 compatibility #6758

wants to merge 1 commit into from

Conversation

napoly
Copy link
Contributor

@napoly napoly commented Jul 7, 2023 •
edited
Loading

Title says it all. Had to update Gradle, Mockito, johnrengelman and kotlin plugins..

@napoly napoly force-pushed the openjdk-20-compatibility branch 3 times, most recently from b0b5a20 to 21f56ec Compare July 7, 2023 13:01
alvasw
alvasw suggested changes Jul 9, 2023
View reviewed changes
Copy link
Contributor

@alvasw alvasw left a comment

Choose a reason for hiding this comment

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

NACK
Bisq crashes at startup.

@HenrikJannsen
Copy link
Collaborator

What are the concrete improvements? We are conservative with updates due security risks (not about the JDK but about 3rd party libs).

@napoly
Copy link
Contributor Author

napoly commented Jul 10, 2023

NACK Bisq crashes at startup.

can you please share the logs..? works fine for me under different jdk versions..

What are the concrete improvements? We are conservative with updates due security risks (not about the JDK but about 3rd party libs).

There is no improvement from user perspective here. But as a developer one can run the newest production ready JDK with all the improvements. I also did not add any new 3rd party lib. Just updated the ones that were already used.

@napoly napoly marked this pull request as draft July 12, 2023 09:08
@alvasw
Copy link
Contributor

alvasw commented Jul 14, 2023

can you please share the logs..? works fine for me under different jdk versions..

Start Bisq and click on "Funds" in the top menu.
Logs:

Jul-14 11:47:49.086 [JavaFX Application Thread] ERROR b.c.s.CommonSetup: Uncaught Exception from thread JavaFX Application Thread 
Jul-14 11:47:49.086 [JavaFX Application Thread] ERROR b.c.s.CommonSetup: throwableMessage= Cannot invoke "javafx.scene.Node.getLayoutBounds()" because "this.textNode" is null 
Jul-14 11:47:49.087 [JavaFX Application Thread] ERROR b.c.s.CommonSetup: throwableClass= class java.lang.NullPointerException 
Jul-14 11:47:49.087 [JavaFX Application Thread] ERROR b.c.s.CommonSetup: Stack trace:
java.lang.NullPointerException: Cannot invoke "javafx.scene.Node.getLayoutBounds()" because "this.textNode" is null
        at bisq.desktop.components.JFXTextFieldSkinBisqStyle.updateTextPos(JFXTextFieldSkinBisqStyle.java:95)
        at bisq.desktop.components.JFXTextFieldSkinBisqStyle.layoutChildren(JFXTextFieldSkinBisqStyle.java:79)
        at javafx.scene.control.Control.layoutChildren(Control.java:601)
        at javafx.scene.Parent.layout(Parent.java:1207)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Parent.layout(Parent.java:1214)
        at javafx.scene.Scene.doLayoutPass(Scene.java:576)
        at javafx.scene.Scene$ScenePulseListener.pulse(Scene.java:2476)
        at com.sun.javafx.tk.Toolkit.lambda$runPulse$2(Toolkit.java:413)
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
        at com.sun.javafx.tk.Toolkit.runPulse(Toolkit.java:412)
        at com.sun.javafx.tk.Toolkit.firePulse(Toolkit.java:439)
        at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:563)
        at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:543)
        at com.sun.javafx.tk.quantum.QuantumToolkit.pulseFromQueue(QuantumToolkit.java:536)
        at com.sun.javafx.tk.quantum.QuantumToolkit.lambda$runToolkit$11(QuantumToolkit.java:342)
        at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)
        at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
        at com.sun.glass.ui.gtk.GtkApplication.lambda$runLoop$11(GtkApplication.java:277)
        at java.base/java.lang.Thread.run(Thread.java:1623)

But as a developer one can run the newest production ready JDK with all the improvements.

I haven't looked at the updated libraries yet. What are the concrete improvements?
We are conservative and only update 3rd party libraries if we have too.

@napoly napoly force-pushed the openjdk-20-compatibility branch 4 times, most recently from 525cf14 to c14f67e Compare July 14, 2023 18:10
@napoly
Copy link
Contributor Author

napoly commented Jul 14, 2023 •
edited
Loading

Thanks @alvasw for sharing the log! (seems similar to #5624) I was able to simulate and the stacktrace is preceded by java.lang.reflect.InaccessibleObjectException: Unable to make boolean java.lang.reflect.AccessibleObject.setAccessible0(boolean) accessible: module java.base does not "opens java.lang.reflect" to unnamed module @4738a206 which means that the warning that you might saw before:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.jfoenix.adapters.ReflectionHelper (file:/Users/napoly/git/haveno/lib/jfoenix-9.0.10.jar) to method java.lang.reflect.AccessibleObject.setAccessible0(boolean)
WARNING: Please consider reporting this to the maintainers of com.jfoenix.adapters.ReflectionHelper
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

has changed to throw exception in Java 17+.. and we need to add javafx libraries explicitly so it can be reached without reflection.. so this should work now (please give it a try)

While updating the libs I did hit the issue with JFXProgressBar so I changed it for pure JavaFX ProgressBar.

What are the concrete improvements?

Working with newest production ready tech stack.....

@napoly napoly requested a review from alvasw July 14, 2023 18:30
@napoly napoly marked this pull request as ready for review July 14, 2023 18:33
@napoly napoly force-pushed the openjdk-20-compatibility branch from c14f67e to 72d9efe Compare July 17, 2023 19:01
alvasw
alvasw suggested changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@alvasw alvasw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @napoly,
Unfortunately, updating the JDK to a version above 15 breaks many things in the JFoenix library. The worst part is that all exceptions are triggered at run-time, making finding the issues extremely hard. I cloned and debugged the upstream project to find the root causes. The upstream build.gradle file cheats by manually exporting the needed classes (see build.gradle).

You can see the compile errors after targeting OpenJDK 20 and JavaFX 20 in the upstream project. So the best thing to do is to fork JFoenix, fix the root cause, and point Bisq to the fixed library. I'm not sure if you want to do it. Do you want to do it?

@napoly
Copy link
Contributor Author

napoly commented Jul 21, 2023

hmm.. sounds like a suicide mission.. if I look closely at JFoenix repository it seems more less abandoned.. I unfortunately don't think it's in my powers to keep that repo alive and thriving.. could we just drop it? as I don't think it adds that much value.. as far as I understand it just adds "perky" stuff like ripple effect over text field, secondary progress bar..... we should be fine with pure JavaFX and if not then use some up to date lib like MaterialFX

@HenrikJannsen
Copy link
Collaborator

I don't think we should invest in such a large effort. In Bisq 2 we have avoided JFoenix and developed our own components, but there is still stuff missing like input validation support. Doing that in Bisq 1 would not add any value to the user and as the project has a limited lifetime the lack of upgrade compatibility is less critical.

@napoly
Copy link
Contributor Author

napoly commented Jul 23, 2023

OK so I'm closing this one.. according to https://endoflife.date/java there is a security update till 30 Sep 2026 for Java 11 LTS

@napoly napoly closed this Jul 23, 2023
@alvasw alvasw mentioned this pull request Aug 18, 2023
Closed
@napoly napoly deleted the openjdk-20-compatibility branch March 13, 2024 06:57
emilazy added a commit to emilazy/nixpkgs that referenced this pull request Oct 7, 2024
@emilazy
bisq-desktop: mark as broken
9d3560b 
OpenJFX 11 has been EOL for over a year; upstream
uses OpenJFX 15 which is also EOL. According
to <bisq-network/bisq#6758>, the
application does not function with later versions, and has a
limited remaining lifespan as there is a new major version that
uses more modern versions. It looks like that version officially
supports OpenJDK 22, which is also EOL, but hopefully it will
work with 21 or 23. Some work was done already to package it here:
<NixOS#318594>.
wrbbz pushed a commit to wrbbz/nixpkgs that referenced this pull request Oct 9, 2024
@emilazy @wrbbz
bisq-desktop: mark as broken
58b922d 
OpenJFX 11 has been EOL for over a year; upstream
uses OpenJFX 15 which is also EOL. According
to <bisq-network/bisq#6758>, the
application does not function with later versions, and has a
limited remaining lifespan as there is a new major version that
uses more modern versions. It looks like that version officially
supports OpenJDK 22, which is also EOL, but hopefully it will
work with 21 or 23. Some work was done already to package it here:
<NixOS#318594>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvasw alvasw alvasw requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@napoly @HenrikJannsen @alvasw