Page MenuHomePhabricator

Migrating from FindBugs to SpotBugs
Needs ReviewPublic

Authored by CLOVIS on Apr 24 2019, 9:27 AM.

Details

Summary

Update the Gradle Wrapper

Migrate from FindBugs to SpotBugs

Test Plan

Diff Detail

Repository
rLIBWFJVA WildFyre Java Libary
Branch
spotbugs
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 685
Build 685: arc lint + arc unit

Event Timeline

CLOVIS created this revision.Apr 24 2019, 9:27 AM
Harbormaster completed remote builds in B648: Diff 879.
Hackintosh5 requested changes to this revision.Wed, Apr 24, 11:51 AM
Hackintosh5 added inline comments.
.gitlab-ci.yml
41

Do we really want spotBugs on tests?

api.sh
49 ↗(On Diff #879)

--norun should never start the api

build.gradle
78–79

don't use compile its deprecated
use format strings not concatenation

80

again, format strings please

src/test/Dockerfile
32 ↗(On Diff #879)

Better to use python3 -m pip; on some systems the alias is missing

This revision now requires changes to proceed.Wed, Apr 24, 11:51 AM
Hackintosh5 added inline comments.Wed, Apr 24, 12:53 PM
.gitlab-ci.yml
41

Just looked at old stuff, Info Screen accepted D226 (closed cos out of date) which removed spotbugsTest in favour of spotbugsMain only

.idea/runConfigurations/Static_Analysis.xml
15

Here also should remove spotbugsTest

api.sh
81 ↗(On Diff #879)

again the & won't help. you have to remove exec if you want it to run in the background.

gradle/wrapper/gradle-wrapper.properties
2 ↗(On Diff #879)

You cant copyright the gradle wrapper configs!

src/test/Dockerfile
20 ↗(On Diff #879)

why --no-cache?

27 ↗(On Diff #879)

why? also this isnt the official repo...

CLOVIS marked 2 inline comments as done.Wed, Apr 24, 1:07 PM
CLOVIS added inline comments.
build.gradle
78–79

The FindBugs to SpotBugs migration post says to use compile

src/test/Dockerfile
20 ↗(On Diff #879)
27 ↗(On Diff #879)

Find that file in any official repo and I'll change the link

32 ↗(On Diff #879)

Doesn't matter here, it's a raw Alpine container there's no compatibility issues

Hackintosh5 added inline comments.Wed, Apr 24, 1:14 PM
build.gradle
78–79

Don't.
From https://stackoverflow.com/a/44419574/5509575:
Using api is the equivalent of using the deprecated compile, so if you replace all compile with api everything will works as always.

src/test/Dockerfile
27 ↗(On Diff #879)

Well the README of that repo says that it comes from mercurial at https://hg.openjdk.java.net/jdk8/jdk8/. I'd have thought it was fairly obvious.

32 ↗(On Diff #879)

True, but nicer to be as platform agnostic as possible

CLOVIS marked 2 inline comments as done.Wed, Apr 24, 1:38 PM
CLOVIS added inline comments.
src/test/Dockerfile
27 ↗(On Diff #879)

Yeah, that's the official repo. The file I need is not in there though, Oracle stopped distributing it.

The closer you can get to an "official" way of doing it is this: https://bgasparotto.com/extract-src-zip-jdk-installer

Hackintosh5 added inline comments.Wed, Apr 24, 2:22 PM
src/test/Dockerfile
27 ↗(On Diff #879)

What file do you need then?

Hackintosh5 added inline comments.Wed, Apr 24, 2:33 PM
src/test/Dockerfile
27 ↗(On Diff #879)
CLOVIS added inline comments.Wed, Apr 24, 3:07 PM
src/test/Dockerfile
27 ↗(On Diff #879)

That uses apt, which doesn't exist on Alpine.

CLOVIS updated this revision to Diff 925.Wed, May 15, 8:58 PM

Rebasing everything on master

WyldBot requested changes to this revision.Sun, May 19, 1:41 PM
This revision now requires changes to proceed.Sun, May 19, 1:41 PM
CLOVIS updated this revision to Diff 939.Mon, May 20, 5:14 PM
CLOVIS added inline comments.Mon, May 20, 5:15 PM
.gitlab-ci.yml
41

I don't know... code quality in the tests sounds important to me, even if they're not in prod