8343196: Add build property to identify experimental builds of JavaFX #1637

arapte · 2024-11-14T12:01:29Z

Introduce a new build property jfx.experimental.release.suffix to be used for the early access builds of an under development feature in JavaFX.
This property would be set to a value ONLY in a branch specific to an experiment in jfx-sandbox repo.
For example:
For the metal branch in jfx-sandbox repo, it would be set to -metal. The javafx.version for the early access build generated from that branch would be 24-metal.

And it would always be empty for master branch in both main jfx repo and in jfx-sandbox repo.
This change has no effect on a regular developer build from master branch.

Progress

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

Issue

JDK-8343196: Add build property to identify experimental builds of JavaFX (Enhancement - P3)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Johan Vos (@johanvos - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1637

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

Using diff file

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

Using Webrev

Link to Webrev Comment

arapte · 2024-11-14T12:02:02Z

@kevinrushforth Please take a look.

bridgekeeper · 2024-11-14T12:02:11Z

👋 Welcome back arapte! 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-11-14T12:03:01Z

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

8343196: Add build property to identify experimental builds of JavaFX

Reviewed-by: kcr, jvos

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:

7d1b2c3: 8342993: Remove uses of AccessController and AccessControlContext from JavaFX

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.

mlbridge · 2024-11-14T12:07:04Z

Webrevs

kevinrushforth · 2024-11-14T15:37:39Z

Reviewers: @kevinrushforth @johanvos

/reviewers 2

openjdk · 2024-11-14T15:38:06Z

@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 1 Reviewer, 1 Author).

kevinrushforth · 2024-11-14T15:38:42Z

@arapte Can you merge in the latest master? The macOS GHA failure is due to your being out of date.

The fix looks good to me. I'll do some testing before approving.

arapte · 2024-11-15T13:03:29Z

@arapte Can you merge in the latest master?

Thanks @kevinrushforth , The checks pass after merging in the latest master.

kevinrushforth

Looks good. I left a couple comments and will reapprove if you decide to make the changes.

kevinrushforth · 2024-11-16T15:09:40Z

build.gradle

+        relSuffix = jfxExperimentalReleaseSuffix != "" ?
+                        jfxExperimentalReleaseSuffix : jfxReleaseSuffix;


One thought I had: we might also want to set relOpt = "-${buildTimestamp}" here like we do for internal builds so we always see a time stamp for experimental builds. What do you think?

I agree with that. It is often useful to have timestamp info in the builds during development.
I know I'm a broken record on this topic, but if possible, it would be great if (parts of) this logic can be moved into a separate file in order to make the build.gradle smaller and more readable. I'm not insisting on this for this PR, but I think it is worth thinking about this whenever changes to build.gradle are made.

I know I'm a broken record on this topic, but if possible, it would be great if (parts of) this logic can be moved into a separate file in order to make the build.gradle smaller and more readable.

Also at the risk of sounding like a broken record, #1232 has been waiting to start this process for a while now and it's stuck waiting for explanations on how publishing works, something only Oracle and Gluon do. If someone can help Jendrik Johannes there then the ball will get rolling. I tried to split the big build file into separate files for each module, but it's entangled with the code in that PR. Completing that PR might also remove the need for the JavaFX plugin.

I know I'm a broken record on this topic, but if possible, it would be great if (parts of) this logic can be moved into a separate file in order to make the build.gradle smaller and more readable.

Also at the risk of sounding like a broken record, #1232 has been waiting to start this process for a while now and it's stuck waiting for explanations on how publishing works, something only Oracle and Gluon do. If someone can help Jendrik Johannes there then the ball will get rolling. I tried to split the big build file into separate files for each module, but it's entangled with the code in that PR. Completing that PR might also remove the need for the JavaFX plugin.

Fair remark. I just commented on the PR.

Added relOpt = "-${buildTimestamp}"

kevinrushforth · 2024-11-16T15:12:29Z

build.properties

+# The experimental release suffix is set only in jfx-sandbox repository
+# for a speficic branch. When this property is set it overrides


Do you want to say something about it needing to start with a -? Or do you think the example of -ea above will be clear enough?

Added a comment to describe the use of -, with example of -metal and -d3d12.

johanvos · 2024-11-16T15:37:22Z

I might be confused, but wasn't the idea to use a new property (e.g. jfx.feature.variant) instead of overloading the existing jfx.release.suffix?

kevinrushforth · 2024-11-16T16:16:58Z

I might be confused, but wasn't the idea to use a new property (e.g. jfx.feature.variant) instead of overloading the existing jfx.release.suffix?

This PR does add a new property in build.properties,jfx.experimental.release.suffix, and does not require modifying jfx.release.suffix. For example, in the "metal" branch of the jfx-sandbox, we would define this as:

jfx.experimental.release.suffix=-metal

johanvos · 2024-11-16T18:32:10Z

I might be confused, but wasn't the idea to use a new property (e.g. jfx.feature.variant) instead of overloading the existing jfx.release.suffix?

This PR does add a new property in build.properties,jfx.experimental.release.suffix, and does not require modifying jfx.release.suffix. For example, in the "metal" branch of the jfx-sandbox, we would define this as:
jfx.experimental.release.suffix=-metal

My apologies, I misread the proposed code.
It is a bit confusing to have "jfx.release.suffix" and "jfx.experimental.release.suffix" where the former refers to an artifact property and the latter refers to a major feature/variant/... like metal. But then, consistent naming is always difficult so I'm ok with the proposed suggestion.

kevinrushforth · 2024-11-16T22:32:30Z

It is a bit confusing to have "jfx.release.suffix" and "jfx.experimental.release.suffix" where the former refers to an artifact property and the latter refers to a major feature/variant/... like metal.

I think you have a good point. Maybe we should drop the '.suffix' in the name of this property (in which case, it wouldn't have a - so the logic in build.gradle would add the - when needed). It would probably be easier to document it as well.

What do you think?

kevinrushforth · 2024-11-16T22:36:17Z

If we do this, then here are some possible names:

jfx.experimental.release
jfx.experimental.release.name
jfx.variant.release
jfx.variant.release.name
jfx.variant
jfx.feature.variant

Thoughts?

johanvos · 2024-11-17T09:00:18Z

It is a bit confusing to have "jfx.release.suffix" and "jfx.experimental.release.suffix" where the former refers to an artifact property and the latter refers to a major feature/variant/... like metal.

I think you have a good point. Maybe we should drop the '.suffix' in the name of this property (in which case, it wouldn't have a - so the logic in build.gradle would add the - when needed). It would probably be easier to document it as well.

What do you think?

That makes sense (removing the word ".suffix")

johanvos · 2024-11-17T09:02:56Z

If we do this, then here are some possible names:

jfx.experimental.release

jfx.experimental.release.name

jfx.variant.release

jfx.variant.release.name

jfx.variant

jfx.feature.variant

Thoughts?

I don't have objections against any of those.
If I have to vote, my vote would go to "jfx.feature.name" (which is not in the list ;) ) because I associate variant rather with a single option from a wider set (e.g. prism variants are sw, es2,,...)
But I am more than happy with any name on this list.

nlisker · 2024-11-17T15:30:50Z

Variants are also something Gradle uses to select one of several components, so I suggest to avoid this overload. JavaFX should use Gradle variants for selection of operating systems.

I would have went with jfx.experimental.name :) because the sandbox has experimental features, as opposed to other features that are directly integrated into the main stream.

arapte · 2024-11-18T12:08:15Z

Why we chose the name jfx.experimental.release.suffix.
The JavaFX version string contains the value of jfx.release.suffix i.e -ea as a suffix. The new variable would be doing same, so it is also a suffix.
24-ea would be now 24-metal

and use of experimental is justified by @nlisker as:

I would have went with jfx.experimental.name :) because the sandbox has experimental features, as opposed to other features that are directly integrated into the main stream.

jfx.release.suffix : JavaFX relase version suffix for early access releases i.e -ea
jfx.experimental.release.suffix : JavaFX relase version suffix for experimental releases.

With this understanding, I am happy with the current name :) but shall update the PR we finalize any other name.

kevinrushforth · 2024-11-18T14:46:57Z

I'd rather have the property be something without suffix for the reasons Johan and I mentioned.

Even though it's a little longer, I like a combination of what Johan and Nir suggested:

jfx.experimental.feature.name -- The name of the experimental feature

No one will ever specify this on the command line or have to type it, so having it be more explicit is good, even though the name is a little long.

You can say something like:

Set this property to the name of the experimental feature for your branch. Do not include a dash in the name. For example, if you have a sandbox branch named myfeature you might set jfx.experimental.feature.name=myfeature.

arapte · 2024-11-18T18:09:23Z

jfx.experimental.feature.name -- The name of the experimental feature

Thanks @kevinrushforth , Updated the property name as suggested.

kevinrushforth

Looks good with one suggested change.

kevinrushforth · 2024-11-18T21:13:11Z

build.gradle

+        relSuffix = jfxExperimentalFeatureName != "" ?
+                        "-${jfxExperimentalFeatureName}" : jfxReleaseSuffix;
+        relOpt = "-${buildTimestamp}"


This will unconditionally add a timestamp to all EA builds of JavaFX, not just experimental builds. Changing the version string for EA builds seems out of scope for this PR.

Suggested change

relSuffix = jfxExperimentalFeatureName != "" ?

"-${jfxExperimentalFeatureName}" : jfxReleaseSuffix;

relOpt = "-${buildTimestamp}"

if (jfxExperimentalFeatureName != "") {

relSuffix = "-${jfxExperimentalFeatureName}"

relOpt = "-${buildTimestamp}"

} else {

relSuffix = jfxReleaseSuffix

}

Thanks @kevinrushforth , Included the change.

Review correction Co-authored-by: Kevin Rushforth <[email protected]>

kevinrushforth · 2024-11-19T16:41:39Z

Looks good now. @johanvos this should be ready for your review.

arapte · 2024-11-20T00:29:45Z

/integrate

openjdk · 2024-11-20T00:30:46Z

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

7d1b2c3: 8342993: Remove uses of AccessController and AccessControlContext from JavaFX

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-20T00:30:54Z

@arapte Pushed as commit 35ff442.

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

8343196: Add build property to identify experimental builds of JavaFX

355dec5

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

Merge branch 'master' into exp-suffix

c3395cc

kevinrushforth approved these changes Nov 16, 2024

View reviewed changes

review comments

e275089

change property name to jfx.experimental.feature.name

ee7b69b

correcttion

bbe492a

kevinrushforth reviewed Nov 18, 2024

View reviewed changes

Update build.gradle

26e7790

Review correction Co-authored-by: Kevin Rushforth <[email protected]>

kevinrushforth approved these changes Nov 19, 2024

View reviewed changes

johanvos approved these changes Nov 19, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Nov 19, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 20, 2024

openjdk bot closed this Nov 20, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8343196: Add build property to identify experimental builds of JavaFX #1637

8343196: Add build property to identify experimental builds of JavaFX #1637

arapte commented Nov 14, 2024 •

edited by openjdk bot

Loading

arapte commented Nov 14, 2024

bridgekeeper bot commented Nov 14, 2024

openjdk bot commented Nov 14, 2024 •

edited

Loading

mlbridge bot commented Nov 14, 2024 •

edited

Loading

kevinrushforth commented Nov 14, 2024

openjdk bot commented Nov 14, 2024

kevinrushforth commented Nov 14, 2024

arapte commented Nov 15, 2024

kevinrushforth left a comment

kevinrushforth Nov 16, 2024

johanvos Nov 16, 2024

nlisker Nov 16, 2024

johanvos Nov 17, 2024

arapte Nov 18, 2024

kevinrushforth Nov 16, 2024

arapte Nov 18, 2024 •

edited

Loading

johanvos commented Nov 16, 2024

kevinrushforth commented Nov 16, 2024 •

edited

Loading

johanvos commented Nov 16, 2024

kevinrushforth commented Nov 16, 2024

kevinrushforth commented Nov 16, 2024

johanvos commented Nov 17, 2024

johanvos commented Nov 17, 2024

nlisker commented Nov 17, 2024

arapte commented Nov 18, 2024

kevinrushforth commented Nov 18, 2024

arapte commented Nov 18, 2024

kevinrushforth left a comment

kevinrushforth Nov 18, 2024

arapte Nov 19, 2024

kevinrushforth commented Nov 19, 2024

arapte commented Nov 20, 2024

openjdk bot commented Nov 20, 2024

openjdk bot commented Nov 20, 2024

		relSuffix = jfxExperimentalReleaseSuffix != "" ?
		jfxExperimentalReleaseSuffix : jfxReleaseSuffix;

		# The experimental release suffix is set only in jfx-sandbox repository
		# for a speficic branch. When this property is set it overrides

8343196: Add build property to identify experimental builds of JavaFX #1637

8343196: Add build property to identify experimental builds of JavaFX #1637

Conversation

arapte commented Nov 14, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

arapte commented Nov 14, 2024

bridgekeeper bot commented Nov 14, 2024

openjdk bot commented Nov 14, 2024 • edited Loading

mlbridge bot commented Nov 14, 2024 • edited Loading

Webrevs

kevinrushforth commented Nov 14, 2024

openjdk bot commented Nov 14, 2024

kevinrushforth commented Nov 14, 2024

arapte commented Nov 15, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arapte Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

johanvos commented Nov 16, 2024

kevinrushforth commented Nov 16, 2024 • edited Loading

johanvos commented Nov 16, 2024

kevinrushforth commented Nov 16, 2024

kevinrushforth commented Nov 16, 2024

johanvos commented Nov 17, 2024

johanvos commented Nov 17, 2024

nlisker commented Nov 17, 2024

arapte commented Nov 18, 2024

kevinrushforth commented Nov 18, 2024

arapte commented Nov 18, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth commented Nov 19, 2024

arapte commented Nov 20, 2024

openjdk bot commented Nov 20, 2024

openjdk bot commented Nov 20, 2024

arapte commented Nov 14, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 14, 2024 •

edited

Loading

mlbridge bot commented Nov 14, 2024 •

edited

Loading

arapte Nov 18, 2024 •

edited

Loading

kevinrushforth commented Nov 16, 2024 •

edited

Loading