github checks: gometalinter structcheck/varcheck/deadcode

[lkwg82]

Purpose

static code analysis: this add checks to build.go:

deadcode - Finds unused code.
structcheck - Find unused struct fields.
varcheck - Find unused global variables and constants.

see more on https://github.com/alecthomas/gometalinter

Testing

$ go run build.go setup
$ go run build.go lint

[lkwg82]

conversation from #3084

@lkwg82

mhm, I thought these checks will be executed in jenkins and declared in build.go?

@AudriusButkevicius

Currently we just have

#!/bin/bash
set -euo pipefail

# Check for incorrectly formatted code
unformatted=$(gofmt -l $(find cmd/ lib/ -name \*.go))
if [ -z "$unformatted" ] ; then
    echo "Files formatted correctly"
    exit 0
else
    echo Files need gofmt:
    echo "$unformatted"
    exit 1
fi

which publishes stuff back into github.

[lkwg82]

My understanding is, that every developer can run locally these checks. So I suggest to add these checks in the project and make them usable with e.g. ./build.sh check.

These checks should run on jenkins with ./build.sh ci-fmt-check in another job ./build.sh ci-xyz-check ... and so on. But aggregated for devs in one call.

[AudriusButkevicius]

Potentially, as a general blanket check yet, though in the PR's it might be nice to have detailed checks.

[calmh]

If it's structcheck we want, I think we should pull in https://github.com/opennota/check directly. I use the metalinter now and then, but find it way too noisy in the general case.

[calmh]

The checks that we want to run always are run as part of regular install (i.e. go run build.go without parameters) already - lint and vet. We should add this there as well.

[lkwg82]

I see metalinter as a frontend. I just tried structcheck directly and I has inconvinient path handling. I rather prefer metalinter with a selected set of linters. Evaluate one by one.

[calmh]

That's fair. In that case I'd focus on the tool that you used for the dead code analysis for example, as that has proven itself in finding stuff that we've left behind. :) But regardless, the preferred way to me would be to hook into the place where we do the current vet and lint calls. Verify that that it (the function in build.go) correctly understands the difference between an error because the linter isn't installed and an error because the linter found something wrong - fail the build in the latter case.

I don't expect the exit code 3 check to work on metalinter not being installed, for example.

[lkwg82]

Maybe we can merge vet and lint command as only lint?

[lkwg82]

Why is lint not a github check?

[calmh]

Lint is informational and not meant to be 100% enforced. It is however run as part of the pr-build step, it just doesn't fail the build. go vet is also run there and does fail the build on issues.

[lkwg82]

I'd like to see it enforced, so vetmight be the better location for that, isn't it?

[AudriusButkevicius]

This feels so java.
Yet if we go this way, I'd do this on a non-pointer receiver.

[lkwg82]

This feels so java.

Do you feel bad vibrations? ;)

[AudriusButkevicius]

This will still verify it multiple times, as the methods are no longer pointer receivers, and you have multiple instances of the objects anyway.

[lkwg82]

I mixed. So this should still be valid.

[AudriusButkevicius]

I don't understand.
How is performing existence checks once before the whole song less beneficial than constructing 3 objects, chaining them multiple times and then calling run just to do nothing?

[lkwg82]

no, due to creating new instances, this is b*shit. I need to fix it. Too late yesterday.

I'd like to keep it encasulated and maybe checked multiple times. This should be a performance penalty of 200ms(?) each run. Else it become cluttered.

If it wasn't in the build tool it would have been a different thing.

[lkwg82]

@calmh sometimes it feels to have too few changes to extra do a pull request. But lets have a try ;)

[lkwg82]

@calmh Thought maybe baby commits makes the diff easier. But seems you review the total diff.

[lkwg82]

@st-jenkins retest plz

[AudriusButkevicius]

@st-jenkins whitelist

[AudriusButkevicius]

@st-jenkins retest this please

[lkwg82]

Anything left?

[AudriusButkevicius]

Looks fine, but I'll leave for @calmh to merge

[lkwg82]

ok

[calmh]

Lgtm apart from being too noisy. We don't print or time any other checks so no reason to do it here either, and the "no output = no issues" property is convenient when using this as a check from other places. Remove that and merge. :)

[lkwg82]

@st-review merge

build.go: add gometalinter to lint runs

[st-review]

👌 Merged as b78bfc0. Thanks, @lkwg82!