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

Overload javafx:run goal to work with JavaFX 17 #136

Merged
merged 9 commits into from
Sep 14, 2021

Conversation

abhinayagarwal
Copy link
Collaborator

This is a hack to make sure that Maven application don't fail with JavaFX 17.

Since there is no way to update the dependency list after its creation in MavenProject, we had to resort for the creation of a new Mojo: JavaFXRunFixMojo which will be called for javafx:run. This new Mojo will then create a new pom.xml which will add classifiers to the JavaFX dependencies and exclude all JavaFX transitive dependencies.

For example, a dependency for javafx-controls on linux, will be available as the following in the new modified pom:

<dependency>
  <groupId>org.openjfx</groupId>
  <artifactId>javafx-controls</artifactId>
  <version>17</version>
  <classifier>linux</classifier>
  <exclusions>
    <exclusion>
      <artifactId>*</artifactId>
      <groupId>org.openjfx</groupId>
    </exclusion>
  </exclusions>
</dependency>

We will then call the existing, JavaFXRunMojo with this new pom.

Since 2 Mojo's cannot have the same name, the name for JavaFXRunMojo has been updated to runx.

This has been tested against the following Applications:

Modular

Non-Modular

@@ -44,7 +44,7 @@
import static org.openjfx.model.RuntimePathOption.CLASSPATH;
import static org.openjfx.model.RuntimePathOption.MODULEPATH;

@Mojo(name = "run", requiresDependencyResolution = ResolutionScope.RUNTIME)
@Mojo(name = "runx", requiresDependencyResolution = ResolutionScope.RUNTIME)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a TODO to restore the name, once fix is not longer required?

src/main/java/org/openjfx/model/JavaFXModule.java Outdated Show resolved Hide resolved
@@ -55,6 +56,7 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

@Ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test works with fine, but needs runx:

JavaFXRunMojo mojo = (JavaFXRunMojo) lookupMojo("runx", testPom);

src/main/java/org/openjfx/JavaFXRunMojo.java Outdated Show resolved Hide resolved
src/main/java/org/openjfx/JavaFXRunFixMojo.java Outdated Show resolved Hide resolved
@betanzos
Copy link
Contributor

Great work @abhinayagarwal!

However I have found 2 issues that I comment below.

1. Custom settings.xml file definition is ignored

If we need to use a settings.xml file other than the default one via command line (by using the -s param), configuration defined by this are ignored and the goal can ends with error.

For example, I have some settings.xml files which define server entries for my org private artifacts. If I try to use any of this settings files those private artifacts are not downloaded because credentials are not found.

2. JAVA_HOME required from IntelliJ IDEA

When running the goal from IntelliJ IDEA the goal fails if the JAVA_HOME env variable is not defined.

[INFO] --- javafx-maven-plugin:0.0.7-SNAPSHOT:run (default-cli) @ hellofx ---
The JAVA_HOME environment variable is not defined correctly 
This environment variable is needed to run this program 
NB: JAVA_HOME should point to a JDK not a JRE 

IMPORTANT NOTES
None of these problems are present...

  1. in the version 0.0.6
  2. if we using the goal javafx:runx and JavaFX 16

@abhinayagarwal
Copy link
Collaborator Author

@betanzos I have pushed a fix for the settings file.

For the IntelliJ IDEA issue, I cannot reproduce the issue. If I use the platform Maven in IntelliJ IDEA, everything seems to work fine. However, if I use the bundled Maven, I am hit by https://youtrack.jetbrains.com/issue/IDEA-139236. Once I provide executable permission to mvn, everything works fine.

Copy link
Contributor

@johanvos johanvos left a comment

Choose a reason for hiding this comment

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

works for me

@johanvos
Copy link
Contributor

@betanzos I have pushed a fix for the settings file.

For the IntelliJ IDEA issue, I cannot reproduce the issue. If I use the platform Maven in IntelliJ IDEA, everything seems to work fine. However, if I use the bundled Maven, I am hit by https://youtrack.jetbrains.com/issue/IDEA-139236. Once I provide executable permission to mvn, everything works fine.

In case it turns out to be an issue, we can create a follow-up issue. For now, this PR is good to be merged.

@abhinayagarwal abhinayagarwal merged commit 88781ed into openjfx:master Sep 14, 2021
@betanzos
Copy link
Contributor

@betanzos I have pushed a fix for the settings file.

@abhinayagarwal this is fixed!

For the IntelliJ IDEA issue, I cannot reproduce the issue. If I use the platform Maven in IntelliJ IDEA, everything seems to work fine. However, if I use the bundled Maven, I am hit by https://youtrack.jetbrains.com/issue/IDEA-139236. Once I provide executable permission to mvn, everything works fine.

Using both an external and bundled Maven I get the same error. This only happens when I don't have defined the JAVA_HOME env variable. Version 0.0.6 and javafx:dorun goal work fine, so this problem was introduced by the new javafx:run goal.

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.

4 participants