java9-modularity / gradle-modules-plugin

This Gradle plugin helps working with the Java Platform Module System
https://javamodularity.com/
MIT License
233 stars 36 forks source link

JavaExec internal API spec seems to have changed in Gradle 6.6, making v1.7.0 fail #165

Open gwinstanley opened 4 years ago

gwinstanley commented 4 years ago

Trying to use application plugin with gradle-modules-plugin 1.7.0 and Gradle 6.6 causes a ReflectionException to be raised at ModularJavaExec (line 97). The Gradle internal API appears to have changed so that the line:

var hb = on(this).field("javaExecHandleBuilder").get();

fails to find the specified field (JavaExecAction javaExecHandleBuilder) as it no longer exists in the JavaExec class. It has been replaced by: DefaultJavaExecSpec javaExecSpec (which should, at first look, provide the same support for getExecutable() required by ModularJavaExec).

siordache commented 4 years ago

Thanks for analyzing this issue, @gwinstanley! The problematic line that you mentioned is part of the fix for #65, which can be disabled by adding the following to your build.gradle:

modularity.disableEffectiveArgumentsAdjustment()

However, you can use the above workaround only if your application takes no arguments.

I will provide a pull request shortly.

siordache commented 4 years ago

The fix for issue #65 was a dirty hack. The PR for the current issue makes it even dirtier. I am not happy with it and I would be very grateful to anyone who can suggest a better solution. What we need is a cleaner way to circumvent this Gradle issue.

gwinstanley commented 4 years ago

So I haven't examined this in-depth, and I've never developer a Gradle plugin to-date, so I'm very likely not experienced enough in this arena to comment extensively, but I'm wondering if this section of code is overly complex for what it's aiming to achieve. Given that ModularJavaExec extends JavaExec, doesn't it have access to what it needs already? This patch is using reflection to get a handle to exec-builder, but surely the JavaExec class already does what's required, and just needs modified arguments? Maybe I've missed something here, but something like Gradle's RhinoShellExec (altho clearly much simpler) simply calls super.exec() with modified arguments, instead of this patch which finds/modifies old arguments, then execs via this more complex reflection-based indirection.

If there's a way to selectively modify the arguments and use the normal exec mechanism, it seems the handleBuilder is maybe unnecessary..?

siordache commented 4 years ago

That's exactly the kind of solution I would like to have and I think it should be feasible. It probably requires setting modularity.inferModulePath = true when using Gradle 6.4 or newer versions. I also assume that some of the settings applied by the plugin are no longer necessary (and even harmful) when using a recent version of Gradle. It would be great if someone can send a pull request with a solution along these lines.

In a more general context, the question arises whether it still makes sense to use this plugin now that Gradle has built-in JPMS support. What kind of limitations has the built-in JPMS support? What use cases are not covered?

msgilligan commented 4 years ago

In a more general context, the question arises whether it still makes sense to use this plugin now that Gradle has built-in JPMS support. What kind of limitations has the built-in JPMS support?

It's very limited (especially without the dependency fix-up plugin)

What use cases are not covered?

JavaFX with the Gradle JavaFX plugin is the big use case for me.

gwinstanley commented 4 years ago

What use cases are not covered?

JavaFX with the Gradle JavaFX plugin is the big use case for me.

Agreed; I find JavaFX a strong use-case (also via javafx-gradle-plugin), so until they may choose to forego use of gradle-modules-plugin, we need to accommodate it within build files. That said, this modularity plugin has also been very useful while the Gradle team have taken their time to add JPMS support; it clearly has further to go in terms of feature parity though, so until then we'd all be bloating our build files a bit by reverting to life without the plugin (and moreso if needing JavaFX).

siordache commented 4 years ago

@msgilligan @gwinstanley Thanks for the feedback! So it makes sense to further maintain this plugin. I will wait for a couple of weeks, in the hope that someone sends a pull request that is less hacky than mine. After that, I will make a patch release.

aalmiray commented 4 years ago

I use this plugin in combination with openjfx for the Ikonli build as the JPMS support form Gradle core is abysmal. Unfortunately I have to downgrade to Gradle 6.3 and use version 1.7.0 to have a successful build.

msgilligan commented 4 years ago

I wouldn't call the module support in Gradle 6.4 "abysmal", I would call it "bare bones". It is usable for simple use cases and with the "extra module info" plugin it can transform some libraries to work with Java Modules.

I've been thinking about this lately and I did not give a proper answer when @siordache asked:

What kind of limitations has the built-in JPMS support? What use cases are not covered?

I just answered with the high-level use case "JavaFX with the OpenJFX Gradle Plugin".

I think we should make a more detailed and specific list of features that this plugin provides and support the incorporation of those features into Gradle itself while simultaneously making sure this plugin can work with Gradle's built-in support and by only adding the extra features needed.

It would also be helpful to make the JavaFX Gradle plugin only optionally require this plugin -- because for simple JavaFX apps this plugin shouldn't be needed. (Further the JavaFX Gradle plugin doesn't correctly support libraries see https://github.com/openjfx/javafx-gradle-plugin/issues/85 )

Siedlerchr commented 3 years ago

The workaround modularity.disableEffectiveArgumentsAdjustment() worked with gradle 6.7 and also the openjfx plugin