editorconfigCheck fails due to incompatible version of antlr4-runtime, take two

climategadgets commented 8 months ago

Original submission: #7

Expected Behavior

./gradlew editorconfigCheck passes, or fails with a syntax related message

Actual Behavior

ANTLR Tool version 4.7 used for code generation does not match the current runtime version 4.13.0
ANTLR Runtime version 4.7 used for parser compilation does not match the current runtime version 4.13.0

To Reproduce

Execute this sequence:

git clone https://github.com/home-climate-control/dz.git
git checkout 63a749891de487f6910769bca237019fcd42877a
cd dz
./gradlew editorconfigCheck

Additional Context

I was tempted to just force resolution, but then looked at what wants the newer version of antlr4-runtime, and that would be the org.springframework.boot and io.quarkus plugins, so I thought it would be reasonable to ask to please bump the dependencies' versions to catch up with those two :)

Run ./gradlew buildEnvironment to see where exactly antlr4-runtime version is changed. Note the (c) in Quarkus subtree - that dependency tree is constrained with enforcePlatform(), trying to change it would be painful.

PS: Hope this helps a bit: https://github.com/ben-manes/gradle-versions-plugin

ppalaga commented 8 months ago

I always had the feeling and was hesitant to believe that Gradle has either none or broken class path isolation for the individual plugins. What you say seems to confirm my deepest fears. It is really the case that an upgrade of a dependency of plugin X can break plugin Y, because Gradle just puts all plugins into a single flat classloader?

climategadgets commented 8 months ago

In a word, yes. Here's just one of many possible entry points into this: https://github.com/gradle/gradle/issues/25616

I was also surprised by the fact that code classpath affects the plugin classpath, but I don't know Gradle well enough to offer any educated judgment.

climategadgets commented 8 months ago

Summary

I don't think the upgrade is avoidable, read on.

Workaround

Obviously, this will only work if nothing else is broken by the antlr4-runtime version adjustment, I'm sure it'll explode elsewhere in an identical manner. Having said that, the coercion below makes the plugin work - but only if there are no dependencies that enforce their versions (notably, Quarkus):

diff --git a/build.gradle.kts b/build.gradle.kts
index f0112c5b..3da9c72e 100644
--- a/build.gradle.kts
+++ b/build.gradle.kts
@@ -69,3 +69,12 @@ subprojects {
         useJUnitPlatform()
     }
 }
+
+configurations.all {
+    resolutionStrategy.eachDependency {
+        if (requested.group == "org.antlr" && requested.name == "antlr4-runtime") {
+            useVersion("4.7")
+            because("https://github.com/ec4j/editorconfig-gradle-plugin/issues/28")
+        }
+    }
+}

Additional Context

Cursory code review of this project, and https://github.com/ec4j/editorconfig-linters, makes me think that antlr4-runtime is only used for linting XML (and derivatives), however, it is invoked even if XML is not present in .editorconfig. I didn't have a chance to dive deeper, but I wonder if it is possible to only instantiate/invoke linters if their respective sections are present.

ppalaga commented 8 months ago

When thinking of it again, perhaps the best way to prevent any ANTLR version clashes would be to shade antlr-runtime inside editorconfig-linters. WDYT?

climategadgets commented 8 months ago

Knee jerk reaction: shaded jars are evil.

Assumption based on cursory code review (please let me know if I'm right): ANTLR is only used for XML linting here (though I see that it can do much more).

If that assumption is correct, then I'd think of replacing this particular XML linter with another that doesn't impose this constraint.

If not, or if that's too heavy of a toll... a few options (neither perfect, for different reasons):

(lowest hanging fruit) upgrade ANTLR and call it a day. Looking at a fresh Spring & Quarkus version distribution, if it is available, might confirm viability for this choice. On a tangent, here is some food for thought, and here is some more, albeit obsolete, though which can be interpolated into today. I'd say this would be a good first choice and a fast fix.
(slightly higher) See how ANTLR understands version compatibility, and make several "release trains" (if they honor semver then you'll just have to create one release per their major version number and possibly minor. Provide compatibility matrix (like this) and let the users choose what they want.
(slightly sideways) Make named releases like SLF4J and some Scala tools, off the top of my head (here is a counter-example), with jar name containing the ANTLR version.
(sky high) Run it from its own classloader with no regard to what Gradle provides. Should work because I don't see this plugin having anything to do with compiling files, it just takes whatever it finds (again, cursory analysis) and applies either checks or transformations. This will be the most seamless solution, but the question is, is it worth the effort?

Either way, it would help to catch that particular exception and emit a detailed log message with the link to the spot you'd want to collect feedback at. Going one step further, you might want to collect the versions of relevant dependencies surrounding the crash, provide that information with the error message, and ask users to include that in a report.

ppalaga commented 8 months ago

Thanks for your thoughts, @climategadgets!

Knee jerk reaction: shaded jars are evil

Often yes, but here, I'd see it as a cheap and elegant workaround.

Assumption based on cursory code review (please let me know if I'm right): ANTLR is only used for XML linting here (though I see that it can do much more).

Yeah, it is only adding/removing whitespace

If that assumption is correct, then I'd think of replacing this particular XML linter with another that doesn't impose this constraint.

SAX and Stax parsers tend to swallow some whitespace. Any other suggestions?

If not, or if that's too heavy of a toll... a few options (neither perfect, for different reasons):

(lowest hanging fruit) upgrade ANTLR and call it a day. Looking at a fresh Spring & Quarkus version distribution, if it is available, might confirm viability for this choice. On a tangent, here is some food for thought, and here is some more, albeit obsolete, though which can be interpolated into today. I'd say this would be a good first choice and a fast fix.

The upgrade would make us loose the Java 8 compatibility which I'd prefer not to do because of Gradle.

(slightly higher) See how ANTLR understands version compatibility, and make several "release trains" (if they honor semver then you'll just have to create one release per their major version number and possibly minor. Provide compatibility matrix (like this) and let the users choose what they want.

I'd like to avoid any additional maintenance overhead.

(slightly sideways) Make named releases like SLF4J and some Scala tools, off the top of my head (here is a counter-example), with jar name containing the ANTLR version.

Same as previous.

(sky high) Run it from its own classloader with no regard to what Gradle provides. Should work because I don't see this plugin having anything to do with compiling files, it just takes whatever it finds (again, cursory analysis) and applies either checks or transformations. This will be the most seamless solution, but the question is, is it worth the effort?

I thought of this one, but shading still sounds easier.

Either way, it would help to catch that particular exception and emit a detailed log message with the link to the spot you'd want to collect feedback at. Going one step further, you might want to collect the versions of relevant dependencies surrounding the crash, provide that information with the error message, and ask users to include that in a report.

climategadgets commented 7 months ago

SAX and Stax parsers tend to swallow some whitespace. Any other suggestions?

Nope, I try to avoid XML for a while now.

(lowest hanging fruit) upgrade ANTLR and call it a day. Looking at a fresh Spring & Quarkus version distribution, if it is available, might confirm viability for this choice. On a tangent, here is some food for thought, and here is some more, albeit obsolete, though which can be interpolated into today. I'd say this would be a good first choice and a fast fix.

The upgrade would make us loose the Java 8 compatibility which I'd prefer not to do because of Gradle.

You're referring to something I'm not aware of. Worst case, the users will always have access to previous releases of your tool which does work with Java 8.

However... for practical purposes, Java 8 is dead... all right, let me soften that, has limited usefulness, as it looks from my standpoint (absolutely limited, but that's the only one I have). I was clinging to JDK 11 for a long time because some of my target platforms didn't support anything newer, but then figured out that quality of life improvements are absolutely worth it to upgrade to JDK 17, so I bit the bullet and dropped platform support (at least for a while). (context: Open Source works only; day job is not being discussed - but I take the same stance there as well especially since a project lifetime is usually slightly shorter than 20+ years like some Open Source applications have).

You'll never know until you do. A way to find out is to ask. Your tool is quite unique and very useful, I would hope that users would be vocal in expressing their disapproval should you do something they don't like :)

(sky high) Run it from its own classloader with no regard to what Gradle provides. Should work because I don't see this plugin having anything to do with compiling files, it just takes whatever it finds (again, cursory analysis) and applies either checks or transformations. This will be the most seamless solution, but the question is, is it worth the effort?

I thought of this one, but shading still sounds easier.

It's your call, all in all :)

ec4j / editorconfig-gradle-plugin