8309381: Support JavaFX incubator modules #1616

kevinrushforth · 2024-10-29T22:23:35Z

This PR add the necessary support for JavaFX Incubator Modules. It includes the following:

Changes to the build scripts build.gradle and settings.gradle to document where to add your incubator module. Also added jlink flags to not resolve incubator modules by default and warn when resolving (same as is done for JDK incubator modules).
A utility class to produce warnings when first using an incubator module.
Changes to javafx.base/module-info.java to document where to add the needed qualified exports to your module to access the utility class in 2.

See PR #1617 for a sample module that is built on top of this PR's source branch.

This is almost ready to review. I will take it out of Draft in the next day or so.

/reviewers 2 reviewers
/csr

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
Change requires CSR request JDK-8344644 to be approved

Issues

JDK-8309381: Support JavaFX incubator modules (Enhancement - P3)
JDK-8344644: Support JavaFX incubator modules (CSR)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Ambarish Rapte (@arapte - Reviewer) Review applies to 1d262fb3

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1616

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-29T22:24:07Z

👋 Welcome back kcr! 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 · 2024-10-29T22:24:51Z

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

8309381: Support JavaFX incubator modules

Reviewed-by: angorya, arapte

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:

da6ad4b: 8343336: Add persistentScrollBars preference

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 · 2024-10-29T22:25:10Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

openjdk · 2024-10-29T22:25:48Z

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@kevinrushforth please create a CSR request for issue JDK-8309381 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

kevinrushforth · 2024-11-01T22:18:37Z

@andy-goryachev-oracle @arapte Can you review?

mlbridge · 2024-11-01T22:22:07Z

Webrevs

andy-goryachev-oracle

looks consistent with the original changes, but easier to understand.

had a few minor suggestions, will re-approve if you decide to fix it (WRAIYDTFI).

andy-goryachev-oracle · 2024-11-05T22:24:37Z

settings.gradle

+        "jsobject",
+        "web",
+        "media",
+        "systemTests"


thank you for placing each entry on a separate line!

I would prefer to remove the blank lines around the comment so as not to break the visual grouping (the comments are typically syntax colored anyway unless the user is on TRS80)

andy-goryachev-oracle · 2024-11-05T22:24:59Z

settings.gradle

+// BEGIN: incubator placeholder
+//project(":incubator.mymod").projectDir = file("modules/jfx.incubator.mymod")
+// END: incubator placeholder
+


same suggestion about empty lines

andy-goryachev-oracle · 2024-11-05T22:25:50Z

build.gradle

+        // BEGIN: incubator placeholder
+        //'incubator.mymod',
+        // END: incubator placeholder
+


(same comment about empty lines)

andy-goryachev-oracle · 2024-11-05T22:31:14Z

build.gradle


+// Add a project declaration for each incubator module here, leaving the
+// incubator placeholder lines as an example.
+// BEGIN: incubator placeholder


Q: maybe we should mention the JBS every time we say the word "incubator"?

It might be useful to anyone who is looking at the code and has no access to git history (or when git history is obscured by a move). I mean, JBS is our knowledge base, and it usually helps.

I generally prefer not to do this. In this case, the JBS Enhancement isn't what we would want to point to anyway (it would just be a placeholder). I can see some value in pointing to the CSR, so I'll add that here (I don't think there a need to repeat it).

What would really be useful, though, is a pointer to a template with a sample module, along the lines of what I mentioned in #1617, and a pointer to the JEP (which gets back to an earlier discussion of a permanent repository for JEPs). I'll file a follow-up issue to address this.

thank you, the CSR is definitely better.

Referring to a ticket has another positive side effect: it's easy to grep.

johanvos · 2024-11-14T09:32:33Z

build.gradle

+        'graphics',
+        'controls',
+
+        // Add an entry for each incubator module here, leaving the incubator


Can we check for the existence of a file in e.g. buildSrc/incubator.gradle instead, and if so, modify things like dependendProjects in that file?
The build.gradle is already huge and hard to understand/maintain. Every line we add to it makes it harder.

That's an interesting idea. I'll prototype it and see how it looks.

kevinrushforth · 2024-11-22T19:34:38Z

I made the changes to split out the build logic from build.gradle so that adding a new incubator module will not require touching the main build.gradle file. I pushed this to a new 8309381-incubator.v2 branch to evaluate whether to update this PR with that approach.

Click here to see the diff between this PR branch and v2.

See README-incubator.md for instructions on creating a new incubator module.

I also updated the sample incubator module, jfx.incubator.myfeature in my jfx.incubator.v2 branch.

Click here to see the jfx.incubator.v2 diffs.

@andy-goryachev-oracle Can you take a look?

I do think it makes a better separation to get the build logic out of build.gradle, but I wonder whether we want to wait and do this as part of the larger build.gradle cleanup.

@nlisker @johanvos What do you think?

andy-goryachev-oracle · 2024-11-22T19:57:15Z

@andy-goryachev-oracle Can you take a look?

could you create a draft PR for v2, so we can leave comments?

kevinrushforth · 2024-11-22T20:32:59Z

could you create a draft PR for v2, so we can leave comments?

Done. I created Draft PR #1646

nlisker · 2024-11-23T02:10:21Z

I had a look (at v2). If this approach works then it's fine for now, but it repeats the sin of coupling configurations of (potentially) unrelated modules and will increase the build time for a normal build (at least it will from the looks of it). It can be fix as part of the cleanup, but I don't see an easy way to make it work "properly" with the current setup.

johanvos · 2024-11-23T10:18:56Z

I had a look (at v2). If this approach works then it's fine for now, but it repeats the sin of coupling configurations of (potentially) unrelated modules and will increase the build time for a normal build (at least it will from the looks of it). It can be fix as part of the cleanup, but I don't see an easy way to make it work "properly" with the current setup.

I second that. Hence, I am ok with it as the build.gradle rehaul will probably take much more time, and the incubator module PR should not have to wait for that. It stresses the importancy of doing the "cleanup" (or modernization) of build.gradle, but I think we are all aware of that, so +1.

kevinrushforth · 2024-11-23T17:08:55Z

@nlisker @johanvos @andy-goryachev-oracle Thanks for taking a look. On further reflection, I prefer to stick with the version in this PR, rather than doing something ad hoc for just the incubator modules (Andy recommended the same in this comment on Draft PR v2).

So I propose to integrate this PR "as is", and file a follow-up bug for further improvements, linking it to the overall Gradle modernization umbrella Task, JDK-8344728.

I'll wait until at least Tuesday of next week to see if there are additional comments.

nlisker · 2024-11-27T15:47:53Z

Alright, the changes are not big, so if you feel it's better to have them inside I'm fine with it. It's not going to be the make-or-break for the build file.

andy-goryachev-oracle · 2024-12-02T18:51:30Z

This PR has been reviewed by 2 "R"eviewers, the last changes are trivial (added a comment and removed some blank lines).

I am going to downgrade the requirements so it can be integrated.

/reviewers 1

openjdk · 2024-12-02T18:51:45Z

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 1 (with at least 1 Reviewer).

kevinrushforth · 2024-12-02T18:53:34Z

Thanks for the reviews.

/integrate

openjdk · 2024-12-02T18:53:53Z

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

da6ad4b: 8343336: Add persistentScrollBars preference

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-02T18:54:01Z

@kevinrushforth Pushed as commit 67eed6d.

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

kevinrushforth added 2 commits October 28, 2024 07:12

8309381: Support JavaFX incubator modules

260a1f1

Remove call to doPrivileged

7f2632a

openjdk bot added the csr Need approved CSR to integrate pull request label Oct 29, 2024

This was referenced Oct 29, 2024

JavaFX Incubator Modules #1617

Closed

JavaFX Incubator Modules #1375

Closed

8301121: RichTextArea Control (Incubator) #1524

Closed

kevinrushforth marked this pull request as ready for review November 1, 2024 22:18

openjdk bot added the rfr Ready for review label Nov 1, 2024

andy-goryachev-oracle approved these changes Nov 5, 2024

View reviewed changes

arapte approved these changes Nov 14, 2024

View reviewed changes

johanvos reviewed Nov 14, 2024

View reviewed changes

openjdk bot added ready Ready to be integrated and removed csr Need approved CSR to integrate pull request labels Nov 21, 2024

Merge branch 'master' into 8309381-incubator.dev

1d262fb

kevinrushforth mentioned this pull request Nov 22, 2024

Draft: 8309381: Support JavaFX incubator modules (v2) #1646

Closed

3 tasks

kevinrushforth added 2 commits December 2, 2024 08:57

Merge branch 'master' into 8309381-incubator.dev

aa9809d

Remove extra blank lines

d9f0ab7

Add pointer to CSR in JBS

cc2499b

kevinrushforth requested review from andy-goryachev-oracle and arapte December 2, 2024 17:18

openjdk bot removed the ready Ready to be integrated label Dec 2, 2024

andy-goryachev-oracle approved these changes Dec 2, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Dec 2, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 2, 2024

openjdk bot closed this Dec 2, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Dec 2, 2024

8309381: Support JavaFX incubator modules #1616

8309381: Support JavaFX incubator modules #1616

Uh oh!

Conversation

kevinrushforth commented Oct 29, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 29, 2024

Uh oh!

openjdk bot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 29, 2024

Uh oh!

openjdk bot commented Oct 29, 2024

Uh oh!

kevinrushforth commented Nov 1, 2024

Uh oh!

mlbridge bot commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Nov 22, 2024

Uh oh!

andy-goryachev-oracle commented Nov 22, 2024

Uh oh!

kevinrushforth commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nlisker commented Nov 23, 2024

Uh oh!

johanvos commented Nov 23, 2024

Uh oh!

kevinrushforth commented Nov 23, 2024

Uh oh!

nlisker commented Nov 27, 2024

Uh oh!

andy-goryachev-oracle commented Dec 2, 2024

Uh oh!

openjdk bot commented Dec 2, 2024

Uh oh!

kevinrushforth commented Dec 2, 2024

Uh oh!

openjdk bot commented Dec 2, 2024

Uh oh!

openjdk bot commented Dec 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

kevinrushforth commented Oct 29, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 29, 2024 •

edited

Loading

mlbridge bot commented Nov 1, 2024 •

edited

Loading

kevinrushforth commented Nov 22, 2024 •

edited

Loading