Spring Cleaning Step 1: Spotbugs Integration

Development
1

Hello Guys,
I’ve had the idea to improve the code quality this spring so we can iron out problems and start implementing features on an even more solid base soon.

For this, I’ve worked on an integration of “SpotBugs”. This tool will analyze .class files and find common problems such as infinite loops, (guaranteed) Nullpointers, resource leaks and style problems such as unused variables.

I wrote a more detailed introduction at the readme here.

In short: The Plugin integrates into our Github Actions pipeline and will annotate pull requests and commits with potential problems.

If you want to look into what issues we currently (could) have (keep in mind that some might be on purpose and others are false positives) integrate this PR and execute gradlew spotbugsMain.

Currently I am looking for your opinions on:

  • The naming of the code/repository. On one hand it’s made for our code base, but on the other hand it’s relatively easy to adopt (hence we are uncertain about the jme3 prefix etc)
  • Any opinions towards the project and it’s integration into the engine repo? (The PR shows a few screenshots)

Edit: Currently the bot is broken, yesterday I splitted up the code into a few files and added a config, which is why some code is unreferenced yet.

13 Likes
2

Thanks for this work. I look forward to trying it on the Engine and perhaps also on Minie.

3

We are very close to Step 2, fellow monkeys.
All that is left before unleashing this onto the main repo is testing and minor tweaks (UX, Wording).

Please do your best and try to submit broken/bugged code to our testing repository (pay attention to the branch):

Have fun writing shitty Code :wink:

Edit: Currently it doesn’t run for PRs it seems, the Github Checks API.
I try to fix that asap, but since I don’t see any errors, it might be a problem

Edit2: Seems to be a hiccup as adding debug output to it suddenly made it work…

Anyway have a look at Performance Patch 1 by MeFisto94 · Pull Request #1 · MeFisto94/test-spotbugs · GitHub for what to do:
Currently for PRs it does add the expectations to the PR, which I think should just be disabled and run when the PR is merged, because it would need a rebase otherwise.

Also a lot of this data is not needed and adds useless diffs, we need to optimize that.
I don’t understand why spotbugs didn’t spot the endless loop.

But it didn’t mark things inline either, I think because the PR consists of multiple Commits…
Yeah, retrying works:Use endless recursion for more GPU friendly normalization by MeFisto94 · Pull Request #2 · MeFisto94/test-spotbugs · GitHub
But Github Actions seems to be flakey currently anyway, extracting those into dedicated workflows will make it run on different machines then anyway.

2 Likes
4

Perhaps I’m confused about how this is supposed to work. I committed some changes to my local clone of the repo and did a clean build. How can I get useful feedback on those changes without submitting push to GitHub?

Also, there were some unexpected diagnostics during my local builds:

> Task :jme3-jbullet:spotbugsMain
Warning: Obsolete StackMap attribute ignored.
Warning: Obsolete StackMap attribute ignored.
Warning: Obsolete StackMap attribute ignored.
Warning: Obsolete StackMap attribute ignored.
Warning: Obsolete StackMap attribute ignored.
...

and later

> Task :jme3-android:spotbugsMain
The following classes needed for analysis were missing:
  android.app.Activity
  android.content.DialogInterface$OnClickListener
  android.app.Fragment
  android.view.View$OnLayoutChangeListener
  android.opengl.GLSurfaceView$Renderer
  android.opengl.GLSurfaceView$EGLConfigChooser
  android.view.View$OnHoverListener
  android.view.View$OnGenericMotionListener
  android.view.GestureDetector$OnGestureListener
  android.view.GestureDetector$OnDoubleTapListener
  android.view.ScaleGestureDetector$OnScaleGestureListener
  android.hardware.SensorEventListener
...

Afterwards, I submitted my changes as a pull request. Should I expect some feedback? Where should I look for it?

5

You can’t really, it has been made to aid reviewing PRs and specifically to catch dangerous changes right before integration.

Also, ultimatively running spotbugsMain gives you a HTML in the modules’ build folders. Currently this is overwhelming but provided the sketchy code is cleaned out (step 2, actually what you are doing with the latest commits), it is easier to grasp.

Actually when looking for things to solve, this overwhelming thing has to be viewed.

Yep, googling brought me to the source code of spotbugs and it seems to be related to bytecode, not sure how we could trigger that…

So this is another speciality, we build against android but for some reason the internal classes are missing, not sure if we can solve this.

So basically there are two uses:

Your PR didn’t trigger any feedback, because Github Actions seems to have problems currently but also because you neither fixed nor created bugs, I think, because unused imports trigger compiler warnings, so it’s out of the scope of Spot Bugs.

Also Github Actions was having an HTTP Error remove some unused imports by stephengold · Pull Request #3 · MeFisto94/test-spotbugs · GitHub regardless.

But basically you have two choices: Add some infinite loop, certain null dereference or something to trigger a fault or have a look at the html files and fix some of those bugs and see if this is added (You need this under Checks: SpotBugs Static Analysis Task.

If this Task isn’t visible, Github Actions is having problems again.

Edit: Turns out we HAVE a problem for now: HttpError: Resource not accessible by integration
Googling that lead to this being a permission error. Pull Requests from internal branches seem to work while foreign PRs don’t have that for security reasons or something. I wonder how linters are supposed to work with this then.

Probably linters aren’t run/affected by github actions but run as their own dedicated app.

6

If one of my commits is dangerous, I want to know before I submit a pull request, not afterward!

Also, what can the GitHub runner can do that I can’t do on my own PC?

OK. Thanks!

7

That’s a good point, but I am not worried about you at all, more about people who aren’t as thoughtful when coding.

Currently the code uses the Github API to annotate code. We could adopt the code to write to stdout when there is no API Token present, but you would still need nodeJS, as I have written that as “Github Action” and to have easy code, so it’s not just a gradle task.

I guess rewriting in Gradle would even fail because when my task depends on spotbugsMain, it won’t be executed as spotbugsMain will fail when there are bugs.

8

I think sgold’s point in general is that smart contributors will want a way to check their code before they commit rather than having to wait for github to show them their mistakes. Or certainly users who will already be self conscious about committing would want to do every check possible first.

I guess the tricky part about running it locally is having it only run on your changes and not all of JME? Or is it not even a regular bug checking software that is integrated in build tools like bug checkers like pmd or checkstyle are? Because, yeah, nodeJs is a non-starter for me.

Edit: note that integrating PMD or checkstyle into the build might be desirable anyway. I know at least in maven integration you can configure it to fail the build on certain rules like “tabs in source code”. Something that can’t be done with class-based analysis. However, I do remember struggling to get tools like that to play nice in multiproject environments… and having it only run on a user’s changes might still be tricky.

9

Yes, PR https://github.com/jMonkeyEngine/jmonkeyengine/pull/1298 would show all issues in a nice HTML Document, but as it stands it’s 1.8k potential bugs.

So the code to do diffs is written in nodeJS (GitHub - MeFisto94/spotbugs-github-integration: A nodeJS project to analyse and diff spotbugs reports in order to annotate github commits and pull requests.), so it’s not “easy” to run locally [and currently fails because of a missing github token]

Edit: And doing the same with checkstyle is on the to-do list actually. Like: “here you added 7 spaces instead of 8” “No Pascal Case” etc

Edit 2: while it misses the point, one can at least run the analysis on their fork without a change by running github actions

Edit 3: Tracking issue for the most important problem right now: Creating a Check doesn't work for foreign PRs · Issue #2 · MeFisto94/spotbugs-github-integration · GitHub

10

Okay, turns out we need to take a different route.

→ Those pretty annotations work on everything but (foreign) pull requests.

2 Likes