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

8263760: Update gradle to version 7.0.1 #498

Closed

Conversation

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented May 12, 2021

This PR fixes the gradle deprecation warnings described in JDK-8240336 and updates the JavaFX build to use gradle 7.0.1 as described in JDK-8263760. The minimum version of gradle is set to 6.3.

In addition to keeping gradle up to date, updating to gradle 7 will allow building and testing JavaFX with JDK 16.

I have done a full build and test on all three platforms, comparing the artifacts produced before and after this change.

Notes to Reviewers:

The PR branch has two separate commits, one for each fix, in case reviewers want to look at them separately. As always, they will be squashed into a single commit when integrated. Both bug IDs will be listed in the commit.

The following changes were done for JDK-8240336 to eliminate the use of deprecated features removed in gradle 7:

  1. Replaced compile with either implementation or compileClasspath as needed
  2. Replaced obsolete archiveName and destinationDir properties in archive tasks with archiveFileName and destinationDirectory
  3. Added missing @Input annotation to custom Groovy task properties
  4. Bumped the minimum version of gradle to 6.3 (which we have been using for more than 1 year)

The following changes were done for JDK-8263760 to update to gradle 7.0.1:

  1. Ran bash ./gradlew wrapper --gradle-version=7.0.1
  2. Updated the gradle version in build.properties to 7.0.1
  3. Updated the SHA-256 checksum in gradle/wrapper/gradle-wrapper.properties

Progress

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

Issues

  • JDK-8263760: Update gradle to version 7.0.1
  • JDK-8240336: JavaFX build uses deprecated features that will be removed in gradle 7

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 498

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 12, 2021

👋 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.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented May 12, 2021

/issue 8240336
/issue add 8263760
/reviewers 2

@openjdk openjdk bot changed the title WIP: gradle 7 8240336: JavaFX build uses deprecated features that will be removed in gradle 7 May 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 12, 2021

@kevinrushforth The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

@openjdk
Copy link

@openjdk openjdk bot commented May 12, 2021

@kevinrushforth
Adding additional issue to issue list: 8263760: Update gradle to version 7.0.1.

@openjdk
Copy link

@openjdk openjdk bot commented May 12, 2021

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

@kevinrushforth kevinrushforth changed the title 8240336: JavaFX build uses deprecated features that will be removed in gradle 7 8263760: Update gradle to version 7.0.1 May 12, 2021
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented May 12, 2021

/issue remove 8263760
/issue add 8240336

@openjdk
Copy link

@openjdk openjdk bot commented May 12, 2021

@kevinrushforth
Removing additional issue from issue list: 8263760.

@@ -28,7 +44,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`

# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
DEFAULT_JVM_OPTS=""
DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Copy link

@Crazyjavahacking Crazyjavahacking May 12, 2021

This is automatically generated by Gradle, but the heap memory seems to be too low.

  • the same for gradlew.bat

Copy link
Member Author

@kevinrushforth kevinrushforth May 12, 2021

I agree with this concern, and will revert this auto-generated change. I don't know why the gradle team felt that a 64Mbyte heap was sufficient.

@openjdk
Copy link

@openjdk openjdk bot commented May 12, 2021

@kevinrushforth
Adding additional issue to issue list: 8240336: JavaFX build uses deprecated features that will be removed in gradle 7.

List sourceRoots = new ArrayList();
@Optional @Input String matches; // regex for matching input files
@Input List<String> params = new ArrayList<String>();
@Input List sourceRoots = new ArrayList();
Copy link

@Crazyjavahacking Crazyjavahacking May 12, 2021

Shouldn't this be @Internal?:

  1. updateFiles() uses sourceRoots and maps that into the allFiles field, which has already @InputFiles. If the sourceRoots is changed in a way that does not affect allFiles, this task can remain UP_TO_DATE and save task execution.

Copy link
Member Author

@kevinrushforth kevinrushforth May 12, 2021

Perhaps, but I wouldn't want to make this change as part of this bug fix. I'd rather not have to do the analysis of whether there is any case where sourceRoots can change in a way that wouldn't affect the outputs. If not, then this is only a theoretical concern, and if so, it seems better to err on the side of correctness unless you can prove that all similar cases won't affect the outputs.

@Optional @InputDirectory File headers;
@Optional Closure eachOutputFile; // will be given a File and must return a File
@Optional boolean exe = false;
@Optional @Input Closure eachOutputFile; // will be given a File and must return a File

Is this field actually used anywhere?

As this field is not initialized + there is the @Optional annotation, Gradle will not verify this is fine. Gradle docs is specifying that:

  1. @Input can be used on Closure if it is a provider of File (which means no param in closure).
  2. @Input can be used for any Serializable which Closure actually is.

It's not obvious if Gradle will correctly handle Closure taking argument.

Copy link
Member Author

@kevinrushforth kevinrushforth May 12, 2021

I'll take a look, but I will likely want to do any cleanup of this in a follow-on issue. As you note the eachOutputFile property is unused...as is the exe property. They were formerly used by one of the javapackager tasks, which has since been removed.

@kevinrushforth kevinrushforth marked this pull request as ready for review May 12, 2021
@openjdk openjdk bot added the rfr label May 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 12, 2021

Webrevs

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented May 13, 2021

On closer inspection, I am reverting my earlier change to gradlew (the one that reverted the setting of DEFAULT_JVM_OPTS to a 64Mbyte heap). The gradle launcher in gradle 7.0.1 also sets its DEFAULT_JVM_OPTS to a 64Mbyte heap as you can see in the following, taken from gradle-7.0.1/bin/gradle:

# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'

So even though I agree that a 64 Mbyte heap is a bit on the small side, it's more important that running gradlew behave the same as installing gradle and running $GRADLE_HOME/bin/gradle. Plus I'd prefer not to maintain local modifications of this upstream script.

@Crazyjavahacking
Copy link

@Crazyjavahacking Crazyjavahacking commented May 14, 2021

On closer inspection, I am reverting my earlier change to gradlew (the one that reverted the setting of DEFAULT_JVM_OPTS to a 64Mbyte heap). The gradle launcher in gradle 7.0.1 also sets its DEFAULT_JVM_OPTS to a 64Mbyte heap as you can see in the following, taken from gradle-7.0.1/bin/gradle:

# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'

So even though I agree that a 64 Mbyte heap is a bit on the small side, it's more important that running gradlew behave the same as installing gradle and running $GRADLE_HOME/bin/gradle. Plus I'd prefer not to maintain local modifications of this upstream script.

I don't think this is a good idea. The only modification of the wrapper scripts would need to be the removal of the heap setting, nothing else.

Setting 64MB heap will definitely cause performance issues for JFX builds (because of Garbage collection, not enough memory for caching, ...). + the previous setting was not to restrict the heap in Wrapper scripts. So someone already removed the heap setting as 64MB is the setting back from at least Gradle 4.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented May 14, 2021

We haven't updated the gradle wrapper since gradle 4.3. I did it this time as a better practice, which came up during the review of PR #411 in this comment.

Anyway, I do agree with you that the value is (surprisingly) low, but it's easy to override this by setting the GRADLE_OPTS env var. That's what we've always done in our CI build.

Let's see what the reviewers think. If others think we should change it, I'd certainly be willing to reconsider setting it to a higher value (which seems better than just removing the setting like I did in the earlier, now-reverted commit).

@swpalmer
Copy link
Collaborator

@swpalmer swpalmer commented May 14, 2021

Isn't that just the settings for running the gradle wrapper - i.e.  it is not setting the heap used by the gradle daemon process that will actually run the build script?
I only see it used one spot in the script to run the gradle-wrapper.jar, in which case it makes sense that it is intentionally set low as the wrapper itself isn't doing much other than ensuring the correct version of Gradle is used.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented May 15, 2021

Isn't that just the settings for running the gradle wrapper - i.e.  it is not setting the heap used by the gradle daemon process that will actually run the build script?

Ah yes, you are right. I just verified this with a test build. So this is definitely not an issue then. Thanks!

@Crazyjavahacking
Copy link

@Crazyjavahacking Crazyjavahacking commented May 16, 2021

Sorry for confusion. I misunderstand the mechanics.

Copy link
Collaborator

@tiainen tiainen left a comment

I've ran the build jobs and building and testing both completed successfully on all platforms.

arapte
arapte approved these changes May 19, 2021
Copy link
Member

@arapte arapte left a comment

Looks good to me. Verified build and test execution on mac and windows machine.

@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2021

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

8263760: Update gradle to version 7.0.1
8240336: JavaFX build uses deprecated features that will be removed in gradle 7

Reviewed-by: sykora, 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 13 new commits pushed to the master branch:

  • 485b242: 8264219: tools/scripts/build.ps1 is out of date and no longer works
  • e76b7b1: 8186904: TableColumnHeader: resize cursor lost on right click
  • 4619cdd: 8264138: Replace uses of Class.newInstance
  • 93de584: 8266966: Wrong CSS properties are applied to other nodes after fix for JDK-8204568
  • c511789: 8266860: [macos] Incorrect duration reported for HLS live streams
  • d2d145d: 8266643: Intermittent failure of HonorDeveloperSettingsTest unit test
  • 9c97d9b: 8267121: Illegal access to private "size" field of ArrayList from build.gradle
  • f236a7d: 8266539: [TreeView]: Change.getRemoved() contains null item when deselecting a TreeItem
  • 285a0b6: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
  • 8ca7815: 8267160: Monocle mouse never get ENTERED state
  • ... and 3 more: https://git.openjdk.java.net/jfx/compare/c7833f1205e3a97b49abb945d0a2eeff9b9c34bc...master

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 label May 19, 2021
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented May 19, 2021

/integrate

@openjdk openjdk bot closed this May 19, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2021

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

  • 485b242: 8264219: tools/scripts/build.ps1 is out of date and no longer works
  • e76b7b1: 8186904: TableColumnHeader: resize cursor lost on right click
  • 4619cdd: 8264138: Replace uses of Class.newInstance
  • 93de584: 8266966: Wrong CSS properties are applied to other nodes after fix for JDK-8204568
  • c511789: 8266860: [macos] Incorrect duration reported for HLS live streams
  • d2d145d: 8266643: Intermittent failure of HonorDeveloperSettingsTest unit test
  • 9c97d9b: 8267121: Illegal access to private "size" field of ArrayList from build.gradle
  • f236a7d: 8266539: [TreeView]: Change.getRemoved() contains null item when deselecting a TreeItem
  • 285a0b6: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
  • 8ca7815: 8267160: Monocle mouse never get ENTERED state
  • ... and 3 more: https://git.openjdk.java.net/jfx/compare/c7833f1205e3a97b49abb945d0a2eeff9b9c34bc...master

Your commit was automatically rebased without conflicts.

Pushed as commit 111bac4.

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

@kevinrushforth kevinrushforth deleted the 8263760-gradle-7 branch May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants