On Wed, 9 Oct 2019 12:24:29 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> On Wed, 9 Oct 2019 07:50:44 GMT, Johan Vos <j...@openjdk.org> wrote: > >> On Tue, 8 Oct 2019 13:54:22 GMT, Kevin Rushforth <k...@openjdk.org> wrote: >> >>> JBS issue: [JDK-8226754](https://bugs.openjdk.java.net/browse/JDK-8226754) >>> >>> As noted in the JBS bug, the JavaFX build fails with gradle 6 (as well as >>> not building correctly with 5.6 or later). >>> >>> The existing JavaFX build uses two deprecated features that are removed in >>> gradle 6, so the build fails when building with gradle 6. Additionally, >>> some change that went into gradle 5.6 prevents all of our resource files >>> (e.g., css files, images, shaders) from being included in the built >>> artifacts, which causes JavaFX to be non-functional (our unit tests catch >>> this failure). >>> >>> The purpose of this bug fix is to allow JavaFX to build with gradle 6, >>> which is needed to allow building with JDK 13. We will likely upgrade to >>> gradle 6 in the near future. Additionally, this fixes the resource bug that >>> was exposed (or introduced) in gradle 5.6 and also affects gradle 6. >>> >>> The changes are as follows: >>> >>> 1. Remove unneeded STABLE_PUBLISHING setting, which was transitional to >>> allow gradle 4.x to continue working while we moved to gradle 5.x >>> 2. Use `ivy patternLayout ...` instead of `layout "pattern", ...` >>> 3. Specify no metadata for ivy repositories >>> 4. Set output.resourcesDir of sourceSet to match >>> processResources.destinationDir >>> 5. Bump minimum gradle version to 5.3 (since it will no longer run with >>> gradle 4.x after change 1) >>> >>> I verified that the build artifacts produced by gradle 5.3 before and after >>> this changes are identical (so it is behavior neutral for the supported >>> version of gradle). After the fix, I also verified that the build artifacts >>> produced by gradle 5.6.2 and 6.0 nightly match those produced by 5.3. I >>> have tested this fully on Linux and Windows, and I will do a sanity test on >>> Mac in parallel with the review. >>> >>> ---------------- >>> >>> Commits: >>> - bc6bd441: 8226754: FX build fails using gradle 5.6+ or 6 >>> >>> Changes: https://git.openjdk.java.net/jfx/pull/9/files >>> Webrev: https://webrevs.openjdk.java.net/jfx/9/webrev.00 >>> Issue: https://bugs.openjdk.java.net/browse/JDK-8226754 >>> Stats: 28 lines in 4 files changed: 17 ins; 4 del; 7 mod >>> Patch: https://git.openjdk.java.net/jfx/pull/9.diff >>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/9/head:pull/9 >> >> build.gradle line 1836: >> >>> 1835: url JFX_DEPS_URL >>> 1836: metadataSources { >>> 1837: artifact() >> >> From the JBS entry, I understood for now you wanted to keep layout (instead >> of patternLayout): >> https://bugs.openjdk.java.net/browse/JDK-8226754?focusedCommentId=14293009&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14293009 >> >> I understand the reasoning behind this (not using an incubating API), so I >> wonder why it is changed in this PR? > > I think you meant to point to [this earlier > comment](https://bugs.openjdk.java.net/browse/JDK-8226754?focusedCommentId=14273845&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14273845) > that I made back in June. Yes, I had indicated that I wanted to wait until > gradle 6 builds were available before switching from `layout` to > `patternLayout`, in case they made any changes. > > Your question does raise a good point, though: Should we wait until we > actually want to switch to gradle 6 before making this change? If so, then it > might make sense to split this change into two bugs: the `layout` --> > `patternLayout` changes, which would wait until we switch to gradle 6, and > the rest, which would be done now. > > I could go either way. Which do you prefer? https://docs.gradle.org/current/dsl/org.gradle.api.artifacts.repositories.IvyArtifactRepository.html still shows `patternLayout` to be incubating. Gradle doesn't have an excellent track record for finishing incubating API's (e.g. https://github.com/spring-projects/spring-boot/issues/11640). Hence, I think it is indeed safer not to switch from a deprecated API to an incubating API (and as a result, split the PR so that the `layout` --> `patternLayout` is not included for now. I don't have a very strong opinion on this though, so if you want to keep the changes, I don't object that. PR: https://git.openjdk.java.net/jfx/pull/9
