Add Static code analysis tool to ESH build #3995

svilenvul · 2017-08-09T11:10:43Z

Before merging:

~~CQ should be approved~~;
~~the version of the tool has to be updated with the latest that is released~~;
~~the high priority issues should be fixed~~;
~~Hue binding: XML thing types in error on tag representation-property #3963 is resolved. See XML validation check openhab/static-code-analysis#136~~

htreu · 2017-08-09T11:54:34Z

tools/static-code-analysis/checkstyle/suppressions.xml

@@ -16,4 +16,6 @@
 <!-- Eclipse SmartHome specific suppressions-->
 <!-- These bundles are generated trough XText -->
 <suppress files=".+org.eclipse.smarthome.model.+" checks="RequireBundleCheck|ExportInternalPackageCheck|ManifestPackageVersionCheck|ImportExportedPackagesCheck"/>
+ <!-- Some source files have different headers -->
+ <suppress files=".+org.eclipse.smarthome.automation.+" checks="HeaderCheck"/>


Do we really want to suppress checks or rather update the automation bundles to adhere to our guidelines?

Yes, because we currently have a different arrangement for the license header of the automation component.

maggu2810 · 2017-08-09T12:38:17Z

....test/src/test/groovy/org/eclipse/smarthome/binding/hue/test/HueBridgeHandlerOSGiTest.groovy

@@ -218,7 +218,7 @@ class HueBridgeHandlerOSGiTest extends AbstractHueOSGiTest {
 }

 @Test
- void 'assert that a status configuration message for missing bridge IP is properly returned (IP is an empty string)'() {
+ void 'assert that a status configuration message for missing bridge IP is properly returned - IP is an empty string'() {


Can you create a separate PR for that change?
I expect that it will be okay to change the long groovy names.

maggu2810 · 2017-08-09T12:38:23Z

....test/src/test/groovy/org/eclipse/smarthome/binding/hue/test/HueBridgeHandlerOSGiTest.groovy

@@ -197,7 +197,7 @@ class HueBridgeHandlerOSGiTest extends AbstractHueOSGiTest {
 }

 @Test
- void 'assert that a status configuration message for missing bridge IP is properly returned (IP is null)'() {
+ void 'assert that a status configuration message for missing bridge IP is properly returned - IP is null'() {


Can you create a separate PR for that change?
I expect that it will be okay to change the long groovy names.

Yes, of course.

maggu2810 · 2017-08-09T12:39:45Z

...core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/items/ItemUtil.java

@@ -78,7 +78,7 @@ public static void assertValidItemName(String itemName) throws IllegalArgumentEx

 public static State convertToAcceptedState(State state, Item item) {
 if (state == null) {
- LOGGER.error("A conversion of null was requested", new NullPointerException("state should not be null"));
+ LOGGER.error("A conversion of null was requested", new NullPointerException("state should not be null")); // NOPMD


What is the exact PMD warning that is suppressed here?

kaikreuzer · 2017-08-09T14:15:12Z

Created CQ 13980 for the tool dependency.

kaikreuzer · 2017-08-09T18:35:28Z

Woah, that was quick: The CQ is already approved!

See eclipse-archived#3995 Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

See eclipse-archived#3995 Fix typos eclipse-archived#3969 (review) Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

See #3995 Fix typos #3969 (review) Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

See #3995 Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

See eclipse-archived#3995 Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

See #3995 Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

The plugin exports annotations for FindBugs(SpotBugs). See eclipse-archived#4016 (comment) See eclipse-archived#3995 Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul · 2017-08-11T15:02:21Z

The latest build fails with the hue binding. After the update of the XML things schema, the build should be green :-).

svilenvul · 2017-08-14T08:52:27Z

@kaikreuzer

Friday the only remaining errors were caused from the .xsd file that is not up to date - #3963.

Should I ignore these errors or wait for the update of the schema file ?

kaikreuzer · 2017-08-14T09:17:29Z

Wait for the schema file - it should be up to date now! I retrigger Travis...

svilenvul · 2017-08-14T11:20:31Z

Retrigger Travis.

svilenvul · 2017-08-14T12:18:24Z

The build failed after 49 mins :

...
Still running (49 of 60): ./.build.sh
Still running (50 of 60): ./.build.sh

The job exceeded the maximum time limit for jobs, and has been terminated.

@kaikreuzer I guess the build is too slow with the static-analysis-tool added.

sjsf · 2017-08-15T07:06:24Z

I restarted it once again, just to make sure it was not caused by a slow repository, but the result was the same. That's indeed not so good news...
The global timeout for travis builds indeed is 50 minutes (see https://docs.travis-ci.com/user/customizing-the-build#Build-Timeouts), so there is nothing much we can do about it.
The build until now took something just above 20 minutes. Is it at least plausible that the code analysis adds that much to it? Sounds like a lot, so I'm wondering whether something went seriously wrong or not...

svilenvul · 2017-08-15T09:55:04Z

When we talk about speed/performance, FindBugs is always the main suspected :-) .

mvn static-code-analysis:findbugs
....
[INFO] Total time: 18:46 min
[INFO] Finished at: 2017-08-15T11:45:17+03:00
[INFO] Final Memory: 238M/1127M

Checkstyle checks are taking 06:32 min and PMD 02:41 min (and this includes the resolving of different repositories, actually the time is less).

I guess we can speed it up a bit if we tweak the configuration of FindBugs a little bit, but I am not sure if it will be enough.

I would appreciate help here.

svilenvul · 2017-08-18T08:10:00Z

@kaikreuzer, I guess it is up to you.

Please let me know what do you think about disabling the FindBugs checks for the ESH repository.

What we are going to do in this situation ?

kaikreuzer · 2017-08-18T08:35:58Z

Okay, then I missed some news. AFAIK you prefer a split of core and extensions -- some time ago.

No, you are only always underestimating my pragmatism. While I agree that it would be nice to have it in a separate repo and also to version it separately from the core, I have learned from having openHAB split into several repos what overhead this means in terms of build & release management. After all, we duplicate the number of build jobs, the repos to update for new tools like the static code analysis or other upgrades, the efforts on doing releases and the number of issue trackers (while people today already never know where to post something).
Seeing that we didn't manage to do an official release for a long time let's me doubt that we are in a position to think about separate versioning of core and extensions (which would imho be the main reason for a repo split).

Please let me know what do you think about disabling the FindBugs checks for the ESH repository.

I don't think that genereally disabling valuable checks is an option. We should rather think about how to reduce the situations, where we execute them. I would claim that the build time of the nightly job on our hudson is not an issue, so I would prefer to have it executing the full test/check suite.

Wrt Travis, I am not sure what our options are. If you could add a parameter which deactivates the FindBugs checks and we set that in the Travis configuration only, it might be an acceptable compromise.

svilenvul · 2017-08-18T10:59:35Z

Great, I have deactivated the FindBugs checks only in Travis.

This has caused some errors in the report generation goal, but they are already fixed with this commit.

I have also managed to generate a combined report for the whole repository, so maybe it is better to release new version first and then add it to the ESH build.

kaikreuzer · 2017-09-01T12:13:45Z

so maybe it is better to release new version first and then add it to the ESH build.

New version 0.3.0 has been released - do you want to update the PR accordingly? I think we would be good to merge then!

svilenvul · 2017-09-04T09:20:36Z

@kaikreuzer, I have rebased and updated the PR.

Do you think we should add a few sentences for the tool in the ESH documentation like we have done this in the openHAB docs - http://docs.openhab.org/developers/development/bindings.html#static-code-analysis ? Where would be the best place for that - maybe in the Coding Guidelines ?

kaikreuzer · 2017-09-04T09:29:47Z

Yes, I guess the coding guidelines are the best place for it.

Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul · 2017-09-04T11:26:06Z

@kaikreuzer, I am ready with the updates on this PR.

maggu2810

I wounder about the repositories and plugin repositories that has been added. Do you really need all of them?

maggu2810 · 2017-09-04T12:09:10Z

docs/documentation/development/guidelines.md

+
+The Eclipse SmartHome Maven build includes [tooling for static code analysis](https://github.com/openhab/static-code-analysis) that will validate your code against the Coding Guidelines and some additional best practices. Information about the checks can be found [here](https://github.com/openhab/static-code-analysis/blob/master/docs/included-checks.md).
+
+The tool will generate an idividual report for each binding that you can find in `../your_binding_directory/target/code-analysis/report.html` file and a report for the whole build that contains links to the individual reports in the `../smarthome/target/summary_report.html`. The tool categorizes the found issues by priority: 1(error),2(warning) or 3(info). If any error is found within your code the Maven build will end with failure. You will receive detailed information (path to the file, line and message) listing all problems with Prioriry 1 on the console.


It is not only about bindings but every bundle that is build (for each binding => for each bundle). And to be fair it would be not only be used for bundles, but for every artifact in the reactor, so "for each artifact" would be correct.

At the moment it will be used only on bundles - there are two profiles in this PR that activate the tool for bundles and bindings (the one is obsolete now, I will remove it). Initially the idea was to have separate rulesets included in each profile, but this changed over time.

Activating the checks for each artifact is not tested well enough, some of the OSGi specific checks might have problems with that.

maggu2810 · 2017-09-04T12:10:14Z

docs/documentation/development/guidelines.md

+
+The Eclipse SmartHome Maven build includes [tooling for static code analysis](https://github.com/openhab/static-code-analysis) that will validate your code against the Coding Guidelines and some additional best practices. Information about the checks can be found [here](https://github.com/openhab/static-code-analysis/blob/master/docs/included-checks.md).
+
+The tool will generate an idividual report for each binding that you can find in `../your_binding_directory/target/code-analysis/report.html` file and a report for the whole build that contains links to the individual reports in the `../smarthome/target/summary_report.html`. The tool categorizes the found issues by priority: 1(error),2(warning) or 3(info). If any error is found within your code the Maven build will end with failure. You will receive detailed information (path to the file, line and message) listing all problems with Prioriry 1 on the console.


Why do you add a ../ in front of the path.
Normally you call mvn ... in the root of the reactor and IMHO the relative paths should be relative to the root.

Thanks, I was wondering how to be more precise here, but without much success. I will change it to relative to the root of the reactor.

maggu2810 · 2017-09-04T12:11:15Z

pom.xml

+ </pluginRepository>
+ <pluginRepository>
+ <id>buchen-maven-repo</id>
+ <url>http://buchen.github.io/maven-repo</url>


Hasen't this one been removed previously by intention?
Why do you add it again?

maggu2810 · 2017-09-04T12:11:37Z

pom.xml

+ <id>eclipse</id>
+ <name>Eclipse Snapshot Repository</name>
+ <layout>default</layout>
+ <url>https://repo.eclipse.org/content/repositories/snapshots/</url>


This has been removed some days ago, why do you need the Eclipse Snapshot Repository?

As I see I only need both JFrog repositories (one for the plugin and one for its dependencies) , I will remove the others.

Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul · 2017-09-05T09:53:54Z

I hope I have answered all questions. Do you have any other concerns ?

kaikreuzer · 2017-09-05T11:19:05Z

pom.xml

 </profiles>

+ <pluginRepositories>
+ <pluginRepository>
+ <id>openhab-artifactory-release</id>


Actually, the official repo for the releases is "jcenter" at https://jcenter.bintray.com/.

It probably makes sense that I also request to have it published to Maven central, then we would not at all need a specific repo here.

kaikreuzer · 2017-09-05T11:20:47Z

pom.xml

+ </pluginRepositories>
+
+ <repositories>
+ <repository>


Isn't the pluginRepository enough? Why do we need it as a dependency repository as well?

Good question, I haven't taught about that much until you asked.
The tool(as a Maven plugin) internally starts the Maven plugins for Checkstyle, PMD and FindBugs and adds itself as a dependency to these plugins, so the included custom checks that are contained there are available in the classpath.

Did you test if it also works without it?

Yes, I tested it, it is not working without this repository.

kaikreuzer · 2017-09-05T13:30:32Z

pom.xml

 </profiles>

+ <pluginRepositories>
+ <pluginRepository>
+ <id>JCenter Repository</id>


id should imho rather be "jcenter"

kaikreuzer · 2017-09-05T13:30:44Z

pom.xml

+
+ <repositories>
+ <repository>
+ <id>openhab-artifactory-release</id>


id should be "jcenter"

Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

kaikreuzer · 2017-09-05T17:41:48Z

Thanks!

htreu reviewed Aug 9, 2017

View reviewed changes

maggu2810 reviewed Aug 9, 2017

View reviewed changes

kaikreuzer added the CQ label Aug 9, 2017

kaikreuzer removed the CQ label Aug 9, 2017

svilenvul mentioned this pull request Aug 10, 2017

Remove braces from method names. #4007

Merged

svilenvul pushed a commit to MusalaSoft/smarthome that referenced this pull request Aug 10, 2017

Add placeholder to logging messages

e6e0f66

See eclipse-archived#3995 Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul mentioned this pull request Aug 10, 2017

Add placeholder to logging messages #4011

Merged

svilenvul pushed a commit to MusalaSoft/smarthome that referenced this pull request Aug 10, 2017

Author tags added.

c4de3a6

See eclipse-archived#3995 Fix typos eclipse-archived#3969 (review) Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul mentioned this pull request Aug 10, 2017

Author tags added. #4012

Merged

kaikreuzer pushed a commit that referenced this pull request Aug 10, 2017

Author tags added. (#4012)

d6dc45a

See #3995 Fix typos #3969 (review) Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

kaikreuzer pushed a commit that referenced this pull request Aug 10, 2017

Add placeholder to logging messages (#4011)

ddd8f9c

See #3995 Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul mentioned this pull request Aug 10, 2017

Add exceptions to analysis tool #4016

Closed

svilenvul pushed a commit to MusalaSoft/smarthome that referenced this pull request Aug 10, 2017

Fix remaining errors reported by the analysis tool

205dd4d

See eclipse-archived#3995 Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul mentioned this pull request Aug 10, 2017

Fix remaining errors reported by the analysis tool #4017

Merged

kaikreuzer pushed a commit that referenced this pull request Aug 10, 2017

Fix remaining errors reported by the analysis tool (#4017)

5db2335

See #3995 Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul mentioned this pull request Aug 11, 2017

Add spotbugs to target platform #4023

Closed

svilenvul force-pushed the static-code-analysis branch from 73a4db1 to 3d27290 Compare August 11, 2017 11:50

svilenvul closed this Aug 14, 2017

svilenvul reopened this Aug 14, 2017

svilenvul changed the title ~~[WIP] Add Static code analysis tool to ESH build~~ Add Static code analysis tool to ESH build Aug 18, 2017

svilenvul force-pushed the static-code-analysis branch 3 times, most recently from cbbf304 to 3676fc0 Compare September 4, 2017 09:19

Static analysis added to ESH build

f2a7179

Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul force-pushed the static-code-analysis branch from 0603f43 to f2a7179 Compare September 4, 2017 10:40

maggu2810 reviewed Sep 4, 2017

View reviewed changes

Svilen Valkanov added 3 commits September 4, 2017 16:17

Remove unneeded repositories

5638566

Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

Remove unused profile

b2a8293

Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

Refine the documentation about the tool

8636a8e

Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

kaikreuzer suggested changes Sep 5, 2017

View reviewed changes

kaikreuzer reviewed Sep 5, 2017

View reviewed changes

pom.xml Outdated

<repositories>

<repository>

<id>openhab-artifactory-release</id>

Copy link

Contributor

kaikreuzer Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id should be "jcenter"

svilenvul force-pushed the static-code-analysis branch from 9e8cff6 to 01ee70d Compare September 5, 2017 14:53

Change to JCenter repository

12c5234

Signed-off-by: Svilen Valkanov <svilen.valkanov@musala.com>

svilenvul force-pushed the static-code-analysis branch from 01ee70d to 12c5234 Compare September 5, 2017 14:55

kaikreuzer approved these changes Sep 5, 2017

View reviewed changes

kaikreuzer merged commit b0812c2 into eclipse-archived:master Sep 5, 2017

svilenvul mentioned this pull request Sep 28, 2017

Parallel builds using static-code-analysis get stuck and fail openhab/static-code-analysis#200

Closed

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

svilenvul mentioned this pull request Dec 15, 2017

Improve build times using the tool openhab/static-code-analysis#232

Closed


		The Eclipse SmartHome Maven build includes [tooling for static code analysis](https://github.com/openhab/static-code-analysis) that will validate your code against the Coding Guidelines and some additional best practices. Information about the checks can be found [here](https://github.com/openhab/static-code-analysis/blob/master/docs/included-checks.md).

		The tool will generate an idividual report for each binding that you can find in `../your_binding_directory/target/code-analysis/report.html` file and a report for the whole build that contains links to the individual reports in the `../smarthome/target/summary_report.html`. The tool categorizes the found issues by priority: 1(error),2(warning) or 3(info). If any error is found within your code the Maven build will end with failure. You will receive detailed information (path to the file, line and message) listing all problems with Prioriry 1 on the console.

Add Static code analysis tool to ESH build #3995

Add Static code analysis tool to ESH build #3995

Conversation

svilenvul commented Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer commented Aug 9, 2017

kaikreuzer commented Aug 9, 2017

svilenvul commented Aug 11, 2017

svilenvul commented Aug 14, 2017

kaikreuzer commented Aug 14, 2017

svilenvul commented Aug 14, 2017

svilenvul commented Aug 14, 2017

sjsf commented Aug 15, 2017

svilenvul commented Aug 15, 2017 • edited Loading

svilenvul commented Aug 18, 2017

kaikreuzer commented Aug 18, 2017

svilenvul commented Aug 18, 2017 • edited Loading

kaikreuzer commented Sep 1, 2017

svilenvul commented Sep 4, 2017

kaikreuzer commented Sep 4, 2017

svilenvul commented Sep 4, 2017

maggu2810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svilenvul commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer commented Sep 5, 2017

svilenvul commented Aug 9, 2017 •

edited

Loading

svilenvul commented Aug 15, 2017 •

edited

Loading

svilenvul commented Aug 18, 2017 •

edited

Loading