We have a new repo at https://github.com/openhab/static-code-analysis.
This contains tooling for static code analysis - it combines PMD with FindBugs and Checkstyle and is meant to help on code reviews of pull requests. My hope is that we can cover most “boring” parts of our coding guidelines, like the correct use of loggers, javadoc, license headers, manifest details, etc.
The tooling is not there yet, but you are all invited to help making it better (and closer to our coding guidelines).
For the start, I have released a version 0.0.1 and already integrated it into the openHAB2-addons build.
So what I would like to ask you is to locally run Maven with the “check” profile enabled, before creating a PR:
mvn clean install -P check
This will produce an HTML summary report in the target folder (in the repo root), where you can click through the results.
A typical report of a single bundle e.g. looks like this. If you find false positives, please enter an issue for the tooling, so that it can be discussed and adapted.
Once we have good results from this tool, I would plan to activate it on PR builds, so that all PRs will have to pass this check as a very first gate. I think this will be helpful as it gives very quick early feedback to every contributor and it will reduce the manual work required for a code review afterwards - so please help making this tool as effective as possible!
Can it be there is an issue with the http://download.eclipse.org/smarthome/updates-stable repository? I reinstalled the Eclipse IDE and cannot build locally anymore with the above repository failing. If I check all error messages from the automated PR build, they all fail to find this repository. See also #24552.
I didn’t look too closely, but I see this error in the zwave build -:
[INFO] Downloading: https://repository-openhab.forge.cloudbees.com/snapshot/org/openhab/tools/static-code-analysis/maven-metadata.xml
[ERROR] Internal error: java.lang.RuntimeException: org.apache.maven.MavenExecutionException: Could not setup plugin ClassRealm: Error resolving version for plugin 'org.openhab.tools:static-code-analysis' from the repositories...
Probably the simplest fix is if you add this plugin repository to your z-wave pom.xml. We will have to clean up the whole pom structure across the different repos - I tried to start with it on the weekend, but didn’t get to any convincing results…
Yes, it actually came as an ESH contribution. It isn’t yet configured in the ESH build though, so you would have to do that manually first. Once we see it working well for openHAB, I will go through the IP process to have it also approved as a tool for ESH.
@Kai Many thanks for fixing. I the code analysis, did a few changes and committed everything in one commit. It was nice to see the online PR build succeed for my Niko Home Control binding (PR #1589)
Ok, got it working. Now it fails with bugs outside my binding and requests those to be fixed. Is it possible to configure the build to not fail on errors? If I add -fae the build runs on, but most of the targets are skipped because smarthome.core failed.
@Kai running the code analysis could be automated as well. For example like this workflow:
Run a Travis build (free for open source projects) on new PR’s or new commits on a PR
Perform a sonar-scanner analysis (free sonarqube.com server can be used). Sonar has support/plugins for PMD, FindBugs and Checkstyle
Make use of the Sonar GitHub Plugin to have a Bot automatically comment the found issues to the PR (check out the screenshots here or see the bot in action on this repo: https://github.com/SonarSource/sonarqube/pulls)
Make the bot add a label like has-code-smell or something similar which indicates that no core dev needs to further take a review at the current time. The author of the PR has all needed information on how to fix the issues by the Bot. Otherwise, a label like ready-for-review might be added.
This should be pretty easy to setup. The most complicated step might be to get sonar-scanner running with PMD, FindBugs and Checkstyle (at least I have no experience with that).
However, it would improve a) the time a PR’s author waits for feedback and b) reviewers can focus on PR’s that have no “boring” code issues. If you guys have questions I’m happy to answer or help out.
However, I don’t see how this should be avoided if I create objects which I all use and that need to be valid after the loop finishes (e.g. I create a object for each DMX channel during thing creation. These objects are added to a List, so they can be accessed later). Is this merely a warning that can be ignored if I check that is done intentionally?
See here. So in general, it should be avoided, but in practice it seems to be necessary. As we are on Java8 by now, populating lists might be more elegantly done through streams now, which should not show such warnings.
As it is just a warning and not an error, I think it is ok to keep it in the report.
Yes, that’s what I want to do as I have mentioned above (“Once we have good results from this tool, I would plan to activate it on PR builds”).
Note that we already have PR builds in place with Jenkins on Cloudbees. I would not want to have addional PR builds on Travis, if this can be avoided as it might rather confuse people.
Issue with SonarQube is that afaik, you cannot use your personal set of checks, so this won’t help us, if I do not miss anything?
Feel free to browse through it and identify anything that is falsely reported as well as anything that you think is covered in our coding guidelines, but not being mentioned in the report. You can directly add an issue here with your suggestions.
I’m doing something wrong, but what?
I tried to run “mvn clean install -P check” after having added the profiles to the pom.xml in rot directory of my binding.
I’m getting those errors:
[ERROR] [ERROR] Some problems were encountered while processing the POMs:
[ERROR] ‘build.plugins.plugin.version’ for org.openhab.tools:static-code-analysis must be a valid version but is ‘${sat.version}’. @ org.openhab:pom:2.0.0-SNAPSHOT, D:\OpenHABEntwicklung\openhab2-master\git\MyTankerkoenig\pom.xml, line 319, column 22
@
[ERROR] The build could not read 1 project → [Help 1]
…
OK.
I’m actually working in support to the author of a binding So I forked from him and he in turn has forked 2.0.0
I’ll try to fork from the actual Addons snapshot and put the binding-code in there.
Will inform the author as well. @dolic
