openjfx / javafx-gradle-plugin

Gradle plugin that makes it easy to work with JavaFX 11+
https://openjfx.io/
BSD 3-Clause "New" or "Revised" License
359 stars 62 forks source link

Use Gradle's variant-aware dependency management and allow using Gradle's built-in Java Module System support #151

Closed jjohannes closed 1 year ago

jjohannes commented 1 year ago

Hi πŸ‘‹ I recently became aware of the current state of JavaFX support in Gradle and there are two things that are quite painful for some users right now. Both could be improved, if the implementation in this plugin (and maybe later JavaFX itself) would rely on Gradle's variant-aware dependency management and Gradle Metadata.

The issues

  1. It's hard to publish libraries relying on JavaFX with good metadata due to the cross-cutting concern of selecting the right "JavaFX Platform Jars". (Which is one of the main use cases that variant-aware dependency management with variant selection based on attributes solves.) See e.g.: https://github.com/gradle/gradle/issues/26144
  2. This plugin is bound to another plugin –org.javamodularity.moduleplugin – right now. That does not work well together with Gradle's built-in Module System support (added in Gradle 6.4). It would be good if users of JavaFX can choose which of the mechanism they like to use.

Details

(My background is that I worked on both the dependency management and the Java Module support in Gradle – I was part of the Gradle core development team for several years – and continue contributing in these areas.)

This PR proposes a full implementation. It also extends the test coverage to show that things still work as before (from the user perspective) but better. It purposefully does not change the API of JavaFXOptions so that the changes should not have an effect on how users use this plugin. Before the actual code change, this PR contains three commits for the following:

  1. Updating the Gradle Wrapper to the current release (8.3)
  2. Updating the "build.gradle" and "settings.gradle" and the plugins used in them to the latest standards and features. Basically doing some cleanup there. I followed what we do for all the plugins of https://gradlex.org/
  3. Updating and extending the smoke test. It now:
    • Tests against multiple Gradle versions
    • Checks the actual -classpath and --module-path parameters used when Gradle calls javac and java by parsing Gradle's --debug output.
    • Has two additional tests: building with a local JavaFX sdk, using Gradle's built-in Module System support (since Gradle 6.4)

Main code changes

With these changes, the minimal supported Gradle version becomes 6.1 as only then all the dependency management APIs used are available.

Next steps / JavaFX and Gradle Metadata

The change contains a JavaFXComponentMetadataRule. This extends the metadata of JavaFX. In the future, JavaFX could publish this information (using Gradle Metadata) directly. Then this plugin will mostly become a convenience for defining dependencies to JavaFX. But, in particular in Module Path-based projects, JavaFX could even be used without this plugin.


I know this is a rather large change. I can add more documentation and explain things in more detail. I wanted to push this out to get the discussion started.

samypr100 commented 1 year ago

FWIW I can at least confirm that this is working on a few projects that I quickly tried it (FXyz, litfx, controlsfx, hansolo charts, trinity, etc.) on. It generates the GMM and POM I would expect.

Was able to even remove the manual pom modifiers used on some of these projects to remove the classifier from the generated maven poms.

pom.withXml {
    asNode().dependencies.'*'
            .findAll() { it.groupId.text() == 'org.openjfx' }
            .each { it.remove(it.classifier) }
}

One thing worthy of mention is that projects that relied knowingly/unknowingly on the transitive 'org.javamodularity.moduleplugin' will now have to be explicit about it otherwise their builds could likely fail as a result, hence I’d consider that a breaking change from a plugin perspective so maybe it deserves a bigger version bump, or doc updates highlighting this change.

johanvos commented 1 year ago

Thanks for the PR, I believe that can indeed be helpful to developers. Pinging @abhinayagarwal , @jperedadnr or @tiainen : can one of you review this?

jjohannes commented 1 year ago

Thank you for the quick response and the thorough review @jperedadnr. I addressed the feedback and answered the questions. Let me know if you have more.

nlisker commented 1 year ago

I think that this will fix the following issues: https://github.com/openjfx/javafx-gradle-plugin/issues/133 https://github.com/openjfx/javafx-gradle-plugin/issues/126 https://github.com/openjfx/javafx-gradle-plugin/issues/94

Possibly https://github.com/openjfx/javafx-gradle-plugin/issues/26 too.

jperedadnr commented 1 year ago

Okay, let's merge and test before doing a new release, and see if those issues can be closed.

nlisker commented 1 year ago

I can test on my local pet projects where I hit this bug. If there's a nightly/integration version out I will test before a stable release is made.

jperedadnr commented 1 year ago

The snapshot gets published to Sonatype, you just need:

pluginManagement {
    repositories {
        maven {
            url = "https://oss.sonatype.org/content/repositories/snapshots"
        }
        gradlePluginPortal()
    }
}
nlisker commented 1 year ago

I tested 15-SNAPSHOT and can confirm that, in Eclipse, the JavaFX modules now appear as "is modular" while "Project and External Dependencies" is on the classpath, and the test libraries (JUnit) appear as "not modular". This allows the tests to work on the classpath as they should, while there are no JavaFX missing modules in module-info.java.

image

I also rolled back to version 14 and saw the problem reappearing, so the snapshot version definitely saves the day here.

Thanks @jjohannes!

nlisker commented 1 year ago

I'll note that this revives a problem with the gluonfx gradle plugin that was partially fixed in https://github.com/gluonhq/gluonfx-gradle-plugin/pull/107. I can provide a reproduceable example on that project's repo if needed.

abhinayagarwal commented 1 year ago

@nlisker can you please open a new issue for this?

nlisker commented 1 year ago

@nlisker can you please open a new issue for this?

Created https://github.com/gluonhq/gluonfx-gradle-plugin/issues/179