Lazy providers and better error reporting

[kriegaex]

Fixes #347. Supersedes #358, #359.

@cstamas, @laeubi.

Error messages now rather look like this:

[INFO] --- maven-compiler-plugin:3.12.1:compile (default-compile) @ example ---
[ERROR] Compiler 'eclipse' either was not found (e.g. wrong compiler ID, missing dependencies) or its class could not be scanned. 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. IllegalArgumentException in ClassReader.<init> might mean, that you need to upgrade Maven.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  2.013 s
[INFO] Finished at: 2024-02-03T08:48:40+07:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.12.1:compile (default-compile) on project example: No such compiler 'eclipse'. -> [Help 1]

I.e., we still have "No such compiler 'eclipse'", but also this error message before the build failure (added line breaks):

Compiler 'eclipse' either was not found (e.g. wrong compiler ID, missing dependencies) or its class could not be scanned.
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.
IllegalArgumentException in ClassReader.<init> might mean, that you need to upgrade Maven.

This is a bit wordy, but IMO much better than nothing or too terse. An error message should not just say that and what kind of error occurred, but - if possible - also hint to the user how t get more information or point out poossible known root causes. At the end of the day, that saves support effort.

[laeubi]

I think logging an error from an component with rather technical details of the implementation that throws an exception is not a good design and hard to understand by the usual user.

I think the case where no compiler is found should simply stay as is and the second case should be enhanced to use the caught exception as the cause.

The callers of that method could enhance their log output, to give some hints for example I would print something like this at the caller side:

if (exception.getCause == null) {
    No compiler was found with id ... can be found make sure you have configured your dependency to include a suitable compiler, start maven with -X for more details
} else {
   Loading compiler with id ... failed: exception.getCause, start maven with -X for more details
}

Side note -X should automatically enable -Dsisu.debug I think!

[kriegaex]

I think logging an error from an component with rather technical details of the implementation that throws an exception is not a good design and hard to understand by the usual user.

I disagree, because these are known problems which occur frequently and caused the corresponding GitHub issue and its duplicate to be created in the first place.

I think the case where no compiler is found should simply stay as is and the second case should be enhanced to use the caught exception as the cause.

The existing error message is not helpful without the additional log, because one cannot differentiate why the compiler with that ID was not found. It requires the user to investigate, and I want to provide a few starting points for further analysis. "Help for self-help" is what we call this, literally translated from German.

I am an experienced Maven user and had big problems, figuring out what went wrong. Therefore, I am trying to improve the situation, imagining myself again in the same situation, but with this PR in place. The log output would help me solve the problem.

The callers of that method could enhance their log output

The callers are outside of this project. I want to clean in front of my own door, before I look towards and rely on others.

to give some hints for example I would print something like this at the caller side:

if (exception.getCause == null) {
    No compiler was found with id ... can be found make sure you have configured your dependency to include a suitable compiler, start maven with -X for more details
} else {
   Loading compiler with id ... failed: exception.getCause, start maven with -X for more details
}

That is more cryptic and less helpful, IMO. Therefore, thanks for your feedback, but I will stick with my current version.

Side note -X should automatically enable -Dsisu.debug I think!

Possibly. But it logs much more, and the most probable causes for the problem can be found with this switch without even more "white noise" around it. Let us try to make life for our users easier, not harder, shall we?

[laeubi]

I don't think that this is suitable,a component should not be a debugging facility, so from the point of the API it should just proved as much information as it could (what it currently dont do), just assume I want to write a plugin that has some fallback and I specify I want to use aspectj if not found ecj and in the end fallback to javac... in such case I get two error messages logged even though my plugin can finally find the javac compiler...

So if you think it is helpfull, log the message at debug, but it is not suitable to surface to the error log of any caller.

[kriegaex]

I disagree fundamentally. Software ought to be written with the user in mind, not first and foremost consider the developer's needs. An error message should be as specific as possible. The whole problem at hand occurred in the first place, because previous developers tried to make the error message as short and generic as possible (probably with saving maintenance effort in mind), leaving the user with the burden to figure out what was actually wrong. Even worse, she had to figure out how to figure out what was wrong! From such an incident, developers should learn not to make the same mistake again. It is not bad design to make error messages more verbose, if the verbosity is actually helpful. It is bad design to make it too short and too generic, because such a design is developer-centric rather than user-centric. It does not help the user if the developer likes the error message. As for the log level, we are dealing with a fatal error stopping the build here. Hence, the message goes to ERROR, not to DEBUG.

[laeubi]

I disagree fundamentally. Software ought to be written with the user in mind, not first and foremost consider the developer's needs.

I agree but your proposed solution is developer centric, not user centric, it talks about all kind of stuff users are not aware of (internal implementation details) or wont understand without deeper knowledge of how maven works internally.

An error message should be as specific as possible.

The error message is not specific for example "could not be instantiated or injected" and "looking for exceptions might help" leaves much room for interpretation.

The whole problem at hand occurred in the first place, because previous developers tried to make the error message as short and generic as possible

I think no one has taken into account that java some time will evolve that fast, so its not to blame on the devloper.

if the verbosity is actually helpful.

verbosity is usually archived in maven by enable the -X (extra verbose) flag...

It does not help the user if the developer likes the error message

As said I doubt this will help the user, this is a libary call so the caller must catch and handle the exception, so the only responsibility of the library is to give as much information as possible, in this case it should either trows a different exception or (what I thing suffice here) add the root cause of the exception, then the caller can decide how to best present this to the user (including printing any error output it find sufficient).

As for the log level, we are dealing with a fatal error stopping the build here. Hence, the message goes to ERROR, not to DEBUG.

You can't know if this "stopps the build", this is the the whole concern of the caller,if it does then the caller should give additional instructions if it is fatal or not, so this is simply not the right place, for me throwing an exception and logging an error is simply an anti-pattern and shows that the API design is fatally flawed if it is required to find problems, I don't think it applies here, the lib just need to give further details why the compiler is not found and then the caller can perfectly handle and instruct the user here.

You would even then has seem any exception with simply passing -e to your maven build ...

[kriegaex]

your proposed solution is developer centric, not user centric, it talks about all kind of stuff users are not aware of (internal implementation details)

The plugin user is also a developer, and he wants to know how to identify and fix the root cause of his problem ASAP. Hence, the message is helpful to him.

An error message should be as specific as possible.

The error message is not specific for example "could not be instantiated or injected" and "looking for exceptions might help" leaves much room for interpretation.

I said, "as specific as possible". It cannot be more specific, because the most probable error causes are not the only ones. You also might have noticed that I did not explicitly mention AspectJ or ECJ, because in theory any compiler implementation could be affected in the future. So "might" is exactly right. I thought about how to formulate my sentences and changed them several times, until I was satiafied. Now I am. Sorry you are not.

The whole problem at hand occurred in the first place, because previous developers tried to make the error message as short and generic as possible

I think no one has taken into account that java some time will evolve that fast, so its not to blame on the devloper.

I am not blaming anyone. I said that it was a mistake to make the error too generic and not let the error message evolve together with the changes in the plugin. No problem, I am very relaxed about people making errors, because I am one of them and make errors all the time. But I try to learn something from them and do better next time. Now is next time, so let us not make the same mistake again, just on a slightly higher level.

verbosity is usually archived in maven by enable the -X (extra verbose) flag...

Like I said, this is a fatal error. Verbosity in this case is appropriate.

It does not help the user if the developer likes the error message

As said I doubt this will help the user

And like I said, I disagree, because this exact error message would have helped me when I had the problem. So I am the first counter-example for your doubt.

this is a libary call so the caller must catch and handle the exception

Theory. In practice, it did not happen, and the user saw an error message that was not helpful. I improved that. The PR is a clear improvement of the previous situation, hence it ought to be merged.

so the only responsibility of the library is to give as much information as possible

Which now it does, and you are suggesting to revert that, which I refuse to do.

add the root cause of the exception

The root cause is hidden in Sisu messages that are not escalated to us. The most probable root causes and how to verify them is now part of the error message. That is a clear user benefit.

then the caller

The caller is a DI framework which does not know squat about the specifics of the callee. Like I said, I do within this project what can be done instead of pushing the task to someone else. Relying on the caller to present a better error message to the user has failed expectations in the past, therefore I improved this product the way I did.

As for the log level, we are dealing with a fatal error stopping the build here. Hence, the message goes to ERROR, not to DEBUG.

You can't know if this "stopps the build", this is the the whole concern of the caller,

True. But currently, it stops the build. If that changes in the future, we can adjust the code again. Presently, relying on the caller would do more harm than good. I am, like everyone making my decision based on the situation today with the knowledge I have today. Software is not static, it can be changed in the future.

simply passing -e to your maven build

Before this PR, it simply shows No such compiler 'eclipse' embedded in a stack trace that does not help the user at all. The same is true after this PR. The stack trace is optional, technical information, hence the required -e. But the error message as such needs to be improved, which is what I have done. Arguably, I could embed this long log message to the exception message, if you insist. I thought, it ought to be logged.

[kriegaex]

Some people find a solution for every problem. Other people find a problem in every solution.

[laeubi]

Suggested change

	throw new NoSuchCompilerException(compilerId);
	throw new NoSuchCompilerException(compilerId, e);

[kriegaex]

That was my first thought, too. But that constructor simply does not exist since 2005. Would you like me to add it? I can do that, if you think it makes sense.

[kriegaex]

BTW, that code block is the one with the shorter error message. The longer one is in the block above, where there is no causing exception. I.e., there -e will not yield more information for the user. In order to see the exception from the block we are talking about here, I actually had to add if (true) throw new Exception("uh-oh") during my manual tests.

[kriegaex]

BTW, the thrown cause will be ignored by Maven Compiler:

It just throws its own MojoExecutionException with the same message and compiler ID, i.e. whatever cause or other error message with more details I add, it has zero effect. Of course, we can open an issue in MCOMPILER and they can improve that, we cannot force all users to upgrade the compiler after the issue has been fixed.

        try {
            compiler = compilerManager.getCompiler(compilerId);
        } catch (NoSuchCompilerException e) {
            throw new MojoExecutionException("No such compiler '" + e.getCompilerId() + "'.");
        }

See? This is the perfect example for my previous point: Let us clean in front of our own door and handle the issue as well as we can here, instead of relying on other components in the Maven ecosystem to always do the right thing.

[kriegaex]

@laeubi: In ffa81de, I added an exception constructor taking and passing on a root cause for good measure, hoping that in the future if Maven Compiler handles it differently, it might be a little more useful than today. That does not make the error log obsolete, though.

[laeubi]

See? This is the perfect example for my previous point: Let us clean in front of our own door and handle the issue as well as we can here, instead of relying on other components in the Maven ecosystem to always to the right thing.

No it is not a perfect example, it more shows what I mentioned before, plexus-compiler is an API / library .. you can't know how it is used and there is not only maven.

So to resolve this better for the user one needs two changes:

1. this one to include additional information at minimum the root cause (as done), maybe a different exception message (e.g. compiler with id was found but can't be loaded) so consumer of plexus-compiler-api has everything we can offer here (e.g. that -Dsisu.debug actually works depends on the used implementation of the injection framework!)
2. another one in maven-compiler to include the causing exception in the MojoExecutionException and probably given more logging / instructions to the user how to handle this in the context of maven, for example it would be useful to log all jars the mojo has on its realm so a user can see what might be missing or is there ... and maybe giving a hint to -Dsisu.debug as we know maven is using that under the hood.

and then the logging becomes obsolete as you have already mentioned we can't even know what is wrong we can only see that something is wrong as the why is dependent on the callers context!

[kriegaex]

@laeubi, we agree to disagree then. FYI, I have just created MCOMPILER-574 and mentioned you there. But again, even if that is fixed and your no. 2 has been addressed, you can expect many users to use older Maven Compiler versions for years. Why should they fix something that is, to the best of their knowledge, not broken and works for them? We live in reality, not in an ideal world.

[laeubi]

The same is true after this PR.

I have added in suggestion to the code now, just in case it was unclear what we are talking about here...

Some people find a solution for every problem. Other people find a problem in every solution.

Such "statements" obviously do not contribute to any solution so its better to avoid such generic insulting people you even not know.

I presented you many different solutions, just you find a problem with all of them and insist on the one that you find fit best your needs and claim that everyone will need that, but other will probably have other requirements, one such requirement might be that a library component is not assuming what is best to be logged or teaching the user.

So you can take my suggestion here or leave it alone, I'm not a maintainer of the project so my word doesn't count anyways here so at the end @cstamas or whoever has final word anyways.

[kwin]

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

[laeubi]

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

[kriegaex]

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.

[laeubi]

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.

[kriegaex]

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.

[laeubi]

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.

[kriegaex]

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.

[laeubi]

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.

[kriegaex]

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.

[kriegaex]

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.

[slawekjaranowski]

@kriegaex - thanks for PR and patience with our discussion 😄