google-java-format-gradle-plugin icon indicating copy to clipboard operation
google-java-format-gradle-plugin copied to clipboard

Log important JVM errors to Gradle output

Open mchwedczuk-box-com opened this issue 3 years ago • 1 comments

Recently I have been migrating my project to JDK 17. I used the common workaround for GJF 15.x, that is extra --add-exports directives:

org.gradle.jvmargs=--add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED

The problem was that I also had org.gradle.jvmargs entry in my ~/.gradle/gradle.properties so my fix was not working (user settings takes precedence over project settings in Gradle).

The behaviour of the plugin was as follows, all files where marked as INVALID. No error message was provided, neither in Gradle --debug output or in Gradle deamon logs.

Interestingly after removing org.gradle.jvmargs from ~/.gradle/gradle.properties the plugin cached the INVALID statuses in fileStates.txt file and the problem persisted until I executed ./gradlew clean.

Looking at the source code of the plugin I noticed that in few places you are catching Throwable or Error. This is a bad practice. If you want to catch JVM errors like ClassDefNotFound (like I get with the missing exports) it would be good to at least log those errors to Gradle output.

One example of such place: https://github.com/sherter/google-java-format-gradle-plugin/blob/100b46c48b6e81a5761387aa189bb5c566133a3b/subprojects/format/src/main/groovy/com/github/sherter/googlejavaformatgradleplugin/format/OneDotOneFactory.groovy#L40

Please make the plugin more debuggable by not swallowing important JVM errors but instead by logging them as WARNings to Gradle output.

mchwedczuk-box-com avatar Jun 03 '22 13:06 mchwedczuk-box-com

Agree with the author's points--the precedence order is surprising, and the error handling obscures root cause.

jimshowalter avatar Jun 03 '22 15:06 jimshowalter