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

Added Support for M1 Mac #113

Merged
merged 3 commits into from
Dec 10, 2021
Merged

Added Support for M1 Mac #113

merged 3 commits into from
Dec 10, 2021

Conversation

henryhchchc
Copy link
Contributor

This fixes issue #112 by considering architecture as well when detecting the platform.

@cbeams
Copy link

cbeams commented Nov 16, 2021

Thanks @henryhchchc, this is a much-needed fix!

Maintainers, any indication when this might become available in a release?

cbeams added a commit to cbeams/bisq2-proto that referenced this pull request Nov 16, 2021
@cbeams
Pick up javafx-gradle-plugin fix for M1 architecture
c743079 
Problem: until the recent release of JavaFX 17, it was impossible to run
a JavaFX app on an M1 Mac without locally installing an early-access
build of JavaFX SDK. Even after the release of JavaFX 17, however, it
was still not possible to run a JavaFX application on an M1 Mac because
of the bug reported at openjfx/javafx-gradle-plugin#112 (which caused
led to the dreaded "No toolkit found" error).

Solution: the PR at openjfx/javafx-gradle-plugin#113 fixes this error in
the javafx-gradle-plugin, but has not yet been released. This commit
assumes that the PR branch as been built and installed on the local
machine, and expects to resolve it from the local maven cache. Happily,
it works as expected, making it possible to run ./bisqfx without the SDK
installed locally.
cbeams added a commit to cbeams/bisq that referenced this pull request Nov 16, 2021
@cbeams
Support building and running on Apple M1 Silicon
98dc483 
Problem: Prior to this commit, it was not possible to build or run the
desktop app on Apple's M1 (macos aarch64) architecture.

Solution: This commit makes all the changes necessary to successfully
build and run on M1, but some of these changes require depending on
snapshots and forks of certain dependencies, so this commit shouldn't be
merged to master until those fixes are officially released. The changes
made here include:

1. Upgrading from JavaFX 16 to JavaFX 17. JavaFX 17 was the first
   release to support aarch64 binaries, and so it's required.

2. Upgrading to a version of the openjfx-gradle-plugin that contains the
   fix at openjfx/javafx-gradle-plugin#113 to download the correct
   aarch64-classified JavaFX 17 jars. NOTE: at the time of this commit,
   the fix has not been released, so we are now depending on a
   locally-built SNAPSHOT resolved from `mavenLocal()`.

3. Upgrading to a version of the JFoenix library that includes a fix for
   the bug documented at sshahine/JFoenix#1187. At the time of this
   commit, this fix has only been publihed in the 'rationalityfrontline'
   fork of JFoenix, and so we've temporarily updated our dependency to
   point to that artifact. It appears that a new version of JFoenix is
   being developed that will contain this fix, but it hasn't shipped
   yet.

Notes:

 - Gradle dependency verification metadata has been updated to reflect
   the changes mentioned above, but will need further updates to capture
   the Linux, Windows and MacOS Intel variants of the JavaFX 17
   binaries.

 - Building and testing has been verified to work on JDKs 11 through 17;

 - Running the desktop app only works on JDKs 11 through 15, however,
   until sshahine/JFoenix#1205 is fixed. Right now, attempting to run on
   JDK 16+ will result in a 'Cannot invoke
   "javafx.scene.Node.getLayoutBounds()" because "this.textNode" is null'
   error. See the above mentioned issue for details.

 - It was necessary to bump the minimum supported source compatibility
   in the Gradle build from Java 10 to Java 11 in order to be able to
   consume the upgraded JFoenix artifact.
@cbeams cbeams mentioned this pull request Nov 16, 2021
Closed
3 tasks
cbeams added a commit to cbeams/bisq that referenced this pull request Nov 18, 2021
@cbeams
Support building and running on Apple M1 Silicon
8546f59 
Problem: Prior to this commit, it was not possible to build or run the
desktop app on Apple's M1 (macos aarch64) architecture.

Solution: This commit makes all the changes necessary to successfully
build and run on M1, but some of these changes require depending on
snapshots and forks of certain dependencies, so this commit shouldn't be
merged to master until those fixes are officially released. The changes
made here include:

1. Upgrading from JavaFX 16 to JavaFX 17. JavaFX 17 was the first
   release to support aarch64 binaries, and so it's required.

2. Upgrading to a version of the openjfx-gradle-plugin that contains the
   fix at openjfx/javafx-gradle-plugin#113 to download the correct
   aarch64-classified JavaFX 17 jars. NOTE: at the time of this commit,
   the fix has not been released, so we are now depending on a
   locally-built SNAPSHOT resolved from `mavenLocal()`.

3. Upgrading to a version of the JFoenix library that includes a fix for
   the bug documented at sshahine/JFoenix#1187. At the time of this
   commit, this fix has only been publihed in the 'rationalityfrontline'
   fork of JFoenix, and so we've temporarily updated our dependency to
   point to that artifact. It appears that a new version of JFoenix is
   being developed that will contain this fix, but it hasn't shipped
   yet.

Notes:

 - Gradle dependency verification metadata has been updated to reflect
   the changes mentioned above, but will need further updates to capture
   the Linux, Windows and MacOS Intel variants of the JavaFX 17
   binaries.

 - Building and testing has been verified to work on JDKs 11 through 17;

 - Running the desktop app only works on JDKs 11 through 15, however,
   until sshahine/JFoenix#1205 is fixed. Right now, attempting to run on
   JDK 16+ will result in a 'Cannot invoke
   "javafx.scene.Node.getLayoutBounds()" because "this.textNode" is null'
   error. See the above mentioned issue for details.

 - It was necessary to bump the minimum supported source compatibility
   in the Gradle build from Java 10 to Java 11 in order to be able to
   consume the upgraded JFoenix artifact.
@norbert-gaulia
Copy link

Thanks @henryhchchc, this is a much-needed fix!

Maintainers, any indication when this might become available in a release?

You would be much faster if you just take the code of this repo include this PR and build jar file, then simply add to your build.gradle.

buildscript {
  repositories {
    mavenCentral()
  }
  dependencies {
      classpath "org.javamodularity:moduleplugin:1.8.10"
    classpath 'com.google.gradle:osdetector-gradle-plugin:1.7.0'
    classpath files('libs/javafx-plugin-0.0.11-SNAPSHOT.jar')
  }
}
apply plugin: 'org.openjfx.javafxplugin'

@cbeams
Copy link

cbeams commented Dec 7, 2021

the code of this repo include this PR and build jar file, then simply add to your build.gradle

The simpler thing to do is just depend on the latest snapshot from this repository, as can be seen at cbeams/bisq@8546f59#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R6-R12.

Naturally, however, we'd like to depend on an official release of the javafx-gradle-plugin from a non-snapshot repository.

@jperedadnr, any word on an upcoming release?

@jperedadnr jperedadnr requested review from johanvos, tiainen and jperedadnr and removed request for johanvos December 7, 2021 19:23
@jperedadnr
Copy link
Collaborator

@henryhchchc As per the Contributions section, you will need to sign the CLA before we can integrate this PR once reviewed.

@cbeams Using a snapshot won't help, unless you fork, commit the proposed change, build and publish locally.

@cbeams
Copy link

cbeams commented Dec 8, 2021

@cbeams Using a snapshot won't help, unless you fork, commit the proposed change, build and publish locally.

Right. That is in fact what I'm doing, as per the comment in the commit I linked to, but I forgot and wrote what I did above as if we were depending on a nightly build. Looking forward to seeing this in a release in any case, thanks.

@henryhchchc
Copy link
Contributor Author

@henryhchchc As per the Contributions section, you will need to sign the CLA before we can integrate this PR once reviewed.

@cbeams Using a snapshot won't help, unless you fork, commit the proposed change, build and publish locally.

I see. I signed the CLA just now.

jperedadnr
jperedadnr reviewed Dec 10, 2021
View reviewed changes
Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

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

Looks good, but I've left some comments

src/main/java/org/openjfx/gradle/JavaFXPlatform.java Show resolved Hide resolved
src/main/java/org/openjfx/gradle/JavaFXPlatform.java Outdated Show resolved Hide resolved
src/main/java/org/openjfx/gradle/JavaFXPlatform.java Outdated Show resolved Hide resolved
src/main/java/org/openjfx/gradle/JavaFXPlatform.java Outdated Show resolved Hide resolved
@henryhchchc
Copy link
Contributor Author

Thanks for the suggestions. I made the updates accordingly.

@jperedadnr jperedadnr requested a review from tiainen December 10, 2021 09:55
@jperedadnr jperedadnr merged commit 61329d2 into openjfx:master Dec 10, 2021
@jperedadnr
Copy link
Collaborator

This PR has been integrated now, and there is a new snapshot available (0.0.11-SNAPSHOT) at https://oss.sonatype.org/content/repositories/snapshots

Before doing a release, @henryhchchc, @cbeams, @norbertas-gaulia and others, could you please test it?

cbeams added a commit to cbeams/bisq that referenced this pull request Dec 20, 2021
@cbeams
Upgrade to newly published javafx-gradle-plugin snapshot
abf86ac 
This removes our dependency on a locally-built javafx-gradle-plugin 0.11
snapshot for the newly-published official snapshot at documented at
openjfx/javafx-gradle-plugin#113 (comment).
@cbeams
Copy link

cbeams commented Dec 20, 2021

Before doing a release, @henryhchchc, @cbeams, @norbertas-gaulia and others, could you please test it?

I've upgraded to the snapshot published above (see cbeams/bisq@abf86ac), and all works as expected. Thanks, @jperedadnr.

@dbast
Copy link

dbast commented Jan 4, 2022 •
edited
Loading

@jperedadnr What about doing a new release? Thanks!

@abhinayagarwal abhinayagarwal mentioned this pull request Jan 14, 2022
Closed
@cbeams
Copy link

cbeams commented Apr 21, 2022

@jperedadnr What about doing a new release? Thanks!

It looks like a release containing this fix was shipped with v0.0.11 on Jan 17th, and 0.0.12 has been released since then. I have not tested either yet, but did test the patch itself as per #113 (comment).

@vewert
Copy link

vewert commented Apr 21, 2022

I am using 0.0.12 and it works for M1

@woodser woodser mentioned this pull request May 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiainen tiainen tiainen approved these changes

@jperedadnr jperedadnr jperedadnr approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@henryhchchc @cbeams @norbert-gaulia @jperedadnr @dbast @vewert @tiainen