Add valgrind parser #738
Add valgrind parser #738
Conversation
|
Thanks for providing this PR! I merged the violations lib PR #735 so you can update your PR with the new pom.xml version. |
It is hard to decide without an example. Do you have an HTML snippet that shows such a description?
I think this is a good starting point. There is currently no strategy, each parser author decides on his own how to map issues to files.
Currently, the description field is used by a few parsers only: these parsers return an HTML string. I think I need to improve the documentation for this field. Currently no escaping is done, so if you have non-HTML content you need to escape on your own by using The HTML string is later processed in the warnings plugin. The whole description is piped through an HTML processor (https://github.com/jenkinsci/antisamy-markup-formatter-plugin) to ensure that the created HTML contains no evil tags.
Do not use the
This was a bug in my configuration.
Actually, the PR checks should be all Green 😄
If the exceptions indicate a problem in the source code of the parser, then they should be thrown so we can fix the parser (fail fast). If the exception may occur more often due to a problem in the tool that writes the reports then we should catch and ignore them. I'll look at the code and comment there.
Normally, it is not required anymore to make any changes in the warnings plugin. A new parser should work out of the box. In order to test your changes you need to wrap your changes into a new https://github.com/jenkinsci/analysis-model-api-plugin Jenkins plugin and deploy it to your Jenkins. When you are using my devenv then the deploy script does that automatically for you. You can then use the |
|
Thanks for the helpful feedback and advice. I think this is sorted out except for the Here's an example of what it's currently generating (with extra newlines and formatting for readability): Here's what that looks like (without warnings-ng CSS):
Primary Stack TraceInvalid write of size 4
Auxiliary Stack TraceAddress 0x4dd0c90 is 0 bytes after a block of size 16 alloc'd
Suppression
|
Codecov Report
@@ Coverage Diff @@
## master #738 +/- ##
============================================
+ Coverage 92.92% 92.94% +0.02%
- Complexity 2338 2368 +30
============================================
Files 345 347 +2
Lines 6499 6592 +93
Branches 672 686 +14
============================================
+ Hits 6039 6127 +88
- Misses 262 263 +1
- Partials 198 202 +4
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I finally got this to work end-to-end through warnings-ng after manually updating analysis-model-api's pom.xml to use the analysis-model SNAPSHOT (you might consider noting this probably obvious required step in your guide somewhere to keep idiots like me from staring open mouthed at it not working for an embarrassingly long time) using the analysisModelId trick you pointed me at. I moved the big blob of HTML into the message from the description because I couldn't find the description anywhere on the warnings-ng UI. It looks like this horrible mess: I obviously have no UI skills, so advice on how to make this look less ridiculous is welcome. Thoughts? |
|
Yes, I need to update the documentation and my development environment (https://github.com/uhafner/warnings-ng-plugin-devenv) description. Since I switched to incremental builds the procedure has been changed and now the version is not a SNAPSHOT version anymore. (That helps to use Dependabot creating automatic PRs when a new release appears.). Currently the description is used by a few parsers only (and there the description is obtained from external files). But normally only the message is wrapped into the I think I need to install your PR in my local instance and play around with it a little bit. I'm also not a UI designer but during the development of the plugin I improved my skills 😁 Are there other tools that produce nice Valgrind reports where we can get an inspiration? |
|
I just installed it and started with some experiments. I think it does not make sense to use fonts larger than the content. I activated some styling using the predefined Bootstrap CSS table styling but I am not sure if that is enough. Maybe we need to provide some new CSS classes for such use cases.
|
|
You can use basically anything from https://getbootstrap.com/ if that helps? (Collapsible sections, etc.) |
|
Hello @bitrunner, |
|
Maybe we can use the current styling in improve it later in another PR? I haven't yet looked at the code because there where these open UI design questions but I can add a review this weekend if it makes sense. |
I have almost zero knowledge about UI development. I can't be of much help to you. But as a potential user of this parser, I would say yes, let merge it as it is, and improve the design in a different PR. |
|
It seems like it might be time to finally finish this up. I like your bootstrap suggestions, style hints, and think your example screenshot is plenty good enough @uhafner. I tried to add some CSS to make it look nicer, but all the CSS seems to get stripped from the message and description fields so no CSS classes can be applied. Were you able to get some through to make that screenshot or were you just adding the styling in the browser? I can make it look nice by editing stuff in my browser, but I can't seem to get enough wizardry through the plugin to make it look reasonable. |
bitrunner
force-pushed
the
add-valgrind-parser
branch
from
832d4a5 to
d5a4095
Compare
|
Good to see that you have some time to finish it. I also have some time to help right now! Actually I forgot whether I changed the HTML output in the browser or in the code 🙈 You can use HTML in the |
bitrunner
force-pushed
the
add-valgrind-parser
branch
from
d5a4095 to
65626d6
Compare
|
It doesn't seem to work in the description either. Here's the best I've been able to make it look without any CSS. I rebased and pushed the latest formatting stuff so what's currently in the pull request should make output like the screenshot. |
bitrunner
force-pushed
the
add-valgrind-parser
branch
from
65626d6 to
c2f4b86
Compare
|
Sorry, seems that I forced pushed to your repository :-( Can you please force push your chagnes again? I wasn't aware that my branch is not set to track my repository. Sorry about that... |
bitrunner
force-pushed
the
add-valgrind-parser
branch
from
ab5371a to
c2f4b86
Compare
|
No problem. Done. Feel free to add whatever you like. |
|
Actually I just wanted to see the results in my Jenkins instance. From your code it also looks that HTML Is working in the description, doesn't it? |
|
Currently, your headers are smaller than the text, maybe you need to tweak these a little bit. |
|
BTW: in Java it might make more sense to use j2html to generate the HTML output (and not a Example: |
|
If you use j2html, be aware that its TagCreator.join method "removes spaces before periods and commas", which may be an undesirable change on text coming from a tool such as valgrind. |
|
The HTML tags I've tried seem to go through fine, but the CSS is stripped. For example, trying to add striping to the tables like this results in a bare table tag (the class="..." part is stripped) in the output HTML: <table class="table table-striped">As for j2html, if you'd prefer it be done that way, I'll switch to it. I'm too clueless to have a preference. @KalleOlaviNiemitalo's comment causes some concern but I can't think of anything specific potentially losing some spaces would mess up that HTML doesn't already do anyway. |
|
Another problem I noticed is that the suppression part of the Violation's specifics is getting lost somehow. It works in the ValgrindAdapter unit test, but Violation.getSpecifics().get("suppression") always returns null when running in Jenkins. Some debugging indicates there does not appear to ever be a suppression key/value pair in the specifics Map. The stack traces are also stored in the specifics Map and they are working fine. Processing the same valgrind output XML in the unit test and on Jenkins shows the suppression part in the unit test but not on Jenkins. The suppression uses CDATA in XML and has \n newlines, curly braces {}, and angle brackets <> in its text so it might have something funky that is making something upset. I don't see anything of interest in the jenkins log. Any idea how this might be possible? |
|
@bitrunner the j2html behaviour did previously cause JENKINS-64051. I don't remember whether valgrind output can include any application-specified text that j2html could corrupt in a similar fashion. |
|
I updated this to use j2html and thanks to @KalleOlaviNiemitalo, avoided problems like JENKINS-64051 by borrowing @uhafner's strategy. In the process, I changed from h5 to h4 to make the headers bigger than the text per @uhafner's note above. I also managed to trick the very strict static analysis into accepting some methods that return null using a somewhat ridiculous obfuscation. null returns appears to be a common idiom in j2html to handle optional things. If my trickery is not acceptable, I'll refactor yet again to find some other middle ground between j2html and the static analysis rules or just go back to StringBuilder where null returns aren't helpful. I also put the CSS in to show the problem of it being stripped. It seems like this might be good enough as is although it has not-so-pretty HTML output and missing suppressions which doesn't seem critical. Shall we squash and merge or pursue further enhancements? |
You are right, the https://github.com/jenkinsci/antisamy-markup-formatter-plugin actually removes potentially unsafe HTML tags and classes. I have no idea why classes are considered a security problem. |
The only thing what you need to be aware: Jenkins might read those values (you will seem them in the serialized XML reports in Jenkins build folder) but will not render them due to the OWASP markup formatter. All tags will be checked against a whitelist to see whether only safe elements are contained. |
| issue -> { | ||
| final String description = issue.getDescription(); | ||
| if (Violation.NO_FILE.equals(issue.getFileName())) { | ||
| assertThat(!description.contains("Primary Stack Trace")); |
There was a problem hiding this comment.
AssertJ does not work in this way. You do not verify anything!
| assertThat(!description.contains("Primary Stack Trace")); | |
| softly.assertThat(description).doesNotContain("Primary Stack Trace"); |
| assertThat(description.contains("Primary Stack Trace")); | ||
| assertThat(description.contains("<insert_a_suppression_name_here>")); |
There was a problem hiding this comment.
| assertThat(description.contains("Primary Stack Trace")); | |
| assertThat(description.contains("<insert_a_suppression_name_here>")); | |
| softly.assertThat(description).contains("Primary Stack Trace", | |
| "<insert_a_suppression_name_here>"); |
| /** | ||
| * A descriptor for Valgrind. | ||
| */ | ||
| public class ValgrindDescriptor extends ParserDescriptor { |
| ).render(); | ||
| } | ||
|
|
||
| private Tag generateGeneralTableHtml(final String executable, final String uniqueId, @CheckForNull final String threadId, @CheckForNull final String threadName, @CheckForNull final JSONArray auxWhats) { |
There was a problem hiding this comment.
| private Tag generateGeneralTableHtml(final String executable, final String uniqueId, @CheckForNull final String threadId, @CheckForNull final String threadName, @CheckForNull final JSONArray auxWhats) { | |
| private ContainerTag generateGeneralTableHtml(final String executable, final String uniqueId, @CheckForNull final String threadId, @CheckForNull final String threadName, @CheckForNull final JSONArray auxWhats) { |
| return generalTable; | ||
| } | ||
|
|
||
| private Tag maybeGenerateStackTracesHtml(@CheckForNull final String stacksJson, final String message, @CheckForNull final JSONArray auxWhats) { |
There was a problem hiding this comment.
| private Tag maybeGenerateStackTracesHtml(@CheckForNull final String stacksJson, final String message, @CheckForNull final JSONArray auxWhats) { | |
| private ContainerTag maybeGenerateStackTracesHtml(@CheckForNull final String stacksJson, final String message, @CheckForNull final JSONArray auxWhats) { |
|
|
||
| private Tag maybeGenerateStackTracesHtml(@CheckForNull final String stacksJson, final String message, @CheckForNull final JSONArray auxWhats) { | ||
| if (StringUtils.isBlank(stacksJson)) { | ||
| return iff(false, null); |
There was a problem hiding this comment.
If there is no null object (empty container) simply return null. If required use a SuppressWarnings directive.
| return iff(false, null); | |
| return null; |
| return stackTraces; | ||
| } | ||
|
|
||
| return iff(false, null); |
There was a problem hiding this comment.
| return iff(false, null); | |
| return null; |
| return iff(false, null); | ||
| } | ||
|
|
||
| private Tag generateStackTraceHtml(final String title, @CheckForNull final String message, final JSONArray frames) { |
There was a problem hiding this comment.
| private Tag generateStackTraceHtml(final String title, @CheckForNull final String message, final JSONArray frames) { | |
| private ContainerTag generateStackTraceHtml(final String title, @CheckForNull final String message, final JSONArray frames) { |
| return stackTraceContainer; | ||
| } | ||
|
|
||
| private Tag generateStackFrameHtml(final JSONObject frame) { |
There was a problem hiding this comment.
| private Tag generateStackFrameHtml(final JSONObject frame) { | |
| private ContainerTag generateStackFrameHtml(final JSONObject frame) { |
There was a problem hiding this comment.
All other usages of raw type Tag as well
| private Tag generateStackFrameHtml(final JSONObject frame) { | ||
| return | ||
| table( | ||
| attrs(".table.table-striped"), |
There was a problem hiding this comment.
I think we can remove the classes since they are stripped away
This is not possible yet (and I don't know if its worth the effort, currently we simply filter everything). This would also imply that we need to sanitize the HTML already here.
Do you have an example in the unit test? Then I can try to debug locally. |
Yes. 4 of the 5 test errors in the test valgrind.xml have suppressions. In valgrind.xml, the suppression is stored in CDATA sections within the The While running in Jenkins,
Sorry, I misunderstood what you wanted me to do. I think I manged to do it right in the latest push. |
I found the problem. In my unit tests the default XML input stream parser of Xerces |
|
I think we can postpone the problem with the parser and create a new release right now. |
This is the beginning of a valgrind report parser. It is a work-in-progress based on the recent addition of a valgrind parser to violations-lib so it depends upon #735 and will fail CI until that is merged because I didn't want to conflict the pom.xml changes for that here. The eventual goal of this pull request is to provide valgrind support in warnings-ng as a replacement for the broken, vulnerable, and unmaintained jenkins valgrind-plugin.
There are a few known problems/open questions with this that I could use some advice/help with to move this forward. I'm willing to work this through whatever needs to be done to achieve the goal of providing a warnings-ng alternative to the valgrind-plugin.
descriptionfield of issues as HTML tables similar to how they were reported by the existing valgrind-plugin. I haven't been able to wire this up successfully through warnings-ng to see what this looks like on the warnings-ng Jenkins UI, so this might be a terrible strategy or otherwise need to be revised to fit existing design paradigms.<, that needs to be escaped. A simple String.replace("<","<") would suffice, but is there a more robust way to escape such strings that may contain special HTML characters? Will this escaping be handled in warnings-ng or further downstream of analysis-model for thedescriptionfield?originandcategoryfields are set to "valgrind:[tool]" where [tool] is the actual valgrind tool that detected the problem (ex. memcheck). It seems important to identify these problems as coming from valgrind and its subtool, but I couldn't find a reference elsewhere in the code for similar handling so this may need some refinement.convertToReportfunction of theValgrindAdapterin analysis-model. I resorted to catching and ignoring them because an alternative was unclear to me. Is there a better way to deal with that?