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

8248122: java.base should not handle JavaFX application in a specific way #978

Closed
wants to merge 1 commit into from
Closed

Conversation

amCap1712
Copy link
Contributor

@amCap1712 amCap1712 commented Oct 31, 2020

JavaFX is no longer a part of OpenJDK. It makes sense to not treat it specially in the JDK. Hence, refactoring the Launcher class to remove JavaFX specific code.

Further investigation reveals that some JavaFX specific code is also present in the javadoc tool. For instance,
https://github.com/openjdk/jdk/blob/master/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java#L90-L96
https://github.com/openjdk/jdk/blob/master/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java#L347-L353
I think we should remove this code as well and the related tests for it.


Progress

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

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8248122: java.base should not handle JavaFX application in a specific way

Download

$ git fetch https://git.openjdk.java.net/jdk pull/978/head:pull/978
$ git checkout pull/978

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 31, 2020

👋 Welcome back amCap1712! 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 label Oct 31, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 31, 2020

@amCap1712 The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs label Oct 31, 2020
@amCap1712
Copy link
Contributor Author

@amCap1712 amCap1712 commented Oct 31, 2020

/cc openjfx

@openjdk
Copy link

@openjdk openjdk bot commented Oct 31, 2020

@amCap1712 The label openjfx is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt

@amCap1712
Copy link
Contributor Author

@amCap1712 amCap1712 commented Oct 31, 2020

/label add jdk
/label remove core-libs

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 31, 2020

Webrevs

@openjdk openjdk bot added the jdk label Oct 31, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 31, 2020

@amCap1712
The jdk label was successfully added.

@openjdk openjdk bot removed the core-libs label Oct 31, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 31, 2020

@amCap1712
The core-libs label was successfully removed.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Oct 31, 2020

/label add core-libs

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Oct 31, 2020

Please start a discussion on core-libs-dev before proposing this patch.

@openjdk openjdk bot added the core-libs label Oct 31, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 31, 2020

@AlanBateman
The core-libs label was successfully added.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

This will cause a regression in behavior. It will break existing JavaFX applications that do not have a main program. It could also break applications that create or use certain JavaFX objects in the class initializer of their JavaFX application.

There was some initial discussion around doing this as a follow-on to the removal of JavaFX from JDK 11, but if it is to be done, it needs to be discussed first. It would need to be done using a process similar to deprecation-for-removal. We would need to make changes in the JDK and/or JavaFX to warn about this in one release, and then remove it in the following. A CSR would be needed for both steps.

I note that while I disagree with the rationale described in JDK-8248122 for making this change, I am not necessarily opposed to the change itself.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 31, 2020

This also needs to be discussed on the openjfx-dev mailing list, since it will have behavioral compatibility implications for JavaFX.

@prrace
Copy link
Contributor

@prrace prrace commented Oct 31, 2020

Indeed this is very much of out the blue and the pre-existence of a bug report discussing this does not mean it
has been accepted to be done ..

And launcher changes need to be done carefully. I would expect them to come from someone who has a long history of contributions in this area and has had extensive discussions with stakeholders before moving on to code ..

@prrace
Copy link
Contributor

@prrace prrace commented Oct 31, 2020

/reviewers 3

@openjdk
Copy link

@openjdk openjdk bot commented Oct 31, 2020

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 28, 2020

@amCap1712 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!

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 26, 2020

@amCap1712 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!

@bridgekeeper bridgekeeper bot closed this Dec 26, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 29, 2020

Mailing list message from Jonathan Gibbons on core-libs-dev:

On 10/31/20 8:37 AM, Kartik Ohri wrote:

Further investigation reveals that some JavaFX specific code is also present in the `javadoc` tool. For instance,
https://github.com/openjdk/jdk/blob/master/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java#L90-L96
https://github.com/openjdk/jdk/blob/master/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java#L347-L353
I think we should remove this code as well and the related tests for it.

This would need to be discussed on javadoc-dev (at least), but at this
time, there are no plans to remove JavaFX support from the javadoc tool.

-- Jon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs jdk rfr
4 participants