Add Java 8 support #28
Conversation
Glavo
force-pushed
the
main
branch
2 times, most recently
from
ea3f4ff
to
e780bbc
Compare
build.gradle.kts
Outdated
| tasks.getByName<Jar>("sourcesJar") { | ||
| exclude("module-info.class") | ||
| } |
There was a problem hiding this comment.
Why would this class file be part of the sources jar in the first place? Could you please elaborate on why you had to add this exclusion? (also if there is a good reason please add it as a comment in the code)
There was a problem hiding this comment.
Why would this class file be part of the sources jar in the first place? Could you please elaborate on why you had to add this exclusion? (also if there is a good reason please add it as a comment in the code)
After setting the mixedJavaRelease, the module-info.class will also be packaged into the sourcesJar. I think this may be a bug with the gradle-modules-plugin.
There was a problem hiding this comment.
Ok thank you. It might be worth reporting this to the plugin authors then. Do you have a link to an issue?
There was a problem hiding this comment.
It looks like there is a similar problem with the Javadoc jar too, by the way
There was a problem hiding this comment.
I fixed that in my branch and opened this issue: java9-modularity/gradle-modules-plugin#220
| tasks.compileTestJava { | ||
| extensions.configure<org.javamodularity.moduleplugin.extensions.CompileTestModuleOptions> { | ||
| isCompileOnClasspath = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Why did we need to fallback to classpath mode? Does that mean that consumers of the library will have to use the classpath instead of the module path as well?
There was a problem hiding this comment.
Why did we need to fallback to classpath mode? Does that mean that consumers of the library will have to use the classpath instead of the module path as well?
This is only for testing, and will not affect the release.
Some module related problems prevent the test, and this option can make the test work normally.
There was a problem hiding this comment.
This is only for testing, and will not affect the release
This specific setting is only for testing, yes. But the fact that you had to do this makes me wary because it might be a symptom of a problem that does affect the release.
Some module related problems prevent the test
Exactly. What is it? Is this problem going to affect consumers? That's exactly my question.
this option can make the test work normally
Yes, but it seems it's just hiding the problem with the modules (unless we understand what the problem is more clearly).
There was a problem hiding this comment.
@joffrey-bion In fact, Gradle's test could not correctly reflect the module related problems.
Testing is completely different from users. The user uses the last generated jar file, and the test is based on the class files in the folder.
Moreover, the test itself in this project has broken the module assumption —— Jigsaw does not allow different modules to contain the same package, so your tests can work normally only after they have invaded the module.
The Jigsaw problem encountered here should be caused by a bug in the gradle modules plugin. This plugin has many bugs. If the JavaFX Gradle Plugin plugin does not depends on it, I do not want to use it.
However, based on my experience working on dozens of modular Java projects, I think it is reasonable to use classpath directly. Because I haven't seen any build tool that can correctly handle Jigsaw in all cases, Gradle itself also has many module related bugs, it is difficult to avoid using workaround in projects to bypass problems related to jigsaw.
Compiling tests using the classpath does not cause any more problems. This is how Gradle works by default. I'm just asking the gradle module plugin to use the default behavior of gradle, so that the plugin will not cause us trouble because of its own bugs.
There was a problem hiding this comment.
In fact, Gradle's test could not correctly reflect the module related problems.
It did for me in the past. Before your change, my tests had shown me I was lacking opens declarations for Gson for example.
The user uses the last generated jar file, and the test is based on the class files in the folder.
By default, yes, but I think that depends on how tests are setup. I think with the Gradle Modules Plugin this default changes, because it sets up Gradle to use the module path instead. That's probably why my tests could catch problems with modules so far.
However, based on my experience working on dozens of modular Java projects, I think it is reasonable to use classpath directly
I am not convinced that it is reasonable. A good quality library should ideally test both when the jar is used on the module path AND on the classpath.
There was a problem hiding this comment.
For some reason, despite the classpath mode it seems to pickup the problems in module-info.java, so it might be good enough for now 🤔
|
I think it is more convenient for me to maintain a fork that supports Java 8. |
Why is that? I'm actually working on Java 8 support right now based on your PR. Just because I'm not merging immediately without good enough reasons for each piece you added doesn't mean I'm not willing to add support. I'm genuinely trying to find the best way to do so. Not yolo something that work-ish without proper testing. |
|
Because I don't want to continue to spend too much time on Jigsaw, which will make me very depressed. In the past few years, I have spent a lot of time to solve these problems. I have tried gradle modules plugin, but it is not a good solution. It has too many bugs. For Jigsaw, I finally implemented a compiler dedicated to It is worth mentioning that a widely used plugin (ModiTect ) of Maven works in the same way. Many projects, including jackson, have added module-info.class in this way. |
|
It seems that you have added Java 8 support? I'm glad to see you accept these concessions. |
I understand your pain. I'm mostly working in Kotlin these days, so I have almost 0 contact with JPMS apart from this library.
What I'm trying to achieve here is to provide reasonable support for the majority of people. And given the numbers in the survey Java 8 is still quite prevalent despite being old. And I think if we could have JavaFX-specific numbers, the value might be much higher than 45% marketshare. This is why I'm willing to revisit my former decision of "just move on in v4, and users that want to use JRE 8 can use FX Gson v3". I think it's best if new features / bugfixes in this lib benefit all users.
As I mentioned in a comment above, I just tested with the classpath mode and it seems to still pick up issues with |
|
Released in FX Gson 5.0.0 |
By setting the
mixedJavaRelease, we can easily providemodule-info.classwhile supporting Java 8.Many users are still using Java 8. We have a popular JavaFX application that has to remain compatible with Java 8, and it depends on fx-gson. Providing support for Java 8 allows us to upgrade fx gson from 3. x to 4. x, so we hope you can merge this PR.