update samples to use javafx 21, gradle 8.5, gradle-plugin 0.1.0, maven-plugin 0.0.8

abhinayagarwal commented 10 months ago

Also, makes the readme more generic by skipping version information

nlisker commented 7 months ago

You might want to add the bin folder to gitinore.

My findings on Eclipse 4.29:

Eclipse - Gradle - Modular

The code

eclipse {
classpath {
    file {
        whenMerged {
            entries.findAll { it.properties.kind.equals('lib') }.each {
                it.entryAttributes['module'] = 'true'
            }
        }
    }
}
}

Doesn't seem to do anything. Is it useful with more types of dependencies?

If I try to run the project from Eclipse by right-clicking on the main method and Run As Java Application, I get the error
```
Error occurred during initialization of boot layer
java.lang.module.FindException: Module javafx.fxml not found, required by hellofx
```
(with or without the code in point 1). According to https://stackoverflow.com/questions/53295226/eclipse-cannot-find-module-even-the-module-path-is-explicitly-provided, this should have been solved.
When running jlink from Gradle, it gives the warning:

> Task :jlink Warning: The 2 argument for --compress is deprecated and may be removed in a future release

Eclipse - Gradle - Non-Modular

If I try to run the project from Eclipse by right-clicking on the main method and Run As Java Application, I get the error
```
Error: JavaFX runtime components are missing, and are required to run this application
```
I think that that's supposed to happen, and should be fine after adding the VM arguments.

Eclipse - Modular

The .classpath file specifies

<classpathentry kind="con" path="org.eclipse.jdt.USER_LIBRARY/JavaFX11">

The user might not have such a user library defined and it's also not clear what to do with the jmods that were requested to be downloaded in the instructions, This is preventing compilation.

Eclipse - Non-Modular

Works fine, but the .classpath file (https://github.com/openjfx/samples/blob/master/IDE/Eclipse/Non-Modular/Java/hellofx/.classpath#L4) contains old versions of Java and JavaFX.

Eclipse - Maven - Modular

You might want to update the Maven compiler version from 3.8.1 to something newer (maybe in the other maven projects too).
I'm getting a warning that <release> is not a valid configuration for the javafx-maven-plugin plugin.
The package name should be org.openjfx, without hellofx, otherwise it's not the right one in the module-info (or change the package in the module-info).
The .classpath points to JavaSE-11 instead of 21.

Otherwise, runs fine both from Maven and from Eclipse.

Eclipse - Maven - Non-Modular

This project uniquely contains test source files that don't compile because there's no JUnit dependency. As a side note, I think that in general all of the projects should contain tests, and probably the same sources too.

Otherwise runs fine from Maven. From Eclipse it runs fine after adding the VM arguments. Might want to mention that in the instructions.

abhinayagarwal commented 7 months ago

Eclipse - Gradle - Modular

Doesn't seem to do anything. Is it useful with more types of dependencies?

This was added as a part of https://github.com/openjfx/samples/pull/4. @jperedadnr can you check?

this should have been solved

@jperedadnr ? I have also observed that issues with run is fixed by running the gradle task eclipse.

When running jlink from Gradle, it gives the warning

jlink task is not handled by javafx plugin. It is added via org.beryx.jlink plugin. I tried v3.0.1, it gives the same warning.

Eclipse - Gradle - Non-Modular

I think that that's supposed to happen, and should be fine after adding the VM arguments.

Yes. This should also start running once we run gradle task eclipse.

Eclipse - Modular

This is preventing compilation.

Even without this entry the compilation will fail until the JavaFX SDK is not added to the module-path.

Eclipse - Maven - Modular

You might want to update the Maven compiler version from 3.8.1

good idea

The package name should be org.openjfx, without hellofx, otherwise it's not the right one in the module-info (or change the package in the module-info).

Check #76

The .classpath points to JavaSE-11 instead of 21.

Can this be a non-version value?

Eclipse - Maven - Non-Modular

contains test source files that don't compile

will add the test dependencies in a different PR

I think that in general all of the projects should contain tests

good idea

nlisker commented 7 months ago

@jperedadnr ? I have also observed that issues with run is fixed by running the gradle task eclipse.

I found the issue. Running eclipse does allow to run the class from Eclipse because it creates a .classpath file that points to the gradle cache. However, refreshing the gradle project (something that happens whenever the build file is updated) creates a different classpath file that doesn't. There is also the 3rd classpath file, which is the one checked in in the repo. This is outside the scope of this PR, but some Eclipse Gradle tool needs to resolve the conflicting classpath generations I would say.

nlisker commented 7 months ago

Eclipse - Modular

This is preventing compilation.

Even without this entry the compilation will fail until the JavaFX SDK is not added to the module-path.

I don't see instructions to do so. The instructions say to download the jmods, not the sdk (as they do for non-modular projects).

Eclipse - Maven - Modular

The .classpath points to JavaSE-11 instead of 21.

Can this be a non-version value?

This should be auto-generated from the pom. The problem is that the pom is wrong:

<maven.compiler.release>11</maven.compiler.release>

This is used as the java version, not the maven compiler version. It's tied to the <release> warning, which is invalid for the javafx-maven-plugin. There's a big mixup in this pom.

abhinayagarwal commented 7 months ago

This is used as the java version, not the maven compiler version. It's tied to the warning, which is invalid for the javafx-maven-plugin. There's a big mixup in this pom.

It was never tend to be used as the maven-compiler plugin version.

One can define the source, target, release values for compilation in the maven-compiler-plugin configuration or as a property <maven.compiler.____> which is automatically picked up by the plugin.

nlisker commented 7 months ago

One can define the source, target, release values for compilation in the maven-compiler-plugin configuration or as a property <maven.compiler.____> which is automatically picked up by the plugin.

Then why the need for

                <configuration>
                    <release>${maven.compiler.release}</release>
                </configuration>

for maven-compiler-plugin?

In any case, <maven.compiler.release> should be updated for to 21. We shouldn't need to be worried about the classpath file as it is generated by Maven.

abhinayagarwal commented 7 months ago

It's redundant piece of code. Ideally, it should be defined only at one place.

nlisker commented 7 months ago

A lot of the problems in these projects have to do with inconsistencies and the need for manual handling. I assume similar issues exist for the other IDE's and CL. As a future grand plan, I suggest that these projects be assembled automatically from prepared components:

Source sets (main and test) should be the same for all projects
module-info is the same for all modular projects
pom is the same for all Maven projects (as far as I can see)
gradle files are mostly the same for all Gradle projects, differing mostly in plugins. Toolchain configuration should also be added to these to specify the Java version.
IDE files might not need to be supplied for Maven/Gradle projects since they are auto-generated (at least for Eclipse).

Keeping 1 copy of each of these components makes it easier to maintain when versions need to be updated. A script can take them and create the projects with the right combinations.

abhinayagarwal commented 7 months ago

Keeping a common source is definitely a good idea. We will have to brain storm on how can we keep the source of the project and the final projects separate so as to not confuse the users.

nlisker commented 7 months ago

Looks good for the Eclipse projects.

There's still a warning on the pom of Maven Modular:

            <plugin>
                <groupId>org.openjfx</groupId>
                <artifactId>javafx-maven-plugin</artifactId>
                <version>${javafx.maven.plugin.version}</version>
                <configuration>
                    <release>${maven.compiler.release}</release> // warning: Invalid plugin configuration: release
                    <jlinkImageName>hellofx</jlinkImageName>
                    <launcher>launcher</launcher>
                    <mainClass>hellofx/org.openjfx.hellofx.App</mainClass>
                </configuration>
            </plugin>

abhinayagarwal commented 7 months ago

looks like an unwanted configuration. removed it.

nlisker commented 7 months ago

@jperedadnr In my comment https://github.com/openjfx/samples/pull/73#issuecomment-1859318589, point 1 for the Eclipse Modular Gradle project shows some code that I don't see making a difference. Maybe it's needed if there are other types of dependencies? Is it still needed?

jperedadnr commented 7 months ago

@nlisker That was added with this PR https://github.com/openjfx/samples/pull/4 some time ago...

I take that the Eclipse IDE at that moment didn't handle the modular dependencies properly. This StackOverflow post from that time might clarify it better: https://stackoverflow.com/questions/53295226/eclipse-cannot-find-module-even-the-module-path-is-explicitly-provided

It seems that once the IDE issues were fixed, we updated the docs (no more warnings about it), but it might be that the code you are referring was never removed...

It would be good to remove it if it is no longer needed, of course.

nlisker commented 7 months ago

It would be good to remove it if it is no longer needed, of course.

I ran the project both after "Refresh Gradle project" and after running eclipse, with and without that piece of code, and saw no differences (failure before running eclipse, launches after). Maybe you will want to do a quick test, and if you don't find any difference then it can be removed in this PR.

jperedadnr commented 7 months ago

If you have tested it, I'm okay removing it. @abhinayagarwal can you go ahead and remove

eclipse {
    classpath {
        file {
            whenMerged {
                entries.findAll { it.properties.kind.equals('lib') }.each {
                    it.entryAttributes['module'] = 'true'
                }
            }
        }
    }
}

from https://github.com/openjfx/samples/blob/master/IDE/Eclipse/Modular/Gradle/hellofx/build.gradle#L19 ?

abhinayagarwal commented 7 months ago

Done 👍

nlisker commented 7 months ago

This looks good from the Eclipse point of view. Some followups can be:

Using the Gradle toolchain to specify the Java version for Gradle projects.
Updating the instructions on the website for the modular projects with instructions on what to do with the jmod files that are required to be downloaded, and needing the SDKs as well (which are specified only for non-modular projects).
Going over the open issues in this repo and seeing if they are still relevant.

nlisker commented 6 months ago

Anything holding this from being merged?

openjfx / samples