Report "Error occurred during initialization of VM" as error

[kriegaex]

Until now, this error message was just swallowed silently.

Along the way, also report "Error occurred during initialization of boot layer" as ERROR, because probably OTHER was never the right category to begin with. With OTHER, Maven Compiler logs both error messages on INFO, which I believe to be wrong. Now, Maven Compiler logs them on ERROR with "COMPILATION ERROR" header, which fits the behaviour that the build fails.

Besides, javadoc for CompilerMessage.Kind.ERROR says "Problem which prevents the tool's normal completion", which also is a good description of what is actually happening for both the boot layer and VM init errors.

@slawekjaranowski, this can be seen as a follow-up on #337, caused by the conversation in this mailing list thread.

[kriegaex]

Sorry, I have noticed that the current output parsing code is hard to read and understand. So I started to refactor it a bit, hence the new commit I just pushed. I am also planning to restructure and improve a few more details, i.e. the scope of this PR is growing beyond what is written in the current subject and description. When I am done, I will adjust them.

Update: Javadoc was a bit picky about nesting <pre> into <code>. I fixed that by force push, nesting a proper javadoc {@code} inside <pre>. That even gives me inline syntax highlighting in IntelliJ IDEA right when looking at the source code. Nice.

[olamy]

Could it be possible to have separate commits for fixing the issue and cosmetic changes.
Just in case something needs to be reverted this will be easier.

Thanks

[kriegaex]

There are separate commits already, 2 at the moment. Just don't squash them when merging the PR, like someone did last time. I hate people squashing my "separation of concerns" commits into bigger ones. Just imagine someone wants to bisect something or simply look at git blame|annotate output later. I would even have liked to have made the second commit smaller still, but I know that many OSS projects do not like that, for whatever weird reasons they might have.

[olamy]

In this case make separate PRs..
As we use automatic tooling to generate release notes
And if you care about separation of concerns...
That will review easier

[kriegaex]

I do not want to do that. Separation of concerns by commit should be enough. The thing is, that further functional improvements I want to do later in this PR will be based on the restructured source code. I do not wish to end up in rebase and merge hell later, because I have to wait for PR 1 to be merged, before I can move on to PR 2. Please try not to make your problem my problem. I want to help and improve something here.

[kriegaex]

The PR is about improving error parsing in general. After the scope will be clear, we can create an issue and describe it properly. That is what will end up in the release notes. Refactoring does not need to go there. The refactoring by boyscout rule ("leave the camp ground behind a little cleaner than you found it") is just a means to an end: I want to understand the code better myself and also make it easier for people reviewing or maintaining this later. That is easier, if I refactor while fixing bugs or adding features.

[olamy]

PR 1 can be merged in a matter of minutes if ready
if the current PR is still WIP please leave it as draft
I agree and appreciate you are here to help. But maybe try to adapt how the community is working here.

[kriegaex]

Sigh - all this micro-management and extra process steps eating everyone's time without anything getting done...

@olamy, please review the first commit then. The build for that was green. If I drop the second commit and re-introduce it in an extra PR, will you merge the small first commit in a matter of minutes? It was ready two days ago, but nobody did anything about it. So after waiting, I decided to commit on top of it. Then, can you merge the 2nd commit quickly, before I build more improvements on top of that? And what else do you need? Are the PRs enough, or does everything have to be duplicated into GitHub issues, too?

[olamy]

C'mon mate, it's middle of Xmas holidays. Volunteers here may have family life....
I'm just asking to have 2 separate PRs for each that's it. It doesn't look too hard?
It probably took you more time discussing this rather than simply doing it.... instead of all of this ping pong, we would have time to make separate PRs and merge them or at least the big fix one

[kriegaex]

maybe try to adapt how the community is working here.

If I am hot on a topic, I want to stay focused and get work done, before I switch to the next topic. Anything else is inefficient. Two days waiting for feedback on a PR is not long for busy reviewers active as contributors in many other projects. But for me it is half an eternity, because during that time, I could finish 5 PRs, each one sweet and small, but I am dependent on other people reviewing and merging my work, because this is not my project. That means, if I want to keep everything separate, I have to rework stuff after having switched focus to something else entirely, unnecessarily rebasing stuff and solving merge conflicts on files which nobody but me is working on in the meantime anyway, doing most of the work twice, because the code has evolved since then. That is a high price to pay just because this is "how work is done around here".

The main criteria for any PR should be:

• Does is work correctly? (Not: Did the contributor do everything exactly the same way the reviewer would have done it? He can still refactor it later, if he thinks that is necessary.)
• Is it an improvement compared to the previous situation? (Not: Is it perfect?)
• Is test coverage at least not significantly worse than before? (Not: Is it 20% better than before?)

In 90%+ of my code reviews, people comment on anything but the functionality as such. They are concerned about code style, the way I comment or write commit messages, whether commits should be split into smaller commits or multiple PRs in project A or should be squashed into a single commit in project B. Many reviewers do not even understand the code they are reviewing and asking questions they would not have had to ask, if they had actually checked out the code, looked at it in an IDE and ran it. They (falsely) think, a web UI alone is a proper reviewing tool.

That in many cases my PRs sit there for weeks, months or even years, and then instead of doing their job, rebasing it on the latest main branch themselves and solving conflicts, the first thing they ask me is why the code is not rebased yet and when I will do that, as if it was my fault that others let the code rot for so long.

Sorry for the rant, but I want you to see my perspective, too.

[kriegaex]

C'mon mate, it's middle of Xmas holidays. Volunteers here may have family life....

I know that. Where I am currently located, it is 15:48 on Xmas Eve, me being German and Xmas Eve being "the big day" for Germans, not Xmas Day. I wanted to get that off my chest before starting to bake and make dinner for my loved ones. It was my little Xmas gift to this project.

I did not expect anyone to merge this before the holidays or even before the end of the year, but in exchange I would have appreciated some leeway, letting me do my work in this one class (and the two test classes covering it) in a single PR. But no, others are excused for being busy or having a private life. But I need to keep PRs separate and solve merge conflicts later.

I fully agree, this takes much more time than simply reviewing and merging this PR, and then after the holidays you could review my follow-up one. Or you just let me commit on top of it. What is the big deal?

But for me it is worth the time, clarifying cross-cutting concerns I have seen dozens of times in many projects I contributed to, and which I expect to slow me down more in the future, if not resolved.

I fully understand that people are off to their families already, I just want to continue working in between my holiday activities (now waiting for some yeast dough to rise) for my own fun and for the benefit of this project. It would unblock me, if I could just continue without later someone complaining, "oh, this is too much to review, because I would have to try to understand the code".

[olamy]

this took to me maybe less than 30s

olamy@pop-os:~/dev/sources/codehaus-plexus/plexus-compiler$ git checkout -b bug-report-error-vm-initialization
Switched to a new branch 'bug-report-error-vm-initialization'
olamy@pop-os:~/dev/sources/codehaus-plexus/plexus-compiler$ git cherry-pick e7f276272b902a10eb6ebca501aa3640e4bde2c4
[bug-report-error-vm-initialization a74853ce] Report "Error occurred during initialization of VM" as error
 Author: Alexander Kriegisch <[email protected]>
 Date: Fri Dec 22 12:48:18 2023 +0700
 2 files changed, 19 insertions(+), 4 deletions(-)
olamy@pop-os:~/dev/sources/codehaus-plexus/plexus-compiler$ gh pr create 
? Where should we push the 'bug-report-error-vm-initialization' branch? codehaus-plexus/plexus-compiler

Creating pull request for bug-report-error-vm-initialization into master in codehaus-plexus/plexus-compiler

? Title Report "Error occurred during initialization of VM" as error
......
remote: 
remote: 
To https://github.com/codehaus-plexus/plexus-compiler
 * [new branch]        HEAD -> bug-report-error-vm-initialization
Branch 'bug-report-error-vm-initialization' set up to track remote branch 'bug-report-error-vm-initialization' from 'origin'.
https://github.com/codehaus-plexus/plexus-compiler/pull/343

The longest part is to wait for CI to be done.
The other PR is now merged. Feel free to rebase etc.... and enjoy the festive period

[kriegaex]

this took to me maybe less than 30s

That was not the point, and it was also unnecessary, because I had offered to do that already without you reacting to it:

@olamy, please review the first commit then. The build for that was green. If I drop the second commit and re-introduce it in an extra PR, will you merge the small first commit in a matter of minutes?

It is always trivial to just commit the first commit of a multi-commit PR. The problems start, if there are multiple PRs in parallel, possibly merged in arbitrary order, maybe the refactoring forst or the functional change first. The other PRs need to be rebased, if they change the same code, which in this case, working on a single class and two unit test classes covering it, is highly probable. I am quite versed in Git and can manage. The point is, that I think it was unnecessary. But thanks for merging that single commit anyway.

[kriegaex]

Superseded by #343 and #346.