-
Notifications
You must be signed in to change notification settings - Fork 61
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
Migration notes from 0.0.14 to 0.1.0 #154
Conversation
Note: I am not sure if all corner-cases of the migration are covered. I tried a rather simple non-modular application to update and it fails with
The project can be found here https://github.com/danielpeintner/Java11Test/tree/non-modular and the commit that makes it fail is danielpeintner/Java11Test@2a38e31 where I essentially just upgrade to 'org.openjfx.javafxplugin' version '0.1.0' and modularity.inferModulePath.set(false) The JavaFX dependencies are/were already excluded before The failing task is |
@danielpeintner Luckily I don't think you'd need to add |
This is a typical problem once a library does more with variants. Similar issues came up recently when Guava started to publish Gradle Metadata (google/guava#6612). When you use JavaFX now with the new setup, Gradle needs to know which "Platform" variant to choose. The JavaFX plugin makes this easy for the standard cases by configuring all @danielpeintner for explanation, you can now explicitly set which variation of JavaFX you want to have for that classpath, by doing something like this:
You probably do not want to do that, but just for explanation that now you could choose different Platforms (linux or windows or ...) in different places of the same build if desired. To get back to the old behavior, you can do this to "copy" the configuration from the
@samypr100 I am not sure if anything should be done in the plugin about this. We could think about some kind of convenience to let users define additional "classpaths" that should be configured. But depending on how individual these are, it might not be enough to just set the |
FYI: At first I thought this fixes the issue. I can now properly execute Having said that, once I run |
@danielpeintner Can you post which app this is about and its build file? |
No, it is still the non-modular one, see https://github.com/danielpeintner/Java11Test/tree/non-modular
Unfortunately not... |
@danielpeintner Not sure I can reproduce. I cloned the repo and the
Only changes I did were adding a toolchain to make sure I was using a proper jpackage compatible jdk since it was trying to use my JDK 11 install by default. java {
toolchain.languageVersion = JavaLanguageVersion.of(17)
} Also, app runs fine, no freezing. Thought: Might be worth comparing the |
@samypr100 Thank you very much for looking into. I did compare the For example in the "broken version" the
is missing. Hence also the according files Similarly the If you want to take a look I can share the jpackage results with you. |
@danielpeintner Definitely weird that they were there to begin with since those are empty jars. Those where removed on
+MODULES="java.base java.compiler java.datatransfer java.xml java.prefs java.desktop java.logging java.security.sasl java.naming java.security.jgss java.transaction.xa java.sql java.sql.rowset java.xml.crypto"
-MODULES="java.base java.compiler java.datatransfer java.xml java.prefs java.desktop java.logging java.security.sasl java.naming java.security.jgss java.transaction.xa java.sql java.sql.rowset java.xml.crypto jdk.jfr jdk.unsupported" This tells me the issue is with the runtime plugin detecting modules. I ran You can add them back in the runtime task via:
My hunch is that the missing I'm definitely curious why those modules are not "detected" anymore by |
@samypr100 thank you once again for your insights 👍 Wrapping it into the
Anyhow, it is somewhat weird to be required to do so... |
@danielpeintner Might be worth opening a separate issue and describing the discussion here thus far as otherwise it will be hard to track this. It's possible it can be a legitimate issue.
So those two entries should likely be there. The fact that they're missing between |
@danielpeintner I think I figured it out by taking a peek at the source code of badass-runtime-plugin I noticed they use On On The badass-jlink-plugin seems unnafected. Now that I see the root of the issue, this can be resolved temporarily on your Java11Test non-modular project by simply doing @jjohannes thoughts if this should be addressed on this plugin after the changes; for non-modular apps? It does feels weird to have to tell users you might have to declare all transitive dependencies on the migration guide in cases like this one where another plugin relied on the |
@samypr100 without having looked at all the details, for me using only Conceptually, I think what we do in this plugin is right. We rely on the JavaFX modules defining there transitive dependencies. Only the JavaFX modules you need directly are the ones you define. However, you could argue that, from the user perspective, it somehow was more useable before. If you combine it with the The change would be to always do what's in the I would probably not do that change. But I can also understand if you like to adjust that. |
Agreed. I did locally try that change earlier and it did fix the issue with the var javaFXModules = JavaFXModule.getJavaFXModules(this.modules);
var javaFXModulesWithTransitives = Stream.concat(
javaFXModules.stream(),
javaFXModules.stream()
.flatMap(m -> m.getDependentModules().stream()))
.distinct()
.sorted();
if (customSDKArtifactRepository == null) {
javaFXModulesWithTransitives.forEach(javaFXModule ->
dependencySet.add(getDependencies().create(
MAVEN_JAVAFX_ARTIFACT_GROUP_ID + ":" +
javaFXModule.getArtifactName() + ":" +
getVersion())));
} else {
// Use the list of dependencies of each module to also add direct dependencies for those.
// This is needed, because there is no information about transitive dependencies in the metadata
// of the modules (as there is no such metadata in the local sdk).
javaFXModulesWithTransitives.forEach(javaFXModule ->
dependencySet.add(getDependencies().create(
Map.of("name", javaFXModule.getModuleName()))));
} Like you said, main question is should this change be done here to support plugins that do the same thing as Another thought is that runtime-plugin/jlink-plugin are used quite extensively, so there's high likelyhood of this affecting others in a wider scope as they update to 0.1.0. Edit (Sep 19, 2023): Added migration notes for this particular situation in af3c0df |
After #151 got merged, I think adding these migration notes is worthwhile as it's easy to encounter the org.javamodularity.moduleplugin missing plugin issue on other larger projects.