<p><b>@cbeams</b> requested changes on this pull request.</p>
<p>NACK, for minor reasons detailed in the review comments, but mostly for lack of <code>javapackager</code> support in JDK 11, as has been mentioned by others above. I did some research here, and there's good news—an implementation of the new <code>jpackager</code> tool specified in <a href="https://openjdk.java.net/jeps/343" rel="nofollow">JEP 343</a> is under review now.</p>
<p>Here's the announcement on the OpenJDK core-libs-dev mailing list: <a href="http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/056186.html" rel="nofollow">http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/056186.html</a>. A review thread ensues, but it's pretty low-level, probably not worth reading in detail for the stuff we care about.</p>
<p>There's a higher-level discussion about the new implementation in r/Java at <a href="https://www.reddit.com/r/java/comments/9qz61j/new_java_packager_tool_under_review_jep_343/" rel="nofollow">https://www.reddit.com/r/java/comments/9qz61j/new_java_packager_tool_under_review_jep_343/</a>.</p>
<p>I haven't done an option-by-option gap analysis to see if the new implementation will have everything we need, but from what I've gathered so far, it will. I think we're using <code>javapackager</code> in a pretty simple way right now, and while the new <code>jpackager</code> "drops support for JavaFX-specific features", I don't think we were using any of those anyway. See <a href="https://openjdk.java.net/jeps/343#Non-Goals" rel="nofollow">https://openjdk.java.net/jeps/343#Non-Goals</a> for a complete list of what's getting dropped from the new implementation.</p>
<p>Excerpted from the results of <code>git grep -A20 javapackager</code> run at the root of the repo, here's our invocation of <code>javapackager</code> for macOS:</p>
<pre><code>$JAVA_HOME/bin/javapackager \
-deploy \
-BappVersion=$version \
-Bmac.CFBundleIdentifier=io.bisq.CAT \
-Bmac.CFBundleName=Bisq \
-Bicon=package/macosx/Bisq.icns \
-Bruntime="$JAVA_HOME/jre" \
-native dmg \
-name Bisq \
-title Bisq \
-vendor Bisq \
-outdir deploy \
-srcdir deploy \
-srcfiles "Bisq-$version.jar" \
-appclass bisq.desktop.app.BisqAppMain \
-outfile Bisq
</code></pre>
<p>I don't see any "JavaFX-specific" stuff there, though it might be worth going through each option (in particular the various <code>-B</code> flags) to ensure they have some equivalent in the new implementation.</p>
<p>As far as staying up-to-date on when <code>jpackager</code> will become available, there is a JIRA issue at <a href="https://bugs.openjdk.java.net/browse/JDK-8200758" rel="nofollow">https://bugs.openjdk.java.net/browse/JDK-8200758</a> that one could theoretically subscribe to in order to get updates, but there seems to be no way to create an account for that JIRA instance. I've emailed the OpenJDK team to ask about that, and will subscribe if I get an answer.</p>
<p>In any case, we need to wait on upgrading to JDK 11 for this, so I'd say we just leave this as a WIP for the time being. <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=603793" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/devinbileck">@devinbileck</a> (or anyone else feeling up to it), you could make sure the new implementation has everything we need in the meantime, and weigh in on the core-dev-libs mailing list if it doesn't. They'd probably appreciate the real-world review.</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1810#discussion_r230393049">.travis.yml</a>:</p>
<pre style='color:#555'>> @@ -1,5 +1,7 @@
language: java
-jdk: openjdk10
+jdk:
+ - oraclejdk11
+ - openjdk11
</pre>
<p>I wouldn't build with both. It'll make the CI builds take 2x as long, and it's probably better in the end to keep things simple and tell everybody they just need to install OpenJDK and forget about the Oracle JDK going forward.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1810#discussion_r230393527">README.md</a>:</p>
<pre style='color:#555'>> @@ -12,7 +12,7 @@ For more information, see https://bisq.network/intro and for step-by-step gettin
## Building Bisq
-You will need OpenJDK [JDK 10](https://jdk.java.net/10/) installed to complete the following instructions.
+You will need either [Oracle JDK 11](https://www.oracle.com/technetwork/java/javase/downloads/jdk11-downloads-5066655.html) or [OpenJDK JDK 11](https://jdk.java.net/11/) installed to complete the following instructions.
</pre>
<p>Same as above; let's just keep it to OpenJDK 11. Note that there's a typo (not introduced by this PR) here: <code>Open JDK JDK 11</code> should just be <code>OpenJDK 11</code>.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1810#discussion_r230395074">gradle/wrapper/gradle-wrapper.properties</a>:</p>
<pre style='color:#555'>> @@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-4.9-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.10.2-bin.zip
</pre>
<p>This should (now) be <code>5.0-rc-1</code>, which is apparently the first release to officially support JDK 11 (even though everything builds just fine for me under <code>4.9</code>).</p>
<p>See <a href="https://docs.gradle.org/5.0-rc-1/release-notes.html#java-11-runtime-support" rel="nofollow">https://docs.gradle.org/5.0-rc-1/release-notes.html#java-11-runtime-support</a></p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/1810#pullrequestreview-171125069">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AkpZtlX4hcrQrEc8_g55bUzVBVKFRCZOks5urFxzgaJpZM4X3N9F">mute the thread</a>.<img src="https://github.com/notifications/beacon/AkpZtoYb6tHdHcfZBILqh8_UzrK50MFaks5urFxzgaJpZM4X3N9F.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/bisq-network/bisq","title":"bisq-network/bisq","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/bisq-network/bisq"}},"updates":{"snippets":[{"icon":"PERSON","message":"@cbeams requested changes on #1810"}],"action":{"name":"View Pull Request","url":"https://github.com/bisq-network/bisq/pull/1810#pullrequestreview-171125069"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/1810#pullrequestreview-171125069",
"url": "https://github.com/bisq-network/bisq/pull/1810#pullrequestreview-171125069",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
},
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB",
"title": "@cbeams requested changes on 1810",
"sections": [
{
"text": "NACK, for minor reasons detailed in the review comments, but mostly for lack of `javapackager` support in JDK 11, as has been mentioned by others above. I did some research here, and there's good news—an implementation of the new `jpackager` tool specified in [JEP 343](https://openjdk.java.net/jeps/343) is under review now.\r\n\r\nHere's the announcement on the OpenJDK core-libs-dev mailing list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/056186.html. A review thread ensues, but it's pretty low-level, probably not worth reading in detail for the stuff we care about.\r\n\r\nThere's a higher-level discussion about the new implementation in r/Java at https://www.reddit.com/r/java/comments/9qz61j/new_java_packager_tool_under_review_jep_343/.\r\n\r\nI haven't done an option-by-option gap analysis to see if the new implementation will have everything we need, but from what I've gathered so far, it will. I think we're using `javapackager` in a pretty simple way right now, and while the new `jpackager` \"drops support for JavaFX-specific features\", I don't think we were using any of those anyway. See https://openjdk.java.net/jeps/343#Non-Goals for a complete list of what's getting dropped from the new implementation.\r\n\r\nExcerpted from the results of `git grep -A20 javapackager` run at the root of the repo, here's our invocation of `javapackager` for macOS:\r\n\r\n```\r\n$JAVA_HOME/bin/javapackager \\\r\n -deploy \\\r\n -BappVersion=$version \\\r\n -Bmac.CFBundleIdentifier=io.bisq.CAT \\\r\n -Bmac.CFBundleName=Bisq \\\r\n -Bicon=package/macosx/Bisq.icns \\\r\n -Bruntime=\"$JAVA_HOME/jre\" \\\r\n -native dmg \\\r\n -name Bisq \\\r\n -title Bisq \\\r\n -vendor Bisq \\\r\n -outdir deploy \\\r\n -srcdir deploy \\\r\n -srcfiles \"Bisq-$version.jar\" \\\r\n -appclass bisq.desktop.app.BisqAppMain \\\r\n -outfile Bisq\r\n```\r\n\r\nI don't see any \"JavaFX-specific\" stuff there, though it might be worth going through each option (in particular the various `-B` flags) to ensure they have some equivalent in the new implementation.\r\n\r\nAs far as staying up-to-date on when `jpackager` will become available, there is a JIRA issue at https://bugs.openjdk.java.net/browse/JDK-8200758 that one could theoretically subscribe to in order to get updates, but there seems to be no way to create an account for that JIRA instance. I've emailed the OpenJDK team to ask about that, and will subscribe if I get an answer.\r\n\r\nIn any case, we need to wait on upgrading to JDK 11 for this, so I'd say we just leave this as a WIP for the time being. @devinbileck (or anyone else feeling up to it), you could make sure the new implementation has everything we need in the meantime, and weigh in on the core-dev-libs mailing list if it doesn't. They'd probably appreciate the real-world review.",
"activityTitle": "**Chris Beams**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@cbeams",
"facts": [
]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/bisq-network/bisq/pull/1810#pullrequestreview-171125069"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 400351045\n}"
}
],
"themeColor": "26292E"
}
]</script>