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
347 stars 59 forks source link

[Eclipse] Tests dependencies are not visible in modular project #168

Closed nlisker closed 2 months ago

nlisker commented 2 months ago

When using a modular project, the module-info dependencies need to be put on the modulepath. However, tests dependencies are put on the module path too, which is wrong, making test code not compile in Eclipse. See https://stackoverflow.com/questions/71453852/running-eclipse-junit-tests-with-gradle-builds-of-modular-java-javafx-projects https://stackoverflow.com/questions/66268068/gradle-cant-access-classes-defined-in-module-src-main-from-src-test-with-javafx

This needs to be looked at, and maybe some documentation needs to be updated.

nlisker commented 2 months ago

@jjohannes Do you have an idea what this plugin does to mess with the test dependencies?

jjohannes commented 2 months ago

However, tests dependencies are put on the module path too

@nlisker I don't think the plugin is influencing this. In general, it depends on the tests if you want to test on the module path or not. It is a very nuanced topic from my experience.

The links you shared are using the older version of the plugin that automatically applied org.javamodularity.moduleplugin – which may do more modification to the classpath / module path. But also has many configuration options for testing as far as I recall. But hat should not be a concern anymore for the javafx-gradle-plugin plugin 0.1.0.

If you have a reproducer of the problem you would like to solve that uses version 0.1.0 I can have a look.

nlisker commented 2 months ago

You are right. The Getting Started page for a modular Eclipse Gradle project has a step to add Eclipse-specific code to put the libraries on the module path, which puts the test dependencies there too, giving compilation errors since these are not in the module-info. Was that code necessary because the older versions used the modularity plugin and not now anymore?

My confusion came from a combination of that and that declaring a dependency on a local modular jar via

implementation files("lib/my.jar")

doesn't put the jar on the module path for some reason (is this a bug in Gradle?).

jjohannes commented 2 months ago

Looks to me as if this specific Eclipse setup should be no longer be necessary. The Module support in Gradle core is connected with the Eclipse integration and should be respected.

Gradle has the general approach to NOT put a Jar on the Module Path if the Jar does not have a module-info.class and no Automatic-Module-Name – because this can lead to errors if there are split-packages or Jar names that are not valid Module Names. Instead, these Jars are put one the Classpath (i.e. the UNNAMED Module). It does not matter where the Jars originate from (repository, local file, etc).

If you do...

implementation files("lib/my.jar")

...and my.jar is not on the Module Path, I would suspect that my.jar does not have Module information.

nlisker commented 2 months ago

I will file PRs to remove the unnecessary step.

Here is a jar that contains a module-info.class, but for me isn't put on the module-path without explicit change to its attributes in Eclipse: module.zip.

jjohannes commented 2 months ago

The Jar looks fine. Can you share a complete project showing the unexpected behavior? Maybe there is no src/test/java/module-info.java? (Then Gradle does everything on the classpath.)

If running tests on the classpath, but still loading certain Jars on the Module Path is what you are after, then this is a missing feature in Gradle (see https://github.com/gradle/gradle/issues/25954 and other issues). That's why this plugin does a "hack" to put the JavaFX Jars on the Module Path for non-modular applications.

nlisker commented 2 months ago

Eclipse doesn't allow more than 1 module-info per project, so there can't be another one under src/test/java. Looks like what you're describing is the case, but I attached a simple project just to be sure. You can see that while regular implementation dependencies are put on the module path, the implementation files one isn't (and I was wondering why that is the case). If you move the whole "project and external dependencies" to the module path, then this error is resolved, but a new one appears for the tests since they are not in the module-info.

modularJar.zip

jjohannes commented 1 month ago

@nlisker thanks for sharing the example. I had to refresh my memory on the Gradle/Eclipse integration. Yes, you are right that there can only be one module-info in Eclipse. Its a general issue with how the integration works at the moment. Eclipse projects are limited in how much different classpaths you have compared to Gradle projects where you can have arbitrary many "Source Sets" and with that arbitrary many classpath variations. This is a general issue. Not only for Module development. I think there is an issue somewhere (can't find it now) discussing to rethink the integration so that there is one Eclipse project per Source Set - which is how the IntelliJ integration works. (Just for background information.)

You are right that the file(...) dependency is not put on the Module Path for Eclipse, although it is for Gradle. This seems to be a missing festure/bug. I think the problem is here: https://github.com/gradle/gradle/blob/master/platforms/ide/ide/src/main/java/org/gradle/plugins/ide/internal/resolver/IdeDependencySet.java#L243 There, asModule is not taken into account for file-based dependencies, while it is for others. Feel free to open an issue for that on https://github.com/gradle/gradle.

Regarding tests, there is this comment in the code (which I wrote four years ago...)

// Test code is not treated as modules, as Eclipse does not support compiling two modules in one project anyway.
// See also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=520667

// We assume that a test-only dependency is not a module, which corresponds to how Eclipse does test running for modules:
// It patches the main module with the tests and expects test dependencies to be part of the unnamed module (classpath).          
nlisker commented 1 month ago

Created https://github.com/gradle/gradle/issues/29296.