globally enable maven stacktrace processing. #5116
Conversation
There was a problem hiding this comment.
Seems helpful in general.
Beware that whereas maven-execute-plugin normally runs a process using a classpath taken from the project’s configured dependencies, meaning that classes it encounters in stack frames are likely to be known to the IDE and accurately mappable via SourceForBinaryQuery, other mojos will typically be loading classes from various tools with no relation to the project’s paths and thus not linkable unless they are in the Java Platform or just happen to be shared. And even when the class can be found in the project’s classpath, there is no guarantee that it is even remotely the same version, so line numbers might be way off.
| if (project != null) { | ||
| toReturn.add(new JavadocOutputProcessor()); | ||
| toReturn.add(new TestOutputListenerProvider()); | ||
| toReturn.add(new SiteOutputProcessor(project)); | ||
| NbMavenProjectImpl nbprj = project.getLookup().lookup(NbMavenProjectImpl.class); | ||
| toReturn.add(new ExecPluginOutputListenerProvider(nbprj)); |
There was a problem hiding this comment.
At least deprecate the original?
Was about to suggest making one extend the other, to reduce duplication, though the impl is pretty small anyway.
There was a problem hiding this comment.
sure I can do that if you want me to
I didn't deprecate because:
- the class still works perfectly fine
- I usually don't ever deprecate anything unless it can be deprecated for removal and NB doesn't really have a process for removing code (unfortunately), although it happened before e.g. with Schlieman or CND cluster etc
To reuse the code of the method I could put it into an abstract class and let both extend it. The only difference between both would be the goal array.
What would you prefer?
There was a problem hiding this comment.
i deprecated ExecPluginOutputListenerProvider and made the new JavaStacktraceOutputProcessor configurable. It uses global scope by default. Ran a project wide search and there were no usages of the deprecated class.
changed the hierarchy to make it DRY
right. There is actually another pending PR #5091 which fixes a few things there. E.g it adds the JDK to the path and fixes parsing for stack traces with module names in them. I mostly got annoyed by NB not linking traces in tests (ant project did that if i remember correctly). Initially I only wanted to enable it in selected mojos, but I couldn't see a reason to not enable it everywhere. A stacktrace is a stacktrace. It does lose it's classloader information once its logged. The IDE can only resolve those links on a best-effort basis. Links leading into JDK or program code will be correct most of the time, but they don't have to be. The application can make up classes at runtime or load them from outside of the cp. This could be potentially improved in future by letting the user pick a source path if the link can't be resolved or maybe make this hotkey activated (ctrl+click?) in case the user notices the link resolves to the wrong line. |
mbien
force-pushed
the
global-stacktrace-parsing
branch
from
3cae9e3 to
5a46d6d
Compare
stack trace processing is currently only enabled for the exec maven plugin. This PR enables it globally.
Unfortunately this is all public API so I don't think we can simply rename
ExecPluginOutputListenerProviderand call it a day. So I created a copy and replaced the original provider with the global variant.before:
imagine no links and no happy face
(don't worry this is not my pw it is a integration test)
this works best when #5099 is also applied, since it fixed some of the patterns.