-
Notifications
You must be signed in to change notification settings - Fork 62
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
Use Gradle's variant-aware dependency management and allow using Gradle's built-in Java Module System support #151
Conversation
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. |
Thanks for the PR, I believe that can indeed be helpful to developers. |
There was a problem hiding this 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.
...roject/modular-with-modularity-plugin/src/main/java/org/openjfx/gradle/javafx/test/Main.java
Show resolved
Hide resolved
test-project/modular-with-modularity-plugin/src/main/java/module-info.java
Show resolved
Hide resolved
test-project/local-sdk/src/main/java/org/openjfx/gradle/javafx/test/Main.java
Show resolved
Hide resolved
- 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)
0a44311
to
59f1e8f
Compare
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. |
e3520c5
to
bc3f697
Compare
bc3f697
to
3c9b4de
Compare
There was a problem hiding this 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).
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Okay, let's merge and test before doing a new release, and see if those issues can be closed. |
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. |
The snapshot gets published to Sonatype, you just need:
|
I tested I also rolled back to version 14 and saw the problem reappearing, so the snapshot version definitely saves the day here. Thanks @jjohannes! |
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. |
@nlisker can you please open a new issue for this? |
|
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
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:-classpath
and--module-path
parameters used when Gradle callsjavac
andjava
by parsing Gradle's--debug
output.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.