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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8307316: Let JavaFX be built on unknown architectures #1124

Closed
wants to merge 6 commits into from

Conversation

jgneff
Copy link
Member

@jgneff jgneff commented May 3, 2023

Please review these changes to the Gradle build files and the dependency verification file. The initial version of this pull request extends the permitted build platforms for Linux to the Java architectures arm, ppc64le, and s390x and adds an entry to the dependency verification file for the i386 architecture. The Debian names for these architectures are armhf, i386, ppc64el, and s390x.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8307316: Let JavaFX be built on unknown architectures (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1124/head:pull/1124
$ git checkout pull/1124

Update a local copy of the PR:
$ git checkout pull/1124
$ git pull https://git.openjdk.org/jfx.git pull/1124/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1124

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1124.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 3, 2023

👋 Welcome back jgneff! 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.

jgneff added a commit to jgneff/openjfx that referenced this pull request May 15, 2023
Update the patch to match the current fix for JDK-8307316:

  8307316: Let JavaFX be built on unknown architectures
  openjdk/jfx#1124
@jgneff
Copy link
Member Author

jgneff commented May 16, 2023

I changed the patch to print a warning message instead of failing when the build machine's architecture is unrecognized, based on the discussion on the mailing list.

I then built JavaFX on six Linux architectures. Three of the resulting build log files contain the warning message as expected (arm, ppc64le, and s390x):

$ grep -T -A5 'Configure project' *.txt
openjfx_amd64.txt:	> Configure project :
openjfx_amd64.txt-	gradle.gradleVersion: 7.6
openjfx_amd64.txt-	OS_NAME: linux
openjfx_amd64.txt-	OS_ARCH: amd64
openjfx_amd64.txt-	JAVA_HOME: /usr/lib/jvm/java-17-openjdk-amd64
openjfx_amd64.txt-	JDK_HOME: /usr/lib/jvm/java-17-openjdk-amd64
--
openjfx_arm64.txt:	> Configure project :
openjfx_arm64.txt-	gradle.gradleVersion: 7.6
openjfx_arm64.txt-	OS_NAME: linux
openjfx_arm64.txt-	OS_ARCH: aarch64
openjfx_arm64.txt-	JAVA_HOME: /usr/lib/jvm/java-17-openjdk-arm64
openjfx_arm64.txt-	JDK_HOME: /usr/lib/jvm/java-17-openjdk-arm64
--
openjfx_armhf.txt:	> Configure project :
openjfx_armhf.txt-	Unknown and unsupported build architecture: arm
openjfx_armhf.txt-	gradle.gradleVersion: 7.6
openjfx_armhf.txt-	OS_NAME: linux
openjfx_armhf.txt-	OS_ARCH: arm
openjfx_armhf.txt-	JAVA_HOME: /usr/lib/jvm/java-17-openjdk-armhf
--
openjfx_i386.txt:	> Configure project :
openjfx_i386.txt-	gradle.gradleVersion: 7.6
openjfx_i386.txt-	OS_NAME: linux
openjfx_i386.txt-	OS_ARCH: i386
openjfx_i386.txt-	JAVA_HOME: /usr/lib/jvm/java-17-openjdk-i386
openjfx_i386.txt-	JDK_HOME: /usr/lib/jvm/java-17-openjdk-i386
--
openjfx_ppc64el.txt:	> Configure project :
openjfx_ppc64el.txt-	Unknown and unsupported build architecture: ppc64le
openjfx_ppc64el.txt-	gradle.gradleVersion: 7.6
openjfx_ppc64el.txt-	OS_NAME: linux
openjfx_ppc64el.txt-	OS_ARCH: ppc64le
openjfx_ppc64el.txt-	JAVA_HOME: /usr/lib/jvm/java-17-openjdk-ppc64el
--
openjfx_s390x.txt:	> Configure project :
openjfx_s390x.txt-	Unknown and unsupported build architecture: s390x
openjfx_s390x.txt-	gradle.gradleVersion: 7.6
openjfx_s390x.txt-	OS_NAME: linux
openjfx_s390x.txt-	OS_ARCH: s390x
openjfx_s390x.txt-	JAVA_HOME: /usr/lib/jvm/java-17-openjdk-s390x

All of the builds were successful:

$ grep -T -A1 ^BUILD *.txt
openjfx_amd64.txt:	BUILD SUCCESSFUL in 2m 40s
openjfx_amd64.txt-	137 actionable tasks: 137 executed
--
openjfx_arm64.txt:	BUILD SUCCESSFUL in 3m 42s
openjfx_arm64.txt-	137 actionable tasks: 137 executed
--
openjfx_armhf.txt:	BUILD SUCCESSFUL in 16m 51s
openjfx_armhf.txt-	137 actionable tasks: 137 executed
--
openjfx_i386.txt:	BUILD SUCCESSFUL in 2m 42s
openjfx_i386.txt-	137 actionable tasks: 137 executed
--
openjfx_ppc64el.txt:	BUILD SUCCESSFUL in 3m 29s
openjfx_ppc64el.txt-	137 actionable tasks: 137 executed
--
openjfx_s390x.txt:	BUILD SUCCESSFUL in 2m 34s
openjfx_s390x.txt-	137 actionable tasks: 137 executed

@jgneff
Copy link
Member Author

jgneff commented May 16, 2023

Two GitHub pre-submit tests failed, but they appear to be temporary network errors:

  • [Linux] Connecting to archive.apache.org (archive.apache.org)|138.201.131.134|:443... failed: Connection timed out.
  • [Windows] curl: (28) Failed to connect to archive.apache.org port 443 after 21065 ms: Couldn't connect to server

@jgneff jgneff marked this pull request as ready for review May 16, 2023 01:04
@openjdk openjdk bot added the rfr Ready for review label May 16, 2023
@mlbridge
Copy link

mlbridge bot commented May 16, 2023

Webrevs

@jgneff
Copy link
Member Author

jgneff commented May 16, 2023

@Glavo I would also appreciate your comments and suggestions. I modified the initial patch that you said you were using, so please let me know if the latest changes in this pull request still work for you on the MIPS64el and RISC-V architectures. Thanks!

@kevinrushforth kevinrushforth self-requested a review May 22, 2023 21:20
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Changing the errors to warnings seems good, as does adding the additional artifact. I left a question about the IS_64 change.

buildSrc/linux.gradle Show resolved Hide resolved
@kevinrushforth kevinrushforth self-requested a review June 17, 2023 14:20
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 12, 2023

@jgneff This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Copy link
Contributor

@Glavo Glavo left a comment

Choose a reason for hiding this comment

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

I tested it on Debian RISC-V 64 and it works fine.

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 10, 2023

@jgneff This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Aug 10, 2023
@jgneff
Copy link
Member Author

jgneff commented Aug 10, 2023

/open

I would still prefer to have this patch integrated upstream instead of maintaining it separately myself. Sorry for letting it close. I thought the comment from Glavo would have kept it open for another round, but it seems that only a comment from the author can keep it alive.

@openjdk openjdk bot reopened this Aug 10, 2023
@openjdk
Copy link

openjdk bot commented Aug 10, 2023

@jgneff This pull request is now open

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

This fell off my radar too. Can you merge in the latest master to get a recent GHA run? I'll take a look in the next few days.

buildSrc/linux.gradle Show resolved Hide resolved
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

The changes look good. I did a quick test and it does what I would expect.

@openjdk
Copy link

openjdk bot commented Aug 11, 2023

@jgneff 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:

8307316: Let JavaFX be built on unknown architectures

Reviewed-by: kcr

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 1 new commit pushed to the master branch:

  • 8998b2d: 8314141: Missing default for switch in CreateBitmap

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 Aug 11, 2023
@jgneff
Copy link
Member Author

jgneff commented Aug 11, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Aug 11, 2023

Going to push as commit bedb964.
Since your change was applied there has been 1 commit pushed to the master branch:

  • 8998b2d: 8314141: Missing default for switch in CreateBitmap

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 11, 2023
@openjdk openjdk bot closed this Aug 11, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Aug 11, 2023
@openjdk
Copy link

openjdk bot commented Aug 11, 2023

@jgneff Pushed as commit bedb964.

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

@jgneff jgneff deleted the allow-armhf-i386-ppc64el-s390x branch August 11, 2023 22:22
@mlbridge
Copy link

mlbridge bot commented Oct 12, 2023

Mailing list message from Gregor Schmid on openjfx-dev:

Hi Johan and all,

WebP vulnerabilty: https://nvd.nist.gov/vuln/detail/CVE-2023-4863

I guess we're not the only ones having been busy in the past weeks
recovering from that disaster, updating the WebP libraries in all the
various places.

What we haven't covered yet is WebView as part of JavaFX as part of
the JRE distributed with QF-Test. As I haven't seen anything on this
list, my questions are:

Is WebView affected? Given that WebKit supports WebP I would assume
yes.

If so, what are the plans? I see that, for example, JavaFX 17.0.9 from
Gluon is planned for release on October 17. Will it have a WebView
update with a clean WebP?

Thanks for any info and best regards,
Greg

--
Gregor Schmid

Quality First Software GmbH
A company of mgm technology partners

B?rgermeister-Graf-Ring 10
82538 Geretsried

Phone: +49 8171 38648-11
Email: gregor.schmid at qfs.de | gregor.schmid at mgm-tp.com
Web: www.qfs.de

Commercial Register: HRB M?nchen 140833
Managing Directors: Gregor Schmid, Karlheinz Kellerer

The data protection information in accordance with the EU General Data
Protection Regulation applies to authorized representatives /
authorized representatives of "legal persons" in accordance with Art.
12 ff. GDPR
https://www.qfs.de/fileadmin/Webdata/pdf/en/dsgvo.pdf

@mlbridge
Copy link

mlbridge bot commented Oct 18, 2023

Mailing list message from Gregor Schmid on openjfx-dev:

I apologize for not testing this earlier.

Safari supports WebP images, so I naively assumed this to be a generic
feature of WebKit.

As it turns out, this it not the case. The WebView component in JavaFX
does not support WebP so I assume that JavaFX was never affected by
the WebP vulnerability.

Best regards,
Greg

Gregor Schmid <gschmidj at qfs.de> writes:

Hi Johan and all,

WebP vulnerabilty: https://nvd.nist.gov/vuln/detail/CVE-2023-4863

I guess we're not the only ones having been busy in the past weeks
recovering from that disaster, updating the WebP libraries in all the
various places.

What we haven't covered yet is WebView as part of JavaFX as part of
the JRE distributed with QF-Test. As I haven't seen anything on this
list, my questions are:

Is WebView affected? Given that WebKit supports WebP I would assume
yes.

If so, what are the plans? I see that, for example, JavaFX 17.0.9 from
Gluon is planned for release on October 17. Will it have a WebView
update with a clean WebP?

Thanks for any info and best regards,
Greg

--
Gregor Schmid

Quality First Software GmbH
A company of mgm technology partners

B?rgermeister-Graf-Ring 10
82538 Geretsried

Phone: +49 8171 38648-11
Email: gregor.schmid at qfs.de | gregor.schmid at mgm-tp.com
Web: www.qfs.de

Commercial Register: HRB M?nchen 140833
Managing Directors: Gregor Schmid, Karlheinz Kellerer

The data protection information in accordance with the EU General Data
Protection Regulation applies to authorized representatives /
authorized representatives of "legal persons" in accordance with Art.
12 ff. GDPR
https://www.qfs.de/fileadmin/Webdata/pdf/en/dsgvo.pdf

--
Gregor Schmid

Quality First Software GmbH
A company of mgm technology partners

B?rgermeister-Graf-Ring 10
82538 Geretsried

Phone: +49 8171 38648-11
Email: gregor.schmid at qfs.de | gregor.schmid at mgm-tp.com
Web: www.qfs.de

Commercial Register: HRB M?nchen 140833
Managing Directors: Gregor Schmid, Karlheinz Kellerer

The data protection information in accordance with the EU General Data
Protection Regulation applies to authorized representatives /
authorized representatives of "legal persons" in accordance with Art.
12 ff. GDPR
https://www.qfs.de/fileadmin/Webdata/pdf/en/dsgvo.pdf

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
3 participants