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

Java 10 support #1660

Merged
merged 21 commits into from Sep 18, 2018

Conversation

Projects
None yet
3 participants
@ripcurlx
Member

ripcurlx commented Sep 7, 2018

I've started to adapt the current mono repository to support Java 10.
The current state of this branch is, that gradle build runs and you can launch the app from IDEA.
Still there are a couple of things left, where help is wanted (see check list)

  • Upgrade JavaFX code
  • Implement alternative for progress indicator (block chain confirmations) as current implementation won't work for java 10
  • Update lombok
  • Update source compatibility settings
  • Update travis setting
  • Implement power mock support
  • Implement JMockit support
  • Merge with master (and update everything new to Java 10)
  • Solve issue with library that includes Java 9 module configuration

Release Build

ripcurlx added some commits Aug 21, 2018

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 7, 2018

@initCCG: With Java 10, Thai now renders fine! 😄
bildschirmfoto 2018-09-07 um 14 29 08

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 11, 2018

@cbeam I don't know why the release build worked for me when I tested the mono repo branch, but now while testing my Java 10 build I saw that running gradle build within the desktop directory fails with following error:

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring root project 'bisq-desktop'.
> Could not resolve all dependencies for configuration ':compile'.
   > Could not resolve project :p2p.
     Required by:
         project :
      > Project : declares a dependency from configuration 'compile' to configuration 'default' which is not declared in the descriptor for project :p2p.
   > Could not resolve project :core.
     Required by:
         project :
      > Project : declares a dependency from configuration 'compile' to configuration 'default' which is not declared in the descriptor for project :core.
   > Could not resolve project :common.
     Required by:
         project :
      > Project : declares a dependency from configuration 'compile' to configuration 'default' which is not declared in the descriptor for project :common.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 5.0.
See https://docs.gradle.org/4.6/userguide/command_line_interface.html#sec:command_line_warnings

I guess it is something obvious if you know how to gradle 😉 - Do you know what I have to configure differently in the build.gradle file of desktop?

@cbeams

This comment has been minimized.

Member

cbeams commented Sep 11, 2018

@ripcurlx, it's not something I've seen before, and I don't have time now to install JDK 10 and investigate personally. I found this: https://stackoverflow.com/questions/42929841/app-declares-a-dependency-from-configuration-compile-to-configuration-default#44457269, but it doesn't provide much insight to me on first read.

I would just start winding back commits (you only have half a dozen or so on that branch) and see where the trouble crept in. Let me know and when I have time later (i.e. tomorrow), I can take a look. Would like to get JDK 10 installed anyway.

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 11, 2018

@cbeams The problem is that I see now the same problem in master with Java 8 as well. I don't know why it worked for me before. Still gradle build works in the root, but not within the desktop sub directory.

@cbeams

This comment has been minimized.

Member

cbeams commented Sep 11, 2018

Confirmed. Same here. This is one of those "improvements we can make after the merge" that I mentioned. I'll patch it up soon. In the meantime, you can run gradle :desktop:build from the root to isolate bisq-desktop. Work for you in the meantime?

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 11, 2018

@cbeams Seems to work - thanks! Added a temporary workaround in create_app.sh for now.

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 12, 2018

@cbeams @ManfredKarrer The good thing with Java 10 is that it supports strong cyphers by default, so there is one workaround less. The bad thing is, that they dropped the extension mechanism, we used for BouncyCastle (https://docs.oracle.com/javase/10/migrate/toc.htm#JSMIG-GUID-2C896CA8-927C-4381-A737-B1D81D964B7B). Unfortunately I haven't figured it out how to include the BC jar otherwise in our release build. Any ideas are highly appreciated :-)

@cbeams

This comment has been minimized.

Member

cbeams commented Sep 12, 2018

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 12, 2018

@ripcurlx I sent an email to @freimair. Maybe we should just drop BC. Too many problems all the time with it...

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 13, 2018

@ManfredKarrer Thanks! If the BC issue is solved this branch is ready for merge.

ripcurlx added some commits Sep 14, 2018

@ripcurlx ripcurlx changed the title from [WIP] Java 10 support to Java 10 support Sep 18, 2018

@cbeams

cbeams approved these changes Sep 18, 2018

utACK. I'd like to properly test it locally against JDK 10 locally, but I still don't have it installed and want to get back about this quickly.

Nit: it would be good to extract the often-duplicated value assigned to @PowerMockIgnore annotations into a static String[] somewhere with a comment as to why it's necessary.

I wouldn't merge this without at least a couple proper ACKs, ideally across OSes. Again, just wanted to report I've looked through this and don't see anything obviously wrong.

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

I will test it soon.
@blabno @mrosseel Could you give it a test as well an ACk if ok for you?

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

@ripcurlx
I get that warning:
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.inject.internal.cglib.core.$ReflectUtils$1 (file:/Users/dev/.gradle/caches/modules-2/files-2.1/com.google.inject/guice/4.1.0/eeb69005da379a10071aa4948c48d89250febb07/guice-4.1.0.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)
WARNING: Please consider reporting this to the maintainers of com.google.inject.internal.cglib.core.$ReflectUtils$1
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

Maybe we need to update Guice?

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

Just tried Guice v 4.2 but same warning.

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

Seems it is a groovy issue (used in Gradle):
gradle/gradle#2995 (comment)

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

@ripcurlx
There was a bug with SetupUtils (preventing seed nodes to start up).
UserThread.execute(resultHandler::handleResult); was removed in an earlier commit.

Should be:

Thread checkCryptoThread = new Thread() {
            @Override
            public void run() {
                try {
                    Thread.currentThread().setName("checkCryptoThread");
                    log.trace("Run crypto test");
                    // just use any simple dummy msg
                    Ping payload = new Ping(1, 1);
                    SealedAndSigned sealedAndSigned = EncryptionService.encryptHybridWithSignature(payload,
                            keyRing.getSignatureKeyPair(), keyRing.getPubKeyRing().getEncryptionPubKey());
                    DecryptedDataTuple tuple = encryptionService.decryptHybridWithSignature(sealedAndSigned,
                            keyRing.getEncryptionKeyPair().getPrivate());
                    if (tuple.getNetworkEnvelope() instanceof Ping &&
                            ((Ping) tuple.getNetworkEnvelope()).getNonce() == payload.getNonce() &&
                            ((Ping) tuple.getNetworkEnvelope()).getLastRoundTripTime() == payload.getLastRoundTripTime()) {
                        log.debug("Crypto test succeeded");

                        UserThread.execute(resultHandler::handleResult);
                    } else {
                        errorHandler.accept(new CryptoException("Payload not correct after decryption"));
                    }
                } catch (CryptoException | ProtobufferException e) {
                    log.error(e.toString());
                    e.printStackTrace();
                    errorHandler.accept(e);
                }
            }
        };
        checkCryptoThread.start();
    }

Beside that all works for me. So as soon that is fixed I can give a ACK.

Here is a patch:

===================================================================
--- core/src/main/java/bisq/core/app/SetupUtils.java	(date 1537286276000)
+++ core/src/main/java/bisq/core/app/SetupUtils.java	(date 1537289718000)
@@ -65,6 +65,8 @@
                             ((Ping) tuple.getNetworkEnvelope()).getNonce() == payload.getNonce() &&
                             ((Ping) tuple.getNetworkEnvelope()).getLastRoundTripTime() == payload.getLastRoundTripTime()) {
                         log.debug("Crypto test succeeded");
+
+                        UserThread.execute(resultHandler::handleResult);
                     } else {
                         errorHandler.accept(new CryptoException("Payload not correct after decryption"));
                     }
@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

@ripcurlx
The removal of the StaticProgressIndicatorBehavior.java and StaticProgressIndicatorSkin.java at #1660 re-introduce the performance problems of the ProgressIndicator.
CPU usage with spinning ProgressIndicator is about 15%, without it its about 10% (run in IDE debug mode, standalone would be lower in total but difference would be the same).
Maybe we remove the ProgressIndicator completely or we re-apply the fixes. We can still merge the PR from that but I think we should fix that before the next release.

public void updateWithCurrencies(List<TradeCurrency> currencies, @Nullable CurrencyListItem first) {
List<CurrencyListItem> result = Lists.newLinkedList();
Optional.ofNullable(first).ifPresent(result::add);
result.addAll(getPartitionedSortedItems(currencies));
setAll(result);
delegate.addAll(result);

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Sep 18, 2018

Member

To have the same behaviour as before (setAll) we should add a clear() before the addAll.

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

Here another nit patch:

Index: core/src/test/java/bisq/core/crypto/SigTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- core/src/test/java/bisq/core/crypto/SigTest.java	(date 1537286276000)
+++ core/src/test/java/bisq/core/crypto/SigTest.java	(date 1537291722000)
@@ -23,13 +23,6 @@
 import bisq.common.crypto.Sig;
 import bisq.common.storage.FileUtil;
 
-import org.bouncycastle.jce.provider.BouncyCastleProvider;
-
-import java.security.KeyStoreException;
-import java.security.NoSuchAlgorithmException;
-import java.security.Security;
-import java.security.cert.CertificateException;
-
 import java.io.File;
 import java.io.IOException;
 
@@ -50,7 +43,7 @@
     private File dir;
 
     @Before
-    public void setup() throws CertificateException, NoSuchAlgorithmException, KeyStoreException, IOException, CryptoException {
+    public void setup() throws IOException {
 
         dir = File.createTempFile("temp_tests", "");
         //noinspection ResultOfMethodCallIgnored

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

I did some smoke tests between old version and java 10 version and all worked.

ripcurlx added some commits Sep 18, 2018

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 18, 2018

@ManfredKarrer I applied your suggestions, but I have a question regarding the Status Indicator. Actually besides extending a different base class the only thing I need to change was in the constructor to replace

if (spinner != null) {
                if (!(getSkinnable().impl_isTreeVisible() && getSkinnable().getScene() != null)) {
                    getChildren().remove(spinner);
                    spinner = null;
                    timeLineNulled = true;
                }
            }

with

if (spinner != null) {
                if (getSkinnable().getScene() != null) {
                    getChildren().remove(spinner);
                    spinner = null;
                    timeLineNulled = true;
                }
            }

as getSkinnable().impl_isTreeVisible() isn't accessible anymore. Do you remember if that was the root cause of the performance issue? So I know where to look for a workaround for this missing check.

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 18, 2018

@cbeams As I'm only able to replace primitives in annotations the best I could do would be something like:

package bisq.core.util;

public class PowerMockUtil {
    // Workaround for PowerMock with Java 9+. For more details see https://github.com/powermock/powermock/issues/864
    public static final String ignoreXerces = "com.sun.org.apache.xerces.*";
    public static final String ignoreJavaxXML = "javax.xml.*";
    public static final String ignoreOrgXML = "org.xml.*";
}

and use it like that:
@PowerMockIgnore({PowerMockUtil.ignoreXerces, PowerMockUtil.ignoreJavaxXML, PowerMockUtil.ignoreOrgXML})
Not sure if that improves it a lot.

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

@ripcurlx I added a new issue here #1692

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

@ripcurlx From my side that PR can be merged. Not sure if you want to wait for more reviewers. I just got build problems with the current master, so maybe its better to get that merged soon, to get those setup changes completed?

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 18, 2018

@ManfredKarrer I only tried it on macOS High Sierra (10.13+) with Java 10.0.1 and macOS El Capitan (10.11+) with Java 10.0.2. With what configuration did you run the test?

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

I tested with 10.12.6 (Sierra) and java 10.0.2.

@ripcurlx

This comment has been minimized.

Member

ripcurlx commented Sep 18, 2018

Also tested now on Ubuntu 16.0.4 with Java 10.0.2

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

ACK to the latest changes db17991

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 18, 2018

As discussed with @ripcurlx I will merge now. He tested also successfully on the VMs.

@ManfredKarrer ManfredKarrer merged commit a6fe389 into bisq-network:master Sep 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ripcurlx ripcurlx referenced this pull request Nov 30, 2018

Closed

For November 2018 #177

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment