-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Upgrade JavaFX to v15 #4242
Upgrade JavaFX to v15 #4242
Conversation
Hmm seems the Travis compiler is complaining: "Version 55" means Java 11, so it would only work if upgraded to OpenJDK 11 or later. There's already an issue to move to Java 11: #1787 -- perhaps it can be combined with this one? Java 11 is the latest LTS version (until 2021), but the latest JDK available is v14 which could bring other improvements in the build process like #3373 #3402 and #4196 (all depending on the latest jpackager which is delivered with Java 14). So going directly to OpenJDK 14 could be worth it. But even just Java 11 would be enough for this change. Locally I have Java 11, so compiling and running worked fine for me. |
FYI: Trying to upgrade the build settings + dependencies to work with Java 14. See #1787 (comment) for current reasoning why v14 (and not an earlier one). Not aware yet of any reasons to not go with Java 14 -- but in the meantime I thought I'd just go ahead and try, see if it works. Locally it works, but the build server seems to need more fine-tuning. |
Alright, buildserver checks are all green. Seems Bisq could theoretically upgrade to Java 14. @cbeams / @devinbileck : you guys both worked on similar issues in the past (#1787, #1792). Could you please have a look at this PR and shortly let me know if I missed anything? |
As @ghubstan mentioned in #4048 (comment):
I hadn't noticed the packager tool was still in incubation, I thought it's GA. Then I'd say we can put this PR on hold and have another look at it when Although alternatively, there is always the option to just move away from trying to ship native apps, an effort which binds Bisq to
Instead, the Bisq team could get rid of that overhead and simply deliver the (signed) source code + 3 platform-dependent start scripts. Basically all that needs to happen is
Some script magic could hide away the console output, or beautify it in some way, if necessary -- but essentially its just that. It could even come pre-compiled and pre-packaged, so no In that case, Bisq would have no more dependency on Cause sometimes a small team with limited resources has to prioritize. What is worth more investing time in:
In fact, 2. actively conflicts with 1. because striving for 2. forces Bisq to now use 3-4 year old frameworks which are EOL (Java 10), as well as modules that have known memory leaks and security issues that have been fixed since years (JavaFX 11, which is used now). Which sort of undermines users' security, making them vulnerable to attackers who could specifically try to exploit the known issues in these older frameworks. Best case, denial of service by finding ways to trigger high memory usage and crashing their Bisq app. Worst case, well there is always the jackpot of getting access to those juicy private keys. So I'd say there is a strong argument to be made for prioritizing 1. above 2. and perhaps sacrificing nice double-click installers for the benefits of state-of-the-art security, reliability, performance. Maybe I'm over-reacting and blowing this out of proportion, but I was just surprised to see how many of the current issues (UI freezing, system crashing, etc) would probably go away if Bisq just upgraded a few modules and components. Solutions are known and are out there, others have worked on and fixed and optimized things, but we're not using them. I realize its not a simple choice to make, and there are nuances -- but at the end of the day, how much better could Bisq's trading protocol be right now, if all that effort invested in trying to analyze and fix tens and tens of memory-related issues, etc -- if even part of that effort went into for example Bisq v2. Or gRPC. Anyway, working on this PR made me a big fan of dropping What do you guys think? Would you generally agree or disagree? (Tagging @freimair since this could be relevant for security) |
That's a good catch. Interested in actually looking at it to see if it's in good enough shape to use now? i.e. can it be a more or less drop-in replacement for our current use of I'd want to see what the answer to this question is before considering dropping installers altogether. Assuming jpackager is already in good enough shape, or soon will be, we shouldn't have any problem keeping up to date in the future. It's actually a great thing, in this way, that we package our own JRE. It means we can stay as current as we like without any concern for what runtime the user has installed. It was just super unfortunate (downright irresponsible, imo) that |
@cbeams: Good point. I'll look into that and experiment with the packager, will update this thread with the findings. |
Build systems isn't my forte, but I would really like to see us moving to JDK14 for the benefits described here. @cbeams has a good point, but if it would work almost as easily for users to build it themselves, then I would think that is a reasonable way to go. |
@cd2357 I can't build this on master with Oracle JDK14. Is OpenJDK required? |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I'm also pro moving to JDK 14 if jpackager supports what we need (it does as far as I remember reading through the draft docu in the past. @cd2357 I agree that ditching the native bundle route would save some time and would make the build process easier, but I fear that it would prevent us to move to a bigger audience the far off we go from the native app route. Did you have time to play around with the jpackager already? |
@ripcurlx yes, I fully agree. I didn't explore this further yet (therefore marked it as draft), but I realized I'm not familiar with the build process. Is there some documentation or some wiki explaining the steps (how to build and package for linux / osx / windows)? I might have to try them out (and evtl adjust them) if the |
Here you can find the release process readme bisq/desktop/package/macosx/create_app.sh Lines 78 to 94 in 88fcb3f
We do some customization using:
Please let me know if you need more info to be able to continue with this. |
@cd2357 I'm still unable to build this against current master & OpenJDK 14.0.1 with
and
|
Yes, there seem to be changes required in our custom components (access of variables/methods changed in the most up-to-data JavaFX version) |
@cd2357 I had to change the build to ignore test results, but I would very much like to solve this as this build you have going here seems to be the first that runs without crashing under WSL. In build, I'm getting this. I'm not versed enough yet to know which particular test is failing.
|
I can confirm the wallet crash issue is also present with WSL/Ubuntu 20.04 |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Still relevant |
25e5d1e
to
222e8d5
Compare
Update the gradle dependency to JavaFX 14. This brings to Bisq the latest JavaFX fixes and improvements, especially in the areas of UI performance, memory management and security. JavaFX can be upgraded independently of the JDK used to build the application, so this change is modular and does not affect other parts of the build process. Related / likely related to: bisq-network#350 bisq-network#2135 bisq-network#2509 bisq-network#3128 bisq-network#3307 bisq-network#3308 bisq-network#3343 bisq-network#3430 bisq-network#3657 bisq-network#3677 bisq-network#3683 bisq-network#3686 bisq-network#3786 bisq-network#3787 bisq-network#3892 bisq-network#3917 bisq-network#3918 bisq-network#3936
@cd2357, I'd recommend rebasing the |
Needs to be either only executed on macOS or adapted to work with Windows as well
used for mobile notification pairing because of missing current macOS support by currently used webcam library
@cd2357 It is not perfect, but IMO the PR is in a mergable state. Everyone is welcome to give binaries created by this PR a shot: https://www.dropbox.com/sh/bos203ga9bq0akp/AAB6j5DPAINCPbCP9vyRB3n-a?dl=0 The deterministic jar is not included anymore as we'll have different libs included by JavaFX based on the OS it is compiled in. We might be able to do something similar as we had in the past, but not in the first step. The jar is compiled with Java 11 and the binaries are bundling a current Java 15 runtime. So in the best (normal) case it should behave similar to the current binaries, but more reliable (on my Windows VM it "feels" faster). Please let me know if you run into any issues so we can merge this PR for the upcoming release. Thanks! Especially I would be interested in Windows updates/installations ( we had some workarounds in place for user accounts containing special characters and auth cookie issues with TOR) which we left out in this upgrade as we think it shouldn't be necessary anymore. If you have installed v1.6.2 already you might need to uninstall it before as the Wix prevents overwritting of the same application version. |
Tried this new version on macOS -- no requests for input monitoring, no crashes, so 👍🏻 . The first time I opened the app, the list of open trades was empty but the "Portfolio" tab had a red badge with the correct number of open trades (even though they were not listed in the main window). Restarted the app and all open trades were visible again. |
Tried on Windows 10 after uninstalling "official" 1.6.2 version. Everything's fine and smooth. |
Tested on Ubuntu Linux: look/feel exactly the same as before. 👍 |
One "feature" I've noticed (not sure if it was like that before) is that chat window moves in parallel to the main window. This might be a problem in a 2+ monitor setup: if you move chat window to a different display and then move the main window to an outmost display -- chat window may go outside the visible area... not a big deal but just an FYI. |
As far as I remember that was already the case on master. Not sure if it is just a setting to undock the window. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK - Tested a full release and did lots of developer testing while doing customizations on it myself.
Following workarounds were deprecated for now:
- handle special characters on Windows (might be solved by the new installer)
- remove the old Tor workaround to prevent Auth Cookie issues (might be solved by the new installer and upgraded Tor library)
- Deterministic jar inclusion in release (difficult to solve as we are bundling different (reduced) fat jars for each OS
- Scan QR code with webcam to pair with your mobile notification app was temporarily disabled as the webcam library is not supported anymore and notarization is not possible right now.
We'll do for the upcoming release a pre-release again to get feedback on the new installers and runtimes.
@cd2357 Could you please move the state of this PR from Draft to Ready for Review? Thanks! |
I ran It created I installed the deb pkg and started Bisq, running the bundled JDK: I noticed the installer created But Environment variable I clicked around the UI while it sync'd, and didn't see any NPEs. Great work!!! I know this had to be difficult. |
About the Gradle upgrade... Gradle 7.0 removes a lot deprecated API, and this PR's build file would need to migrate to all the new dependency configurations I got a Gradle 7 build working yesterday, and you might be interested in discussing some of the changes needed to upgrade to v7.0 after this PR is merged. |
Glad the jfoenix upgrade finally happened. Now the api test harness can start Bob & Alice desktops w/out those NPEs. This makes some release testing go much quicker. |
Yes all good, was just busy for a while then had to travel so didn't get to check Keybase :) Sorry for the late reply. Looks like the PR is already merged, let me know if there's anything open / anything else I can do to help finalize it. |
Update the gradle dependency to JavaFX 14.
This brings to Bisq the latest JavaFX fixes and improvements, especially
in the areas of UI performance and memory management.
Initial tests on Qubes already showed several improvements:
Tests on Mac also show better performance + better / more "native" UI integration.
To use the new JavaFX version, just
./gradlew clean build
Bisq before running it.Note: since JavaFX also uses VRAM (in addition to the normal system RAM
as used by the JDK), it could be that previous attempts to optimize the JDK's
memory usage (like #3677 and #4048) might have had no impact on JavaFX-
specific inefficiencies.
Related / likely related to: #350 #2135 #2509 #3128 #3307 #3308 #3343
#3430 #3657 #3683 #3686 #3786 #3787 #3892 #3917 #3918 #3936