[cmd] parse extends sarif output with extra fields - #4925
Conversation
d96e525 to
8d36e16
Compare
8d36e16 to
e3ec728
Compare
dkrupp
left a comment
dkrupp
left a comment
There was a problem hiding this comment.
The implementation works nicely, Please check my comments related to the default filters.
| self.__log_and_analyze() | ||
|
|
||
| parse_sarif_cmd = [self._codechecker_cmd, "parse", self.report_dir, | ||
| "-e", "sarif", "--review-status", "false_positive"] |
There was a problem hiding this comment.
For sarif export I think the convenient default would be to export everything into sarif and not just the ['confirmed', 'unreviewed'] reports. For standard command line outptut and possible from html, the current default is meaningful (you dont want to see dismissed reports).
Can you please add a filter option "everything" and make that as the default for sarif export?
This way the users would not need to list all review status possibilities at command line invocation if they want to export everything.
There was a problem hiding this comment.
This will be fixed in a followup TR
barnabasdomozi
left a comment
barnabasdomozi
left a comment
There was a problem hiding this comment.
The tests cases were not executed for some reason. Can you try force pushing a commit that would trigger the CI?
| a.o: a.cpp | ||
| $(CXX) -c a.cpp -o /dev/null | ||
|
|
||
| clean: |
There was a problem hiding this comment.
This clean target is pointless. On the line above, the output of the compilation is sent to /dev/null.
|
|
||
| severity = checker_labels.severity( | ||
| checker_name, | ||
| report.analyzer_name) # type: ignore[call-arg] |
There was a problem hiding this comment.
Instead of ignoring type warnings, the current type stub should be updated in file tools/report-converter/codechecker_report_converter/report/checker_labels.py.
| doc_url = doc_url[0] | ||
| rules[checker_name]["helpUri"] = doc_url | ||
|
|
||
| severity = checker_labels.severity( |
There was a problem hiding this comment.
checker_labels.severity() can return UNSPECIFIED, and in that case the _to_level function returns warning. Is that intentional?
| build_json = os.path.join(self.test_workspace, "build.json") | ||
|
|
||
| clean_cmd = ["make", "clean"] | ||
| out = subprocess.check_output(clean_cmd, |
There was a problem hiding this comment.
In this case, why not reuse the existing __run_cmd function?
| if process.returncode != (2 if "parse" in cmd else 0): | ||
| return err | ||
|
|
||
| return ''.join(out) |
There was a problem hiding this comment.
out is already a string, consider:
| return ''.join(out) | |
| return out |
3 participants
There are a few fields in the SARIF specification which would be meaningful to be filled out, but today are not generated into the exported files.
CodeChecker parsealready populates Report objects with review status information, but currently only JSON export makes use of it. This PR addssuppressionobjects to individual results in the SARIF file, according to the spec, including suppression source (source code comment or review config file) and justification message.location.physicalLocationmay be added in a follow-up PR.SARIF
rules[](~checkers) are extended withdefaultConfiguration.levelandhelpUrifields (spec), corresponding to theseverityanddoc_urlchecker labels in CodeChecker config.Note that this only applies to the
CodeChecker parsecommand at the moment, which has the analyzer context (including aCheckerLabelobject) readily available. Doing the same for the standalone report-converter is an orthogonal task, and might entail bigger refactorings.