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

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

Merged

Conversation

jjohannes
Copy link
Contributor

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.: Allow excluding thirdPartyCompatibility from GMM gradle/gradle#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
Copy link
Contributor

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
Copy link
Contributor

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

@johanvos johanvos added the enhancement New feature or request label Aug 26, 2023
Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a quick review, it seems it works fine (tests work, and updated HelloFX samples work)
I have some somehow minor comments, and will give another review once these are addressed.

- Metadata Rule (metadata could later be published by JavaFX)
- Transitive dependencies no longer need to be maintained in the plugin
- Empty Jars are no longer on the classpath
- Manual classpath and module path modification limited to minimum
- No hard dependency on 'org.javamodularity:moduleplugin' anymore
  - Users can then choose between Gradle's built in Modul System
    support or the javamodularity plugin
- Dependency declaration done lazily (via withDependencies)
@jjohannes jjohannes force-pushed the use-variant-aware-dependency-management branch from 0a44311 to 59f1e8f Compare August 28, 2023 18:26
@jjohannes
Copy link
Contributor Author

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.

@jjohannes jjohannes force-pushed the use-variant-aware-dependency-management branch from e3520c5 to bc3f697 Compare August 29, 2023 09:53
@jjohannes jjohannes force-pushed the use-variant-aware-dependency-management branch from bc3f697 to 3c9b4de Compare August 29, 2023 09:54
@jperedadnr jperedadnr added CLA signed enhancement New feature or request and removed enhancement New feature or request labels Aug 30, 2023
Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I've done some testing with different JavaFX Gradle projects, all look good (after some updates that are needed once this PR is merged and a new version of the plugin is published).

Copy link
Collaborator

@abhinayagarwal abhinayagarwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested both modular and non-modular applications and libraries.

Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@nlisker
Copy link

nlisker commented Sep 2, 2023

I think that this will fix the following issues:
#133
#126
#94

Possibly #26 too.

@jperedadnr
Copy link
Collaborator

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

@jperedadnr jperedadnr merged commit f0a8d4c into openjfx:master Sep 2, 2023
2 checks passed
@jjohannes jjohannes deleted the use-variant-aware-dependency-management branch September 2, 2023 13:13
@nlisker
Copy link

nlisker commented Sep 2, 2023

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
Copy link
Collaborator

The snapshot gets published to Sonatype, you just need:

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

@nlisker
Copy link

nlisker commented Sep 2, 2023

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
Copy link

nlisker commented Sep 2, 2023

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

@abhinayagarwal
Copy link
Collaborator

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

@nlisker
Copy link

nlisker commented Sep 4, 2023

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

Created gluonhq/gluonfx-gradle-plugin#179

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants