ECJ usage broken in 2.14.x #347
Comments
|
What version of Maven do you use? Similar to #332 |
|
3.9.5. I quickly tried a few others for you: 3.9.4, 3.8.5, 3.6.3 - same result. 4.0.0-alpha-10 seems to work, at least in this empty project. Does that help you in any way? Have you tried to reproduce? It is really super simple, you only need the POM. |
|
You need at least 3.9.6 - https://issues.apache.org/jira/browse/MNG-7913 |
|
Yes, it works on 3.9.6, thank you. Hm, that error message is not so nice, as you can see from the duplicate issue here. If a minor library version upgrade forces a project to upgrade to the latest Maven release, I am expecting a more helpful error message, not just "No such compiler 'eclipse'". Could you please improve that? Imagine thousands of developers around the world getting Dependabot or Renovate suggestions to upgrade, and then there is this error message they do not understand, creating unnecessary work here like I did. BTW, it is really unfortunate to change an important core library in a minor-minor Maven version, just because the old one does not support annotation scanning on JDK 17+, causing a breaking change for all plugins which need same annotation scanning. Of course, it is not Maven's fault that ECJ requires JDK 17+ now, also forcing its fork AJC to upgrade. So, I understand that in order to use ECJ or AspectJ with Plexus Compilers, JDK 17+ (as a build environment, not as a runtime environment for the compiled programs) is necessary, too. But that this forces users to upgrade to Maven 3.9.6 because of the breaking switch to Sisu, even though they previously had compiled their problems without any problems on Java 17 or 21 with Maven 3.9.5 or older, is really counter-intuitive. It would have been nice, if the old library could have been kept alive until a breaking change in Maven 4. Anyway, sorry for straying off topic. A better error message is what I think justifies keeping this issue open. |
|
@kriegaex it is not breaking change in Maven. It's just a consequence of Eclise ECJ requiring Java 17. Feel free to open PR with improvements to maven-compiler-plugin |
Fine, then it simply has stopped working without a helpful error message. I wonder why the default answer to users asking for something so often is "feel free to open a PR", instead of remembering to be a committer and improve something oneself. Am I really asking too much, hoping for a better, more informative error message? |
|
@slachiewicz, @slawekjaranowski, can you please explain to me how The enforcer rule just skips |
|
Eclipse Sisu does classpath scanning for classes annotated with |
|
This problem is not solved even in Maven 3.9.6. Just compile the same POM on a JDK older than 17. Even though Sisu-Inject 0.9.0.M2 contains ASM capable of parsing class files up to Java 21 and depends on Guice 5.1.0, which also contains its own relocated copy of ASM, but this time the highest class file version it knows is JDK 18. Both should be enough to parse (annotations on) Java 17 class files even when running the build on JDK 8, 11 or 16, then populate the compilers list and try to run e.g. Eclipse or AspectJ compilers, then probably failing due to unknown class file versions on the older JDK running the build, which would give users a clear indicator of what it wrong. So, either there is yet another ASM somewhere in the belly of Maven or Maven Compiler, or maybe it is something else like CGLIB or whatever else it uses. |
|
I was able to reproduce the problem locally with a mini Sisu project, compiling the class receiving the injection to target 8 and the This is caused by the ASM class visitor here. Just run the build for my POM on JDK less than 17 with Maven 3.9.6, and you will see what Sisu does: Then inspect the log: So, the problem does not actually seem to be parsing the class with ASM and finding the annotations, but rather Sisu trying to load the class in Is there, to the best of your knowledge, any way to scan the annotations without actually loading the classes? I certainly know that ASM can do that, but it depends on how Sisu uses ASM. |
This is only partly true, it is a consequence of using So even that |
|
Two pieces of new information:
I.e., we are looking at a |
|
Due to the way dependency injection works it is not possible for m-compiler-p to come up with a better error message than |
|
Has someone tried to simply compile the Another way would be do make it a Multi-Release-Jar and have there a |
|
@kwin, of course I was implying that eclipse-sisu/sisu.plexus#51 is implemented, because it is linked to the other issue. BTW, there is always a way to make something work. In this case, downstream projects need to wait for upstream projects to enable a solution, which is as a mere user I was trying to connect the Sisu people and the Maven people to each other, so they can figure out the best solution together. Even today, there would be an ugly way to make it work: Make Sisu Inject do its log output, capture it (optionally tee-ing it to the console or log file, if the user actually sets the debug option) and react to certain things we find there. I am not suggesting to do it this way, but in theory, it would be possible. |
Yes, that is exactly what I tried first locally. It is not helping, because Sisu is not just parsing the class with ASM, but also loading it for whatever reason. But |
The Multi-Release Jar would be an option... |
|
Option 3 would be to refactor both |
How so? Edit: Forget the question. I missed that at first:
Yes, that would work. I am, however, not claiming to be the champion of multi-release JARs. Other than knowing that they exist and how they basically work, I never used them before. It might also require extra tests for both cases. But of course, that would be true for my "option 3" solution, too. Let me think about it. I do hope that one of the committers here solves the problem first, because I am currently dancing on several other weddings already. Otherwise, I might be able to come up with some kind of ugly, but functioning workaround. |
|
Its not that hard, you just need to have two executions of the compiler with different release levels, its just not hat nice in the IDE (usually you have to work on one of them but not both). |
|
Yes, the IDE bit is what always kept me from exploring it more deeply than in theory. AspectJ uses the reflection + method handles approach in situations like that. Of course, that is ugly too in a way, especially because it sort of obfuscates the code. OTOH, that makes the maintainer a demigod to some people. 😂😂😂 |
|
I have a local PoC for the multi-release JAR approach for ECJ. Actually, for AspectJ it is not strictly necessary, because Tomorrow, I will push a draft PR to get some feedback from the committers here. The rest of the evening (in my current time zone), I will be busy. |
AspectJ can compile many projects on JDK 11, because it is not always using Java 17 classes from JDT Core. If it does not work, there will be a runtime error, and the user can figure out what is wrong. For ECJ, the case is more complicated, because it directly imports Java 17 classes from JDT Core. There were isolated in new class EclipseJavaCompilerDelegate, which is accessed using Class::forName and method handles from EclipseJavaCompiler. I.e., Sisu Inject can scan the annotations on EclipseJavaCompiler on JDK 11, and there is no more confusing "No such compiler 'eclipse'" error, but rather: Error injecting constructor, ... EcjFailureException: Failed to run the ecj compiler: ECJ only works on Java 17+ at org.codehaus.plexus.compiler.eclipse.EclipseJavaCompiler.<init> This explicitly tells the user what is wrong. A multi-release JAR solution was tested and worked well in Maven, but was dismissed due to the terrible developer experience in IDEs. Fixes codehaus-plexus#347.
|
I would appreciate feedback to PR #358. |
This shifts the focus from a supposed Maven component wiring error to the root cause, expressed in the error message: "ECJ needs JRE 17+". The new static boolean field EclipseJavaCompiler.isJdkSupported also enables us to write a simple unit test, asserting on the error message, if the field value is false. Relates to codehaus-plexus#347.
This shifts the focus from a supposed Maven component wiring error to the root cause, expressed in the error message: Fatal error compiling: ECJ needs JRE 17+ We need to add safeguards throwing the exception in both methods accessing class EclipseJavaCompilerDelegate, because we cannot predict who calls which one first. Omitting one safeguard might lead to the return of the infamous "No such compiler 'eclipse'" error. The new static boolean field EclipseJavaCompiler.isJdkSupported also enables us to write a simple unit test, asserting on the error message, if the field value is false. Relates to codehaus-plexus#347.
|
Just my 5 cents: maybe a pattern like this could work to produce sane(r) error messages It would be able to differentiate "not present" vs "present but cannot construct it" (due whatever reason). |
|
@cstamas, did you test your change? The code you changed is never reached, according to extra log messages I put there. Therefore, the change does not solve the problem. The error message I see on JDK 11 is still "No such compiler 'eclipse'.". The class also seems to be unused, I can delete it without any consequences. |
|
Hm, I assumed this is used from m-compier-p based on this: |
|
Sorry, I am not a committer to this project, just an indirect user via Maven Compiler (MC). As it turns out, MC depends on Plexus Compiler Manager (PCM), but PCM's own integration tests, which I tried to run and also used as templates for my own projects, do not override the dependency and hence use the version defined in MC. Unfortunately, Now I see: I think, this is something I can take as a point of departure and develop a solution superior to my current PR. Thank you so much for your input. |
If scanning, injection or construction fails, log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
While testing codehaus-plexus#347, changed in compiler manager were not pulled into ITs, because Maven Compiler has a dependency on it, which must be overridden in all ITs or in projects using Plexus Compiler generally, if they need to override the version predefined by Maven Compiler. Co-authored-by: Alexander Kriegisch <[email protected]>
log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
While testing codehaus-plexus#347, changed in compiler manager were not pulled into ITs, because Maven Compiler has a dependency on it, which must be overridden in all ITs or in projects using Plexus Compiler generally, if they need to override the version predefined by Maven Compiler. Co-authored-by: Alexander Kriegisch <[email protected]>
If scanning, injection or construction fails, log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
While testing codehaus-plexus#347, changed in compiler manager were not pulled into ITs, because Maven Compiler has a dependency on it, which must be overridden in all ITs or in projects using Plexus Compiler generally, if they need to override the version predefined by Maven Compiler. Co-authored-by: Alexander Kriegisch <[email protected]>
If scanning, injection or construction fails, log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
While testing codehaus-plexus#347, changed in compiler manager were not pulled into ITs, because Maven Compiler has a dependency on it, which must be overridden in all ITs or in projects using Plexus Compiler generally, if they need to override the version predefined by Maven Compiler. Co-authored-by: Alexander Kriegisch <[email protected]>
If scanning, injection or construction fails, log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
While testing codehaus-plexus#347, changed in compiler manager were not pulled into ITs, because Maven Compiler has a dependency on it, which must be overridden in all ITs or in projects using Plexus Compiler generally, if they need to override the version predefined by Maven Compiler. Co-authored-by: Alexander Kriegisch <[email protected]>
While testing codehaus-plexus#347, changed in compiler manager were not pulled into ITs, because Maven Compiler has a dependency on it, which must be overridden in all ITs or in projects using Plexus Compiler generally, if they need to override the version predefined by Maven Compiler. Co-authored-by: Alexander Kriegisch <[email protected]>
If scanning, injection or construction fails, log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
If scanning, injection or construction fails, log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
While testing codehaus-plexus#347, changes in compiler manager were not pulled into ITs, because Maven Compiler has a dependency on it, which must be overridden in all ITs or in projects using Plexus Compiler generally, if they need to override the version predefined by Maven Compiler. Co-authored-by: Alexander Kriegisch <[email protected]>
If scanning, injection or construction fails, log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
In order to be able to do that at all, I had to add a constructor taking a throwable first. Now, even though a cause is propagated, at the time of writing this Maven Compiler will just catch the NoSuchCompilerException we throw, ignore its message and root cause and throw a new MojoExecutionException instead. :-/ Relates to codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
|
@cstamas I wonder what would happen when one puts the following plugin descriptor(!)) inside the dependency that requires higher java/maven will it be considered if used as a dependency of another plugin? |
While testing codehaus-plexus#347, changes in compiler manager were not pulled into ITs, because Maven Compiler has a dependency on it, which must be overridden in all ITs or in projects using Plexus Compiler generally, if they need to override the version predefined by Maven Compiler.
If scanning, injection or construction fails, log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes codehaus-plexus#347. Co-authored-by: Alexander Kriegisch <[email protected]>
In order to be able to do that at all, I had to add a constructor taking a throwable first. Now, even though a cause is propagated, at the time of writing this Maven Compiler will just catch the NoSuchCompilerException we throw, ignore its message and root cause and throw a new MojoExecutionException instead. :-/ Relates to codehaus-plexus#347.
Add more detail concerning possible user errors like misspelling the compiler ID or missing dependencies for a compiler. Relates to codehaus-plexus#347.
* ITs need plugin dependency to plexus-compiler-manager While testing #347, changes in compiler manager were not pulled into ITs, because Maven Compiler has a dependency on it, which must be overridden in all ITs or in projects using Plexus Compiler generally, if they need to override the version predefined by Maven Compiler. * Lazy providers and better error reporting If scanning, injection or construction fails, log a comprehensive error message on top of throwing a NoSuchCompilerException. Fixes #347. Co-authored-by: Alexander Kriegisch <[email protected]> * Code review: throw exception with cause In order to be able to do that at all, I had to add a constructor taking a throwable first. Now, even though a cause is propagated, at the time of writing this Maven Compiler will just catch the NoSuchCompilerException we throw, ignore its message and root cause and throw a new MojoExecutionException instead. :-/ Relates to #347. * Code review: improve DefaultCompilerManager.ERROR_MESSAGE Add more detail concerning possible user errors like misspelling the compiler ID or missing dependencies for a compiler. Relates to #347. --------- Co-authored-by: Tamas Cservenak <[email protected]>
Create a new project (no sources needed) with this POM:
Running
mvn compile -eyields:The result is the same on 2.14.0, 2.14.1, 2.14.2. With 2.13.0, it works.
The text was updated successfully, but these errors were encountered: