search

golangci / golangci-lint

Fast linters runner for Go
https://golangci-lint.run
GNU General Public License v3.0
15.52k stars 1.38k forks source link

--timeout does not validate passed value and gives confusing error #957

Open invidian opened 4 years ago

invidian commented 4 years ago

Thank you for creating the issue!

Please include the following information:

Version of golangci-lint ```console $ golangci-lint --version golangci-lint has version 1.23.2 built from fd621bd on 2020-02-02T22:12:01Z ```
Config file ```console $ cat .golangci.yml cat: .golangci.yml: No such file or directory ```
Go environment ```console $ go version && go env go version go1.13.7 linux/amd64 GO111MODULE="" GOARCH="amd64" GOBIN="" GOCACHE="/home/invidian/.cache/go-build" GOENV="/home/invidian/.config/go/env" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GONOPROXY="" GONOSUMDB="" GOOS="linux" GOPATH="/home/invidian/go" GOPRIVATE="" GOPROXY="https://proxy.golang.org,direct" GOROOT="/usr/lib/go" GOSUMDB="sum.golang.org" GOTMPDIR="" GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64" GCCGO="gccgo" AR="ar" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="/home/invidian/repos/kinvolk/lokoctl/go.mod" CGO_CFLAGS="-g -O2" CGO_CPPFLAGS="" CGO_CXXFLAGS="-g -O2" CGO_FFLAGS="-g -O2" CGO_LDFLAGS="-g -O2" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build749823572=/tmp/go-build -gno-record-gcc-switches" ```
Verbose output of running ```console $ golangci-lint run --enable-all --max-same-issues=0 --max-issues-per-linter=0 --build-tags aws,packet,e2e --new-from-rev=$(git merge-base master HEAD) --timeout=0 -v ./... INFO [config_reader] Config search paths: [./ /home/invidian/repos/kinvolk/lokoctl /home/invidian/repos/kinvolk /home/invidian/repos /home/invidian /home /] WARN Failed to discover go env: failed to run 'go env': context deadline exceeded INFO Memory: 2 samples, avg is 68.9MB, max is 68.9MB INFO Execution took 130.84µs INFO [lintersdb] Active 41 linters: [bodyclose deadcode depguard dogsled dupl errcheck funlen gochecknoglobals gochecknoinits gocognit goconst gocritic gocyclo godox gofmt goimports golint gomnd goprintffuncname gosec gosimple govet ineffassign interfacer lll maligned misspell nakedret prealloc rowserrcheck scopelint staticcheck structcheck stylecheck typecheck unconvert unparam unused varcheck whitespace wsl] INFO [lintersdb] Active 41 linters: [bodyclose deadcode depguard dogsled dupl errcheck funlen gochecknoglobals gochecknoinits gocognit goconst gocritic gocyclo godox gofmt goimports golint gomnd goprintffuncname gosec gosimple govet ineffassign interfacer lll maligned misspell nakedret prealloc rowserrcheck scopelint staticcheck structcheck stylecheck typecheck unconvert unparam unused varcheck whitespace wsl] INFO [loader] Go packages loading at mode 575 (files|name|types_sizes|exports_file|deps|imports|compiled_files) took 119.852µs ERRO Running error: context loading failed: failed to load program with go/packages: couldn't exec 'go [list -e -json -compiled=true -test=true -export=true -deps=true -find=false -tags aws packet e2e -- ./...]': context deadline exceeded context.deadlineExceededError ERRO Timeout exceeded: try increase it by passing --timeout option ```

When running linter with --timeout=0, following error is returned to the user:

WARN Failed to discover go env: failed to run 'go env': context deadline exceeded 
ERRO Running error: context loading failed: failed to load program with go/packages: couldn't exec 'go [list -e -json -compiled=true -test=true -export=true -deps=true -find=false -tags aws packet e2e -- ./...]': context deadline exceeded context.deadlineExceededError 
ERRO Timeout exceeded: try increase it by passing --timeout option

I didn't look into the code, but my guess is, that timeout value is not validated.

ernado commented 4 years ago

It is technically correct, timeout in zero seconds is immediately exceeded :)

I agree that error can be confusing. Do you want something like "timeout should be positive" error?

invidian commented 4 years ago

I would expect, that timeout=0 would disable the timeouts, not throw an error. What do you think about this?

ernado commented 4 years ago

It is reasonable, but it looks like breaking change 👀 Personally I prefer no timeout by default, but we can't change this behavior now.

I think it is up to @jirfag to decide here. Issue seems to be pretty minor so we can wait for major stakeholders, because reverting this change will be unfortunate and definitely will break things.

For anybody coming from search: please use some very big value like 24h, most CI/CD systems have ~1h timeout for job anyway.

invidian commented 4 years ago

No timeout by default seems also reasonable for me!

Since this is indeed minor issue, I'm even fine with closing, as it works as expected, if you're saying that's the case.

And thanks for quick feedback, I appreciate it :)