Persistent post run exits early when run is successful in Magefile targets

kraashen commented 8 months ago

Welcome

[X] Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
[X] Yes, I've searched similar issues on GitHub and didn't find any.
[X] Yes, I've read the typecheck section of the FAQ (https://golangci-lint.run/usage/faq/#why-do-you-have-typecheck-errors).
[X] Yes, I've tried with the standalone linter if available (e.g., gocritic, go vet, etc.). (https://golangci-lint.run/usage/linters/)

Description of the problem

We are running golangci-lint using a Golang make/rake -like tool Mage.

There, we have defined a mage target that runs linting as well as other tasks one-by-one in serial order. We encountered that golangci-lint exits too early and the rest of the tasks following that were not executed.

Further examples are provided in the repo URL in the reproductible code section, but in short, when defining mage targets such as:

func Lint(...) {...} // define golangci-lint execution

func VulnCheck(...) {...} // define vuln/scan package execution

func CheckAll(ctx context.Context) {
    mg.SerialCtxDeps(ctx, Lint, VulnCheck)
}

running mage checkall skips the latter.

Now, when running golangci-lint with debugger it seems, that:

an early exit using os.exit was run in pkg/commands/root.go:persistentPostRun function.
when the executor is initialized with NewExecutor() in here the persistentPostRun function is configured as PersistentPostRunE handler for Cobra

So when linting is run in serial with other commands, golangci-lint exits the task pipeline too early with os.exit even when successful, and rest of the tasks are not run. Small (but not sure if relevant) detail is that in this context, golangci-lint is being used as a package library to not require users to install the CLI tool on the host, but using it as an utility library instead. Expected behavior is, that the serial command execution pipeline is exited early only when errors are encountered in the tasks in the pipeline, or finishes all the tasks with successful exit code.

Version of golangci-lint

``` v1.54.2 ```

Configuration

```console run: timeout: 15m ```

Go environment

```console $ go version && go env go version go1.21.3 darwin/arm64 GO111MODULE='' GOARCH='arm64' GOBIN='' GOCACHE='/Users/foo/Library/Caches/go-build' GOENV='/Users/foo/Library/Application Support/go/env' GOEXE='' GOEXPERIMENT='' GOFLAGS='' GOHOSTARCH='arm64' GOHOSTOS='darwin' GOINSECURE='' GOMODCACHE='/Users/foo/go/pkg/mod' GONOPROXY='' GONOSUMDB='' GOOS='darwin' GOPATH='/Users/foo/go' GOPRIVATE='' GOPROXY='https://proxy.golang.org,direct' GOROOT='/opt/homebrew/Cellar/go/1.21.3/libexec' GOSUMDB='sum.golang.org' GOTMPDIR='' GOTOOLCHAIN='auto' GOTOOLDIR='/opt/homebrew/Cellar/go/1.21.3/libexec/pkg/tool/darwin_arm64' GOVCS='' GOVERSION='go1.21.3' GCCGO='gccgo' AR='ar' CC='cc' CXX='c++' CGO_ENABLED='1' GOMOD='/Users/foo/Code/golangci-mage-sample/go.mod' GOWORK='' CGO_CFLAGS='-O2 -g' CGO_CPPFLAGS='' CGO_CXXFLAGS='-O2 -g' CGO_FFLAGS='-O2 -g' CGO_LDFLAGS='-O2 -g' PKG_CONFIG='pkg-config' GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/d7/ngb4dbgs5_10mdxzcxdnvms00000gn/T/go-build1531559415=/tmp/go-build -gno-record-gcc-switches -fno-common' ```

Verbose output of running

```console ❯ mage checkallverboselint # please see the example code for sample how this is executed in verbose mode INFO [config_reader] Config search paths: [./ /Users/tomi/Code/golangci-mage-sample /Users/tomi/Code /Users/tomi /Users /] INFO [config_reader] Used config file .golangci.yml INFO [lintersdb] Active 6 linters: [errcheck gosimple govet ineffassign staticcheck unused] INFO [loader] Go packages loading at mode 575 (compiled_files|deps|files|name|exports_file|imports|types_sizes) took 152.774375ms INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 936.709µs INFO [linters_context/goanalysis] analyzers took 2.813149455s with top 10 stages: fact_purity: 758.80058ms, buildir: 650.577291ms, typedness: 481.034205ms, nilness: 296.102543ms, ctrlflow: 244.157582ms, fact_deprecated: 227.482294ms, printf: 76.501167ms, SA5012: 53.968748ms, inspect: 21.671835ms, SA4014: 440.917µs INFO [runner] processing took 1.418µs with stages: max_same_issues: 333ns, skip_dirs: 167ns, severity-rules: 125ns, cgo: 125ns, nolint: 125ns, max_from_linter: 84ns, source_code: 42ns, identifier_marker: 42ns, skip_files: 42ns, filename_unadjuster: 42ns, path_prettifier: 42ns, path_shortener: 42ns, autogenerated_exclude: 42ns, diff: 42ns, exclude: 41ns, fixer: 41ns, path_prefixer: 41ns, max_per_file_from_linter: 0s, uniq_by_line: 0s, exclude-rules: 0s, sort_results: 0s INFO [runner] linters took 1.06527s with stages: goanalysis_metalinter: 1.065250959s INFO File cache stats: 0 entries of total size 0B INFO Memory: 14 samples, avg is 177.3MB, max is 252.6MB INFO Execution took 1.22676475s ```

A minimal reproducible example or link to a public repository

Sample repo with reproduction guide: https://github.com/kraashen/golangci-lint-mage-issue-sample

Validation

[X] Yes, I've included all information above (version, config, etc.).

boring-cyborg[bot] commented 8 months ago

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

ldez commented 8 months ago

Hello,

golangci-lint is not designed to be used as a library and we discourage that. This means, for example, that the exposed elements of golangci-lint can be changed at any time, and those changes will not imply a specific version change. Golangci-lint is designed to be a CLI tool only.

I understand your problem but you are using golangci-lint in a way that we highly discourage.

My first thought is to not accept changes that encourage the use of golangci-lint as a library.

heppu commented 8 months ago

Regardless of use case I would argue that having multiple exit points for application is bad practice and makes the control flow harder to follow.

ldez commented 8 months ago

This is a purely theoretical point of view, and I can agree with that, but a rule should be evaluated inside a context, not as a dogma.

We need to control the exit code and cobra doesn't provide a way to control it. So we have no choice.

I can use this argument in an opposite way: currently, we control all (except some edge cases) exit codes in one place.

heppu commented 8 months ago

Sure, I was just merely suggesting that this might still be worth looking into regardless the circumtances where user encountered this issue.

ldez commented 8 months ago

My message was oriented as an answer to the user but I evaluated the whole context. If the change was something useful outside of the user's context from my perspective, I would have added a sentence along these lines and maybe merged the related PR.

I didn't close the issue, to get more feedback, because it was possible I missed something. Thank you for interacting and trying to find a parallel way. Your argument led me to think that my first thought was right :smile:

heppu commented 8 months ago

Well, I'm glad to help :D

golangci / golangci-lint