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.

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

Already on GitHub? Sign in to your account

8265469: Allow to build media and webkit for Linux-AArch64 #467

Closed
wants to merge 2 commits into from

Conversation

johanvos
Copy link
Collaborator

@johanvos johanvos commented Apr 19, 2021

Changes that allow to build linux configuration on Linux AArch64
This PR introduces an IS_AARCH64 parameter in build.gradle.
This PR already contains the change from PR #465 so if this one gets integrated, that change needs to removed from this PR.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8265469: Allow to build media and webkit for Linux-AArch64

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/467/head:pull/467
$ git checkout pull/467

Update a local copy of the PR:
$ git checkout pull/467
$ git pull https://git.openjdk.java.net/jfx pull/467/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 467

View PR using the GUI difftool:
$ git pr show -t 467

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/467.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 19, 2021

馃憢 Welcome back jvos! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Apr 19, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 19, 2021

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 20, 2021

/reviewers 2

@openjdk
Copy link

openjdk bot commented Apr 20, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Does passing ARCH=aarch64 to the media Makefiles work on Mac and Windows platforms?

@@ -2861,7 +2862,7 @@ project(":media") {
args("JAVA_HOME=${JDK_HOME}", "GENERATED_HEADERS_DIR=${generatedHeadersDir}",
"OUTPUT_DIR=${nativeOutputDir}", "BUILD_TYPE=${buildType}", "BASE_NAME=jfxmedia",
"COMPILE_PARFAIT=${compileParfait}",
IS_64 ? "ARCH=x64" : "ARCH=x32",
IS_64 ? IS_AARCH64 ? "ARCH=aarch64" : "ARCH=x64" : "ARCH=x32",
Copy link
Member

@kevinrushforth kevinrushforth Apr 22, 2021

Choose a reason for hiding this comment

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

Does ARCH=aarch64 get handled correctly on Mac and Windows?

Copy link
Collaborator Author

@johanvos johanvos Apr 23, 2021

Choose a reason for hiding this comment

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

We don't have media/web on Windows-aarch64 yet.
On mac, it is handled differently: if a TARGET_ARCH is specified, the mac.gradle will add -target TARGET_ARCH-apple-macos-11 to the CC parameter which is passed from build.gradle to the media build scripts.

I realise we are not consequent here: we specify the ARCH to the makefile, but we also provide the CC and LINKER properties which can contain the architecture as well.

Copy link
Member

@kevinrushforth kevinrushforth Apr 23, 2021

Choose a reason for hiding this comment

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

OK, we can do any cleanup in the follow-on issue you plan to file to make our handling of aarch64 more consistent.

Copy link
Member

@arun-Joseph arun-Joseph left a comment

Other than that, the changes looks good.

build.gradle Outdated
@@ -3458,7 +3462,7 @@ project(":web") {
def targetProperties = project.rootProject.ext[t.upper]
def webkitProperties = targetProperties.webkit
def classifier = (t.name != "linux" && t.name != "win") ? t.name :
IS_64 ? "${t.name}-amd64" : "${t.name}-i586"
IS_64 ? IS_AARCH64 ? "${t.name}-aarch64" : "${t.name}-amd64" : "${t.name}-i586"
Copy link
Member

@arun-Joseph arun-Joseph Apr 23, 2021

Choose a reason for hiding this comment

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

I don't think this change is necessary as classifier was used for webview-deps, and it's not used anymore (removed in bf203ad). This classifier definition can also be removed.

Copy link
Collaborator Author

@johanvos johanvos Apr 23, 2021

Choose a reason for hiding this comment

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

that would be a good cleanup indeed.

@@ -2861,7 +2862,7 @@ project(":media") {
args("JAVA_HOME=${JDK_HOME}", "GENERATED_HEADERS_DIR=${generatedHeadersDir}",
"OUTPUT_DIR=${nativeOutputDir}", "BUILD_TYPE=${buildType}", "BASE_NAME=jfxmedia",
"COMPILE_PARFAIT=${compileParfait}",
IS_64 ? "ARCH=x64" : "ARCH=x32",
IS_64 ? IS_AARCH64 ? "ARCH=aarch64" : "ARCH=x64" : "ARCH=x32",
Copy link
Member

@kevinrushforth kevinrushforth Apr 23, 2021

Choose a reason for hiding this comment

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

OK, we can do any cleanup in the follow-on issue you plan to file to make our handling of aarch64 more consistent.

@openjdk
Copy link

openjdk bot commented Apr 23, 2021

@johanvos This change now passes all automated pre-integration checks.

鈩癸笍 This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8265469: Allow to build media and webkit for Linux-AArch64

Reviewed-by: kcr, ajoseph

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 10 new commits pushed to the master branch:

  • dfda00d: 8265514: Openjfx controls running tests broken (Eclipse)
  • 1b407cc: 8239880: CSS tests should cleanup any global state they modify
  • fab638a: 8265425: Hard failure when building OpenJFX for Linux AArch64
  • dffdc6f: 8265758: [TestBug] Remove ignored unit test from CustomMenuItemTest
  • 4a9c892: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64)
  • 86b854d: 8234077: Evaluate ignored unit tests in RenderRootTest
  • 8f0062d: 8234084: [TEST_BUG] Remove unexecuted performance benchmark tests
  • 49d2126: 8263788: JavaFX application freezes completely after some time when using the WebView
  • 67828ae: 8261840: Submenus close to screen borders are no longer repositioned
  • af75a1f: 8265439: [TestBug] Enable and fix ignored unit tests in MenuItemTest

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

鉃★笍 To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Apr 23, 2021
@johanvos
Copy link
Collaborator Author

johanvos commented Apr 23, 2021

/integrate

@openjdk openjdk bot closed this Apr 23, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Apr 23, 2021
@openjdk
Copy link

openjdk bot commented Apr 23, 2021

@johanvos Since your change was applied there have been 10 commits pushed to the master branch:

  • dfda00d: 8265514: Openjfx controls running tests broken (Eclipse)
  • 1b407cc: 8239880: CSS tests should cleanup any global state they modify
  • fab638a: 8265425: Hard failure when building OpenJFX for Linux AArch64
  • dffdc6f: 8265758: [TestBug] Remove ignored unit test from CustomMenuItemTest
  • 4a9c892: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64)
  • 86b854d: 8234077: Evaluate ignored unit tests in RenderRootTest
  • 8f0062d: 8234084: [TEST_BUG] Remove unexecuted performance benchmark tests
  • 49d2126: 8263788: JavaFX application freezes completely after some time when using the WebView
  • 67828ae: 8261840: Submenus close to screen borders are no longer repositioned
  • af75a1f: 8265439: [TestBug] Enable and fix ignored unit tests in MenuItemTest

Your commit was automatically rebased without conflicts.

Pushed as commit 27e57d3.

馃挕 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants