Skip to content
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

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

Merged
merged 11 commits into from
Jan 10, 2024

Conversation

abhinayagarwal
Copy link
Collaborator

Also, makes the readme more generic by skipping version information

@abhinayagarwal abhinayagarwal marked this pull request as ready for review December 17, 2023 08:02
@abhinayagarwal abhinayagarwal changed the title Update gradle plugin and wrapper version Update gradle plugin to 0.1.0 and wrapper assets to 8.5 Dec 17, 2023
@abhinayagarwal abhinayagarwal mentioned this pull request Dec 17, 2023
@abhinayagarwal abhinayagarwal changed the title Update gradle plugin to 0.1.0 and wrapper assets to 8.5 update samples to use javafx 21, gradle 8.5 and gradle-plugin 0.1.0 Dec 17, 2023
@abhinayagarwal abhinayagarwal changed the title update samples to use javafx 21, gradle 8.5 and gradle-plugin 0.1.0 update samples to use javafx 21, gradle 8.5, gradle-plugin 0.1.0 and maven-plugin 0.0.8 Dec 17, 2023
@abhinayagarwal abhinayagarwal changed the title update samples to use javafx 21, gradle 8.5, gradle-plugin 0.1.0 and maven-plugin 0.0.8 update samples to use javafx 21, gradle 8.5, gradle-plugin 0.1.0, maven-plugin 0.0.8 Dec 17, 2023
@nlisker
Copy link

nlisker commented Dec 17, 2023

You might want to add the bin folder to gitinore.

My findings on Eclipse 4.29:

Eclipse - Gradle - Modular

  1. 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?

  1. 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.

  1. 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

  1. 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

  1. You might want to update the Maven compiler version from 3.8.1 to something newer (maybe in the other maven projects too).
  2. I'm getting a warning that <release> is not a valid configuration for the javafx-maven-plugin plugin.
  3. 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).
  4. 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
Copy link
Collaborator Author

Eclipse - Gradle - Modular

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

This was added as a part of #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
Copy link

nlisker commented Dec 18, 2023

@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
Copy link

nlisker commented Dec 18, 2023

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
Copy link
Collaborator Author

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
Copy link

nlisker commented Dec 18, 2023

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
Copy link
Collaborator Author

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

@nlisker
Copy link

nlisker commented Dec 18, 2023

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
Copy link
Collaborator Author

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
Copy link

nlisker commented Dec 19, 2023

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
Copy link
Collaborator Author

looks like an unwanted configuration. removed it.

Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments

CommandLine/Modular/CLI/README.md Show resolved Hide resolved
CommandLine/Modular/CLI/README.md Show resolved Hide resolved
CommandLine/Non-modular/Gradle/hellofx/build.gradle Outdated Show resolved Hide resolved
@nlisker
Copy link

nlisker commented Dec 20, 2023

@jperedadnr In my comment #73 (comment), 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
Copy link
Collaborator

@nlisker That was added with this PR #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
Copy link

nlisker commented Dec 20, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator Author

abhinayagarwal commented Dec 20, 2023

Done 👍

@nlisker
Copy link

nlisker commented Dec 20, 2023

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.

Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@nlisker
Copy link

nlisker commented Jan 10, 2024

Anything holding this from being merged?

@abhinayagarwal abhinayagarwal merged commit 605de7f into openjfx:master Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants