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

Do not automatically apply the gradle-modules-plugin if the project uses a Gradle version >= 6.4. #96

Closed
wants to merge 6 commits into from

Conversation

mrcjkb
Copy link

@mrcjkb mrcjkb commented Mar 11, 2021

See #94.

@abhinayagarwal
Copy link
Collaborator

I missed this PR. Is this still required after #98 has been integrated?

With #98, plugin works with all Gradle versions.

@mrcjkb
Copy link
Author

mrcjkb commented May 3, 2021

I missed this PR. Is this still required after #98 has been integrated?

With #98, plugin works with all Gradle versions.

While the gradle-modules-plugin is compatible with Gradle 7, I don't believe the JavaFX plugin needs to depend on the java-modules-plugin, as Gradle now has full module support (see here and here).

I believe it would be better to allow users to decide themselves if they want to use the java-modules-plugin, as long as the JavaFX plugin doesn't depend on features that would be missing without it.

Since Gradle considers module support stable since version 7 (it doesn't have to be explicitly enabled anymore), it might be better to change the version check in this PR to 7.0. What do you think?

@abhinayagarwal
Copy link
Collaborator

Thanks for your input. I agree that we can avoid depending on java-modules-plugin from Gradle 7.0+. However, in the current state of the PR, disabling java-modules-plugin is a breaking change and users to update their build.gradle.

For example, if I try to run Modular HelloFX sample with Gradle 7.0, mainClassName = "$moduleName/hellofx.HelloFX" entry needs to be updated to:

application {
    mainModule = 'hellofx'
}

mainClassName = "hellofx.HelloFX"

We need to avoid this and let the plugin handle it automatically, such that the same application can run smoothly without user intervention across all Gradle version.

Do you think this PR can be updated to take care of it?

@mrcjkb
Copy link
Author

mrcjkb commented May 3, 2021

So you'd like the JavaFX plugin to inject a moduleName variable, as the gradle-modules-plugin does, for backward compatibility?

There is one issue I have with the automatic moduleName variable injection: It assumes that there is only one module, and injects the module name of the first module-info.java file it finds in the main source set. That is fine for the majority of projects, but could be error-prone for multi-module projects.

Even if this plugin injected the variable itself, there may be projects that depend on the gradle-modularity-plugin in ways we haven't taken into account.

I would suggest one of the following solutions:

  1. Analyse the build file for dependencies on the gradle-modularity-plugin before loading this plugin, and add the modularity plugin if they are present. We could start with just the moduleName variable, and add further checks if needed.
  2. Apply the modularity plugin by default, and add a configuration hook that allows users to prevent this plugin from adding the modularity plugin (for Gradle >= 6.4)

In the case of 2, I would recommend deprecating the dependency on the gradle-modularity-plugin, and issuing a warning message if the moduleName variable is used, but the modularity plugin isn't explicitly applied.

@mrcjkb
Copy link
Author

mrcjkb commented May 22, 2021

@abhinayagarwal I have implemented the second solution: The module plugin can be disabled by setting the useNativeModuleSupport option to true (it is set to false by default). This should ensure backward compatibility.

One change worth noting: The options are now loaded before the modules plugin is applied. The tests don't fail, so it shouldn't have any implications.

@abhinayagarwal
Copy link
Collaborator

@mrcjkb Thank you for the update. I tried the latest changes on the modular HelloFX sample. Below are my observations:

  • useNativeModuleSupport is set to true by default unlike you mentioned in the comment above
  • Setting useNativeModuleSupport to false runs the application without any changes 👍
  • Setting useNativeModuleSupport to true and using Gradle 7.x, the application fails to run because JavaFX dependencies are on the classpath

The same can also be observed with a non-modular HelloFX sample.

In both modular and non-modular application, JavaFX dependencies should always be on the module-path.

@abhinayagarwal abhinayagarwal added the enhancement New feature or request label Jul 31, 2021
@mrcjkb
Copy link
Author

mrcjkb commented Nov 20, 2021

Closing, as this will likely not work without using the Gradle 7.x API.

@mrcjkb mrcjkb closed this Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants