Continuous Integration #47
Conversation
|
kriskowal
force-pushed
the
ci
branch
4 times, most recently
from
241704b
to
8168c2f
Compare
| @@ -62,7 +62,7 @@ func Draw(dst *display.Display, r image.Rectangle, src image.Image, sp image.Poi | |||
| DrawBitmap(dst, r, bits, sp, on) | |||
| } | |||
|
|
|||
| // DrawBitmap | |||
| // DrawBitmap FIXME | |||
There was a problem hiding this comment.
Addressing this down the stack in #44
There was a problem hiding this comment.
Can't say that I "approve" of all the imported cargo, and especially I think that "staticcheck" is a bridge too far at this point... but that could largely be out of lack of familiarity with it.
The "performance" remarks circa the makefile shell convolutions, I'm really more interested to chase to whatever upstream you're cargoing here, and aren't material to bork's "scale" ;-)
Per my comment on the go test ./... line, I suspect that much of the alacritty around "but not vendor" isn't necessary anymore.
Makefile
Outdated
| @@ -1,5 +1,84 @@ | |||
| PACKAGES := $(shell ./scripts/packages.sh) | |||
There was a problem hiding this comment.
Strongly prefer these inlined, having them in a script only increases cognitive load.
Makefile
Outdated
|
|
||
| ERRCHECK_FLAGS ?= -ignoretests | ||
|
|
||
| # Files whose first line contains "Code generated by" are generated. |
There was a problem hiding this comment.
first line seems awfully strict... but ok, I guess we don't yet generate #! files ;-)
Makefile
Outdated
| # Files whose first line contains "Code generated by" are generated. | ||
| GENERATED_GO_FILES := $(shell \ | ||
| find $(GO_FILES) \ | ||
| -exec sh -c 'head -n1 {} | grep "Code generated by\|Automatically generated by " >/dev/null' \; \ |
There was a problem hiding this comment.
Wow so how about something more like:
find . -name '*.go' | \
xargs grep -nm1 "Code generated by\|Automatically generated by" | \
grep '\.go:[0-9]:' | cut -d: -f1
- runs a batched number of greps (probably one for small projects)
- runs one program (another grep) to implement "is in the first N lines (N=10 for mine)"
- doesn't need to run a sub-shell for each file
So in short it runs N/g + 4 programs rather than 3*N + 1 programs (where g = (execveMaxBufferSize - length(ourGrepCommand)) / averageFileName .
(yes of course that should be shipped as find -print0 ... | xargs -0 ..., but the above reads clearer as a review comment ;-) )
But... where is all this coming from, we don't even code generate yet... do we have plans to?
Makefile
Outdated
| -exec sh -c 'head -n1 {} | grep "Code generated by\|Automatically generated by " >/dev/null' \; \ | ||
| -print) | ||
|
|
||
| LINT_EXCLUDES_EXTRAS := bindata.go |
There was a problem hiding this comment.
why is it extra? do they not add a generated marker (i.e. I see a "Code generated by ..." in a project's bindata.go locally)
Makefile
Outdated
| test: | ||
| go test ./... |
There was a problem hiding this comment.
The prior suffices since go 1.9 (golang/go#19090) do we really have to cargo in all this plaque?
Makefile
Outdated
|
|
||
| .PHONY: install-ci | ||
| install-ci: | ||
| go get -u github.com/golang/dep/cmd/dep |
There was a problem hiding this comment.
This can be one unified go get -u -v foo bar baz.
You could even get cute and go for go get -u -v github.com/golang/{dep/cmd/dep,lint/golint} github.com/{kisielk/errcheck} honnef.co/go/tools/cmd/staticcheck.
Either way, let's add that -v flag, so that travis console output becomes more debuggable Just In Case (tm).
Makefile
Outdated
| @[ ! -s "$(STATICCHECK_LOG)" ] || (echo "staticcheck failed:" | cat - $(STATICCHECK_LOG) && false) | ||
|
|
||
| .PHONY: lint | ||
| lint: gofmt govet golint errcheck staticcheck |
There was a problem hiding this comment.
Would like to consider using https://github.com/alecthomas/gometalinter going forward at least, even if you'd rather not in this one...
internal/cops/display/cursor.go
Outdated
| @@ -109,6 +109,9 @@ func (c Cursor) Reset(buf []byte) ([]byte, Cursor) { | |||
| c.Foreground = Colors[7] | |||
| c.Background = Colors[0] | |||
| buf = append(buf, "\033[m"...) | |||
| // Adding this superfluous return worked around staticcheck SA4005: | |||
There was a problem hiding this comment.
Ugh, than can we just not adopt staticcheck for now? Errcheck's signal/noise is just barely at "not annoying" levels, I don't have a feel yet for staticcheck; also I'm concerned because I've only ever encountered staticcheck in terms of "the one that goes really slow, so you probably don't want your editor running it everytime"... :notsureif:
There was a problem hiding this comment.
Found dominikh/go-tools#141 for context, author says that check is broken and needs to be redone...
There was a problem hiding this comment.
I’ve taken staticcheck out, but it did catch the reason my sigwinch's were not working, so now the showroom proof resizes properly.
kriskowal
force-pushed
the
ci
branch
3 times, most recently
from
db66435
to
bf10074
Compare
|
|
||
| .PHONY: install-ci | ||
| install-ci: | ||
| go get -u github.com/golang/dep/cmd/dep github.com/golang/lint/golint github.com/kisielk/errcheck |
There was a problem hiding this comment.
CI choked on curly brace expansion.
There was a problem hiding this comment.
probably posix sh vs bash-ism fwiw
|
Stripped down to minimum cargo. Somehow bindata’s not causing a problem, and we don’t have any other codegen that may or may not pass gofmt yet. Also clawed back some simplicity since we don’t need to filter logs to determine whether lint checks pass. |
This change adds Travis configuration and Makefile details to run tests and lint.