Skip to content

Lazy providers and better error reporting #361

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 21, 2024
Merged

Lazy providers and better error reporting #361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e7282ba
ITs need plugin dependency to plexus-compiler-manager
kriegaex Feb 3, 2024
a3c52c6
Lazy providers and better error reporting
cstamas Feb 2, 2024
ad1164b
Code review: throw exception with cause
kriegaex Feb 3, 2024
90960d1
Code review: improve DefaultCompilerManager.ERROR_MESSAGE
kriegaex Feb 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions plexus-compiler-its/src/main/it/MCOMPILER-346-mre/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-javac</artifactId>
Expand Down
5 changes: 5 additions & 0 deletions plexus-compiler-its/src/main/it/aspectj-compiler/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-aspectj</artifactId>
Expand Down
5 changes: 5 additions & 0 deletions plexus-compiler-its/src/main/it/eclipse-compiler-mapstruct/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-eclipse</artifactId>
Expand Down
5 changes: 5 additions & 0 deletions plexus-compiler-its/src/main/it/eclipse-compiler-procpath/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-eclipse</artifactId>
Expand Down
5 changes: 5 additions & 0 deletions plexus-compiler-its/src/main/it/error-prone-compiler/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-javac-errorprone</artifactId>
Expand Down
7 changes: 6 additions & 1 deletion plexus-compiler-its/src/main/it/missing-warnings/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-javac</artifactId>
Expand All @@ -42,4 +47,4 @@
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<plexus.compiler.version>@pom.version@</plexus.compiler.version>
</properties>
</project>
</project>
5 changes: 5 additions & 0 deletions plexus-compiler-its/src/main/it/simple-eclipse-compiler-fail/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-eclipse</artifactId>
Expand Down
5 changes: 5 additions & 0 deletions plexus-compiler-its/src/main/it/simple-eclipse-compiler/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-eclipse</artifactId>
Expand Down
5 changes: 5 additions & 0 deletions plexus-compiler-its/src/main/it/simple-javac-fork/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-javac</artifactId>
Expand Down
5 changes: 5 additions & 0 deletions plexus-compiler-its/src/main/it/simple-javac/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@
<artifactId>plexus-compiler-api</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-manager</artifactId>
<version>${plexus.compiler.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-javac</artifactId>
Expand Down
30 changes: 26 additions & 4 deletions ...er-manager/src/main/java/org/codehaus/plexus/compiler/manager/DefaultCompilerManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,30 +25,52 @@
*/
import javax.inject.Inject;
import javax.inject.Named;
import javax.inject.Provider;

import java.util.Map;

import org.codehaus.plexus.compiler.Compiler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* @author <a href="mailto:trygvis@inamo.no">Trygve Laugst&oslash;l</a>
*/
@Named
public class DefaultCompilerManager implements CompilerManager {
private static final String ERROR_MESSAGE = "Compiler '{}' could not be instantiated or injected properly. "
+ "If you spelled the compiler ID correctly and all necessary dependencies are on the classpath, "
+ "then next you can try running the build with -Dsisu.debug, looking for exceptions.";
private static final String ERROR_MESSAGE_DETAIL = "TypeNotPresentException caused by UnsupportedClassVersionError "
+ "might indicate, that the compiler needs a more recent Java runtime. "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this rather an indication for an outdated ASM library in sisu which cannot be fixed by the user?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwin as mentioned it is simply unknown at this point, it could be anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this rather an indication for an outdated ASM library in sisu which cannot be fixed by the user?

No. The ASM library can parse Java 17 class files perfectly fine, even on Java 8 or 11. It is the fact that Sisu feels like loading the classes instead of just scanning them, which fails and necessitates a more recent JRE/JDK to run the build. I elaborated about that in #347 and #358 already.

The usage of "might" does not mean that we have no clue about what is happening. There are situations which are much more likely than others, which is why I have mentioned them in the error message details. That is a prudent trade-off, IMO.

The case that Sisu's ASM is too old, is the next part of the error message, right below the one above. Sisu is part of the Maven distribution, necessitating the Maven update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case that Sisu's ASM is too old, is the next part of the error message, right below the one above. Sisu is part of the Maven distribution, necessitating the Maven update.

but again plexus-compiler is NOT maven .. it is used by maven but can be used almost everywhere, so mention any internal maven implementation details is simply wrong here.

Copy link
Contributor Author

@kriegaex kriegaex Feb 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking! According to Maven Central, module plexus-compiler-manager where I put the error message is used only by Maven Compiler and Tycho Compiler, both of which are Maven plugins with the same purpose. I.e., the hint applies to both of them. That in theory, this module can be used elsewhere is just that: theory.

Moreover, I am not mentioning any technical details, when I am saying that the user might need a Maven update when a specific kind of error occurs that the message describes. The descriptions for both error types are just as specific as they need to be to add value for the user to be able to solve the problem. I think, hints like "might indicate, that the compiler needs a more recent Java runtime" and "might mean, that you need to upgrade Maven" are pretty generic.

Copy link
Contributor

@laeubi laeubi Feb 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That in theory, this module can be used elsewhere is just that: theory.

Then it should be part of maven-core or at least maven-whatever, also maven central does not tell anything about other users or future usage, so if there is in practice currently only one consumer that is affected by the problem you try to solve here (Tycho requires Java 17 and latest Maven already) why not add the useful hints at the consumer side than at the generic component side.

Don't get me wrong I think its good to tell the user as much context about an error as possible but this is simply the wrong place and we are already getting enough complaints from user about useless warnings introduced with every maven or dependency update while the one who introduced that always claim its super helpful and only for the best of the users/developers.

I really don't want any highly specific error message to popup suggesting all kind of possible things just because a compiler dependency is missing, there is a typo of the compiler id or the compiler itself is buggy.

For example on maven-plugin side you can even check for the maven version used, so if the maven version currently in use is known to only support Java < 17 and I'm running on a JVM >= 17 and I find a classversion error somewhere in the stack then it makes sense to tell the user to upgrade maven, otherwise it is just a guess not a useful hint and the user will most likely complain that it already is using the latest maven version.

Copy link
Contributor Author

@kriegaex kriegaex Feb 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, Plexus Compilers would easily run on JDK 8 or JDK 11, as I have demonstrated in the first version of this PR. Remember the method handles for the small part of the code actually importing Java 17 files? If I would add this here again, it would be possible to use the plugin on older JDKs again and defer any error messages to runtime instead of wiring time. I still do not understand why some of the modules were just bumped to Java 17 compilation without it being strictly necessary. But this version of the PR so far accepts this and just uses the lazy provider approach to at least create better error messages.

As for where better error handling and reporting should be according to your opinion, I even agree. But I live in reality, not in fantasy land. As I said three times already, our fondest wishes do not make it come true. Even if it would be improved elsewhere in the future, I want this project to play nice with older versions of Maven and Maven Compiler. Therefore - for the last time now, I hope - I am keeping error handling and reporting right here where I can influence it, not elswhere just to keep my hands clean here.

If you think that the error messages or warnings are useless, up to you. I disagree, and I am the author of the PR. You failed to present any convincing arguments for me to change my mind, but I appreciate your opinion and your challenging my own assumptions. That is valuable, but I think it is more than enough now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I live in reality, not in fantasy land.

Claiming everyone that disagree with you lives in fantasy land won't bring things really forward

I am the author of the PR

And you asked for (I'm not sure review? approval? ) from me, but that includes that I might give comment, improvements, suggestions, and here I can only suggest not to add vague error log outputs that are only useful for a small audience even though it might be useful for you and a particular implementation of some compiler implementations. Beside that I think its fine.

Instead I think you should just open another PR for the maven-compiler to fix MCOMPILER-574 as it would bring much more value, for example if any compiler throws an runtime exception or anything else than a class error in the future to fix this configuration problem. The wait for the next release (before that users won't see this change anyways) and all would be fine.

Also please consider the case (what is much more common than running with older maven or older jdks):

  1. compiler id is misspelled
  2. the compiler is not on the classpath

will now advice users

Compiler '{}' could not be instantiated or injected properly. Running the build with -Dsisu.debug, looking for exceptions might help.
TypeNotPresentException caused by UnsupportedClassVersionError might indicate, that the compiler needs a more recent Java runtime.

so we can add another "maybe misspelled" and "maybe missing at all" also why not mention upgrading maven, maybe even using maven4 would help in the future, on the next error some user is facing a problem we add another "maybe" and so on... this simply is not helpful and reading the documentation is much more better than try to hint any possible case.

Copy link
Contributor Author

@kriegaex kriegaex Feb 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I live in reality, not in fantasy land.

Claiming everyone that disagree with you lives in fantasy land won't bring things really forward

I did not claim that everyone lives in fantasy land. I am talking to you here, nobody else. And the reason is not that you disagree, but I explained multiple times already why I said that: The way you suggest to solve the problem is unavaliable, because the other projects do not behave the way you wish them to. I do not rely on wishful thinking, but you do. Hence: fantasy land.

I am the author of the PR

And you asked for (I'm not sure review? approval?) from me

I asked nothing at all of you, not here and not in the original PR. You chimed in of your own accord, and you are totally welcome to do so. I mentioned you in the PR as a professional courtesy, because you also were involved in the previous one. Therefore, like I said several times already, I welcome your feedback, but you cannot force me to agree with you, even thought you are trying hard to.

I can only suggest not to add vague error log outputs

They are not vague. They give clear suggestions what might be wrong and what to try next in each case.

only useful for a small audience

No, they are useful to the majority of users. If they already use the correct combination of build-time JDK version, Maven version configuration, they will never see the error message. Otherwise, if they do see it, I believe than in 90% of all cases it is for one of the two reasons mentioned in the error messages. I never saw any other incident in all the years I used Maven to compile projects, where annotation scanning or DI failed.

Instead I think you should just open another PR for the maven-compiler to fix MCOMPILER-574

Why don't you? But again, even fixing that does not mean that all users upgrade, and their Maven version might still be too old. I am solving a problem other users and I have today.

Also please consider the case (what is much more common than running with older maven or older jdks):

  1. compiler id is misspelled
  2. the compiler is not on the classpath

Those are easy to identify. And in both cases, users will not find the errors mentioned in the Sisu log, i.e. it is clear that they are dealing with another case there. The error message is still OK.

so we can add another "maybe misspelled" and "maybe missing at all"

That is not a bad idea at all, your best suggestion so far. I might add that hint to the error. 👍

also why not mention upgrading maven

But I mention it already. Check again, please.

reading the documentation is much more better than try to hint any possible case.

Trying to mention the most probable cases is sufficient. And the documentation for Plexus Compilers consists of mostly white pages lost inside a Maven site.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we can add another "maybe misspelled" and "maybe missing at all"

That is not a bad idea at all, your best suggestion so far. I might add that hint to the error. 👍

Done, see 90960d1.

+ "IllegalArgumentException in ClassReader.<init> might mean, that you need to upgrade Maven.";

@Inject
private Map<String, Compiler> compilers;
private Map<String, Provider<Compiler>> compilers;

private final Logger log = LoggerFactory.getLogger(getClass());

// ----------------------------------------------------------------------
// CompilerManager Implementation
// ----------------------------------------------------------------------

public Compiler getCompiler(String compilerId) throws NoSuchCompilerException {
Compiler compiler = compilers.get(compilerId);
// Provider<Class> is lazy -> presence of provider means compiler is present, but not yet constructed
Provider<Compiler> compilerProvider = compilers.get(compilerId);

if (compiler == null) {
if (compilerProvider == null) {
// Compiler could not be injected for some reason
log.error(ERROR_MESSAGE + " " + ERROR_MESSAGE_DETAIL, compilerId);
throw new NoSuchCompilerException(compilerId);
}

return compiler;
// Provider exists, but compiler was not created yet
try {
return compilerProvider.get();
} catch (Exception e) {
// DI could not construct compiler
log.error(ERROR_MESSAGE, compilerId);
throw new NoSuchCompilerException(compilerId, e);
}
}
}
5 changes: 4 additions & 1 deletion ...r-manager/src/main/java/org/codehaus/plexus/compiler/manager/NoSuchCompilerException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,11 @@ public class NoSuchCompilerException extends Exception {
private final String compilerId;

public NoSuchCompilerException(String compilerId) {
super("No such compiler '" + compilerId + "'.");
this(compilerId, null);
}

public NoSuchCompilerException(String compilerId, Throwable cause) {
super("No such compiler '" + compilerId + "'", cause);
this.compilerId = compilerId;
}

Expand Down