-
Notifications
You must be signed in to change notification settings - Fork 365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split compiler sensor into GCC and MSVC compiler to use it at the same time #1533
Conversation
MatchResult matchres; | ||
while (scanner.findWithinHorizon(p, 0) != null) { | ||
matchres = scanner.match(); | ||
String filename = matchres.group(1).trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case group not available could be null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since findWithinHorizon() is used as pre-requirement for match(), the MatchResult contains the proper results. Maybe we could assert the number of groups at runtime (matchres.groupCount() >=5). However it's sufficient to double-check the corresponding expression IMHO
public static final String REPORT_PATH_KEY = "gcc.reportPath"; | ||
public static final String REPORT_REGEX_DEF = "gcc.regex"; | ||
public static final String REPORT_CHARSET_DEF = "gcc.charset"; | ||
public static final String DEFAULT_CHARSET_DEF = "UTF-8"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is default charset for GCC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO UTF-8 is de-facto standard for all OSs and can be used as default charset for each input file.
public static final String REPORT_CHARSET_DEF = "gcc.charset"; | ||
public static final String DEFAULT_CHARSET_DEF = "UTF-8"; | ||
// search for single line with compiler warning message - order for groups: 1=file, 2=line, 3=message, 4=id | ||
public static final String DEFAULT_REGEX_DEF = "^(.*):([0-9]+):[0-9]+:\\x20warning:\\x20(.*)\\x20\\[(.*)\\]$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this latest regex for GCC?
@@ -96,7 +67,7 @@ public void processReport(final SensorContext context, File report, String chars | |||
|
|||
private static String removeMPPrefix(String fpath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this MP prefix removal as an extra step? Could be part of global regex?
public static final String REPORT_PATH_KEY = "vc.reportPath"; | ||
public static final String REPORT_REGEX_DEF = "vc.regex"; | ||
public static final String REPORT_CHARSET_DEF = "vc.charset"; | ||
public static final String DEFAULT_CHARSET_DEF = "UTF-8"; // use "UTF-16" for VS2010 build log or TFS Team build log file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the latest charset for VS2017?
public static final String REPORT_CHARSET_DEF = "vc.charset"; | ||
public static final String DEFAULT_CHARSET_DEF = "UTF-8"; // use "UTF-16" for VS2010 build log or TFS Team build log file | ||
// search for single line with compiler warning message VS2008 - order for groups: 1=file, 2=line, 3=ID, 4=message | ||
public static final String DEFAULT_REGEX_DEF = "^(.*)\\((\\d+)\\)\\x20:\\x20warning\\x20(C\\d+):(.*)$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the regex for VS2017?
@@ -52,7 +52,8 @@ private CxxMetricsFactory() { | |||
// Introduce metric keys for sensors - number of issues per file / per module | |||
CLANG_SA_SENSOR_ISSUES_KEY("ClangSA"), | |||
CLANG_TIDY_SENSOR_ISSUES_KEY("Clang-Tidy"), | |||
COMPILER_SENSOR_ISSUES_KEY("Compiler"), | |||
VC_SENSOR_ISSUES_KEY("Compiler-VC"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which metric keys should we use?
Compiler-XXX, or only XXX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler-XXX look more consistent to me
@@ -104,7 +105,8 @@ public String toString() { | |||
|
|||
addSensorMetric(Key.CLANG_SA_SENSOR_ISSUES_KEY, "ClangSA issues", domain, langPropertiesKey, metrics); | |||
addSensorMetric(Key.CLANG_TIDY_SENSOR_ISSUES_KEY, "ClangTidy issues", domain, langPropertiesKey, metrics); | |||
addSensorMetric(Key.COMPILER_SENSOR_ISSUES_KEY, "Compiler issues", domain, langPropertiesKey, metrics); | |||
addSensorMetric(Key.VC_SENSOR_ISSUES_KEY, "VC compiler issues", domain, langPropertiesKey, metrics); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here?
VC compiler issues
-or- only
VC issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VC compiler issues IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guwirth the following diff is mandatory. Should I push it as a small fix right-away? or will you include it in your PR? In any way the bug should be fixed before the next release. Thank you.
diff --git a/cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerGccSensor.java b/cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerGccSensor.java
index 8315dca..dfbbf0c 100644
--- a/cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerGccSensor.java
+++ b/cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerGccSensor.java
@@ -47,6 +47,6 @@ public class CxxCompilerGccSensor extends CxxCompilerSensor {
@Override
public void describe(SensorDescriptor descriptor) {
descriptor.name(getLanguage().getName() + " CxxCompilerGccSensor").onlyOnLanguage(getLanguage().getKey())
- .createIssuesForRuleRepositories(getReportPathKey()).onlyWhenConfiguration(new IsGccParserConfigured());
+ .createIssuesForRuleRepository(getRuleRepositoryKey()).onlyWhenConfiguration(new IsGccParserConfigured());
}
}
diff --git a/cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerVcSensor.java b/cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerVcSensor.java
index a5ff161..df88ce4 100644
--- a/cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerVcSensor.java
+++ b/cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerVcSensor.java
@@ -50,6 +50,6 @@ public class CxxCompilerVcSensor extends CxxCompilerSensor {
@Override
public void describe(SensorDescriptor descriptor) {
descriptor.name(getLanguage().getName() + " CxxCompilerVcSensor").onlyOnLanguage(getLanguage().getKey())
- .createIssuesForRuleRepositories(getReportPathKey()).onlyWhenConfiguration(new IsVcParserConfigured());
+ .createIssuesForRuleRepository(getRuleRepositoryKey()).onlyWhenConfiguration(new IsVcParserConfigured());
}
}
try { | ||
parser.processReport(context, report, reportCharset, reportRegEx, warnings); | ||
for (CompilerParser.Warning w : warnings) { | ||
if (isInputValid(w)) { | ||
CxxReportIssue issue = new CxxReportIssue(w.id, w.filename, w.line, w.msg); | ||
saveUniqueViolation(context, issue); | ||
} else { | ||
LOG.warn("C-Compiler warning: '{}''{}'", w.id, w.msg); | ||
LOG.warn("Invalid compiler warning: '{}''{}'", w.id, w.msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrecognized compiler warning?
MatchResult matchres; | ||
while (scanner.findWithinHorizon(p, 0) != null) { | ||
matchres = scanner.match(); | ||
String filename = matchres.group(1).trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since findWithinHorizon() is used as pre-requirement for match(), the MatchResult contains the proper results. Maybe we could assert the number of groups at runtime (matchres.groupCount() >=5). However it's sufficient to double-check the corresponding expression IMHO
try (Scanner scanner = new Scanner(report, charset)) { | ||
Pattern p = Pattern.compile(reportRegEx, Pattern.MULTILINE); | ||
LOG.debug("Using pattern : '{}'", p); | ||
MatchResult matchres; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move MatchResult matchres into the loop
public static final String REPORT_PATH_KEY = "gcc.reportPath"; | ||
public static final String REPORT_REGEX_DEF = "gcc.regex"; | ||
public static final String REPORT_CHARSET_DEF = "gcc.charset"; | ||
public static final String DEFAULT_CHARSET_DEF = "UTF-8"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO UTF-8 is de-facto standard for all OSs and can be used as default charset for each input file.
public static final String DEFAULT_CHARSET_DEF = "UTF-8"; | ||
// search for single line with compiler warning message - order for groups: 1=file, 2=line, 3=message, 4=id | ||
public static final String DEFAULT_REGEX_DEF = "^(.*):([0-9]+):[0-9]+:\\x20warning:\\x20(.*)\\x20\\[(.*)\\]$"; | ||
// ToDo: as long as java 7 API is not used the support of named groups for regular expression is not possible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently we use java8 and named regexp groups are supported since java7
so we can rewrite the comment as // TODO use named groups
descriptor | ||
.name(getLanguage().getName() + " CxxCompilerGccSensor") | ||
.onlyOnLanguage(getLanguage().getKey()) | ||
.createIssuesForRuleRepositories(getReportPathKey()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERROR: please use getRuleRepositoryKey()
(embarrassingly I've introduced it by myself)
descriptor | ||
.name(getLanguage().getName() + " CxxCompilerVcSensor") | ||
.onlyOnLanguage(getLanguage().getKey()) | ||
.createIssuesForRuleRepositories(getReportPathKey()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERROR: getReportPathKey()
-> getRuleRepositoryKey()
import org.sonar.cxx.sensors.compiler.CxxCompilerSensor; | ||
import org.sonar.cxx.sensors.utils.TestUtils; | ||
|
||
public class CxxCompilerGccSensorTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we already have an issue in Sensor::describe() method, I would suggest to add
@Test
public void sensorDescriptor() {
...
}
both for GCC and VC sensors
@@ -52,7 +52,8 @@ private CxxMetricsFactory() { | |||
// Introduce metric keys for sensors - number of issues per file / per module | |||
CLANG_SA_SENSOR_ISSUES_KEY("ClangSA"), | |||
CLANG_TIDY_SENSOR_ISSUES_KEY("Clang-Tidy"), | |||
COMPILER_SENSOR_ISSUES_KEY("Compiler"), | |||
VC_SENSOR_ISSUES_KEY("Compiler-VC"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler-XXX look more consistent to me
@@ -104,7 +105,8 @@ public String toString() { | |||
|
|||
addSensorMetric(Key.CLANG_SA_SENSOR_ISSUES_KEY, "ClangSA issues", domain, langPropertiesKey, metrics); | |||
addSensorMetric(Key.CLANG_TIDY_SENSOR_ISSUES_KEY, "ClangTidy issues", domain, langPropertiesKey, metrics); | |||
addSensorMetric(Key.COMPILER_SENSOR_ISSUES_KEY, "Compiler issues", domain, langPropertiesKey, metrics); | |||
addSensorMetric(Key.VC_SENSOR_ISSUES_KEY, "VC compiler issues", domain, langPropertiesKey, metrics); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VC compiler issues IMHO
Please create an own PR. Think takes longer to finalize this one:
|
@ivangalkin need an regex expert :-). Like to introduce named groups for the regex parser. What's the best approach to:
MatchResult does not support named groups. Current code: try (Scanner scanner = new Scanner(report, reportCharset)) {
Pattern p = Pattern.compile(reportRegEx, Pattern.MULTILINE);
LOG.info("Using pattern : '{}'", p);
while (scanner.findWithinHorizon(p, 0) != null) {
MatchResult match = scanner.match();
String filename = alignFilename(match.group(gFn).trim());
String line = alignLine(match.group(gLine).trim());
String id = alignId(match.group(gId).trim());
String msg = alignMessage(match.group(gMsg).trim());
if (isInputValid(filename, line, id, msg)) {
if (LOG.isDebugEnabled()) {
LOG.debug("Scanner-matches file='{}' line='{}' id='{}' msg={}", filename, line, id, msg);
}
CxxReportIssue issue = new CxxReportIssue(id, filename, line, msg);
saveUniqueViolation(context, issue);
} else {
LOG.warn("Invalid compiler warning: '{}''{}'", id, msg);
}
} |
Hi @guwirth, unfortunately, I am not a big regex expert (TBH I am not a big Java expert either), but I'll try to help:
The only a little bit confusing pattern I found in your code snippet is |
@ivangalkin, @Bertk, @jmecosta changes done - ready to review again. |
* @return true, if valid | ||
*/ | ||
protected boolean isInputValid(String filename, String line, String id, String msg) { | ||
return !filename.isEmpty() || !line.isEmpty() || !id.isEmpty() || !msg.isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there are some overrides, but a warning/issue with an empty rule-ID is of no benefit at all. Same about the message IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivangalkin The idea of the plugin is to break if there is a configuration error and to give only warnings if there are (partial) problems with a report. So if there is one (several) wrong messages at least the other are reported.
Or what exactly do you expect in such a case?
*/ | ||
public static final String REPORT_PATH_KEY = "compiler.reportPath"; | ||
public static final String REPORT_CHARSET_DEF = "compiler.charset"; | ||
public static final String PARSER_KEY_DEF = "compiler.parser"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only supported value for compiler.parser
is "Visual C++"; (see sonar-cxx/cxx-squid/src/main/java/org/sonar/cxx/CxxConfiguration.java). Maybe we could deprecate this property too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivangalkin idea was to change only one feature with this PR and not also the "read configuration". In general there is still #1365.
@Bertk What are you thinking?
@@ -339,57 +336,73 @@ | |||
private static List<PropertyDefinition> compilerWarningsProperties() { | |||
String subcateg = "(4) Compiler warnings"; | |||
return new ArrayList<>(Arrays.asList( | |||
PropertyDefinition.builder(LANG_PROP_PREFIX + CxxCompilerSensor.REPORT_PATH_KEY) | |||
.name("Compiler report(s)") | |||
PropertyDefinition.builder(LANG_PROP_PREFIX + CxxCompilerVcSensor.REPORT_PATH_KEY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like we don't we miss the following properties now:
public static final String REPORT_PATH_KEY = "compiler.reportPath";
public static final String REPORT_CHARSET_DEF = "compiler.charset";
public static final String PARSER_KEY_DEF = "compiler.parser";
as far as I see, they are still used in order to set the project configuration. Is it your intent to hide them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for this hint. They are really missing now.
import org.sonar.cxx.sensors.compiler.CxxCompilerVcParser; | ||
import org.sonar.cxx.sensors.compiler.CxxCompilerVcRuleRepository; | ||
import org.sonar.cxx.sensors.compiler.CxxCompilerVcSensor; | ||
import org.sonar.cxx.sensors.compiler.gcc.CxxCompilerGccRuleRepository; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for c++ plugin
@ivangalkin I added missing UI properties "compiler.reportPath" and "compiler.charset". Please review again. @Bertk maybe we should rename them as well (compiler => msbuild): public static final String REPORT_PATH_KEY = "msbuild.reportPath";
public static final String REPORT_CHARSET_DEF = "msbuild.charset"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 27 files at r1, 8 of 13 files at r2, 1 of 5 files at r3.
Reviewable status: 22 of 26 files reviewed, 19 unresolved discussions (waiting on @guwirth, @ivangalkin, @jmecosta, and @Bertk)
cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerSensor.java, line 112 at r2 (raw file):
Previously, guwirth (Günter Wirth) wrote…
@ivangalkin The idea of the plugin is to break if there is a configuration error and to give only warnings if there are (partial) problems with a report. So if there is one (several) wrong messages at least the other are reported.
Or what exactly do you expect in such a case?
Looking at the code, which uses isInputValid()
...
if (isInputValid(filename, line, id, msg)) {
...
CxxReportIssue issue = new CxxReportIssue(id, filename, line, msg));
saveUniqueViolation(context, issue);
} else {
LOG.warn("Invalid compiler warning: '{}''{}'", id, msg);
}
... I believe that isInputValid()
must filter only the useful combinations of ìd
, filename
, line
and msg
filename
+line
can be theoretically empty. Such combination means an issue raised on the whole project/modulemsg
is optional (see documentation ofNewIssueLocation::message(String)
)id
can never be empty, because it's used as a rule key
In my opinion the correct implementation of isInputValid()
should be something like that:
protected boolean isInputValid(String filename, String line, String id, String msg) {
// msg is optional - no need to check
// id must be not empty
// for filename and line we expect: either both of them are set, or both of them are missing
return !id.isEmpty() && (filename.isEmpty() == line.isEmpty());
}
This check is nice to have, but it's not critical. The validation takes always place, inCxxIssuesReportSensor
or NewIssue
/NewIssueLocation
at the latest (however invalid input at this step will probably abort the sensor).
cxx-sensors/src/main/java/org/sonar/cxx/sensors/compiler/CxxCompilerSensor.java, line 70 at r3 (raw file):
saveUniqueViolation(context, issue); } else { LOG.warn("Invalid compiler warning: '{}''{}'", id, msg);
LOG.warn("Invalid compiler warning: '{}''{}'", id, msg);
add filename
and line
?
sonar-c-plugin/src/main/java/org/sonar/plugins/c/CPlugin.java, line 364 at r3 (raw file):
.onQualifiers(Qualifiers.PROJECT, Qualifiers.MODULE) .index(1) .build(),
.build(),······
trailing whitespaces (see below too)
sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/CxxPlugin.java, line 390 at r3 (raw file):
.onQualifiers(Qualifiers.PROJECT, Qualifiers.MODULE) .index(1) .build(),
.build(),······
trailing whitespaces (see below too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 22 of 26 files reviewed, 17 unresolved discussions (waiting on @guwirth, @ivangalkin, @jmecosta, and @Bertk)
@guwirth if this is braking change, that it is. Maybe its best to get ride of |
Would be renamed to then
|
For some reason I am unable to activate compiler rules anymore. Any idea? All other sensors activate ok.
|
@klimkin what do you see in the rules section of the UI? Are the rules still available and activated? |
@guwirth Administration->General Settings->C++(Community) all settings are there. Surprisingly, sonar.cxx.cppcheck.reportPath is just enough to see DEBUG logs trying to load the file. sonar.cxx.gcc.reportPath and sonar.cxx.clangtidy.reportPath do not have any effect. I was staring at cppcheck and clangtidy sensor sources for quite time and don't see anything that might cause the difference in behavior! |
Ahh, you are right. The rules were inactive by default. Thanks! |
Users like to use the VC and GCC compiler results in parallel.
Attention:
sonar.cxx.compiler
settings are no more supported.<file>, <line>, <id>, <message>
in your regex now:(.*>)?(?<file>.*)\((?<line>\d+)\)\x20:\x20warning\x20(?<id>C\d+):(?<message>.*)
^...$
no more needed in the regexRemove:
Add:
To read includes and defines from MSBuild log use:
This change is