Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
5 tasks done
kraashen opened this issue Oct 18, 2023 · 7 comments · May be fixed by #4138
Open
5 tasks done

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

kraashen opened this issue Oct 18, 2023 · 7 comments · May be fixed by #4138
Labels
area: CLI Related to CLI no decision No decision to fix or not

Comments

@kraashen
Copy link

kraashen commented Oct 18, 2023 •
edited

Welcome

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:

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

run:
  timeout: 15m

Go environment

$ 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

❯ 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

  • Yes, I've included all information above (version, config, etc.).
@kraashen kraashen added the bug Something isn't working label Oct 18, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Oct 18, 2023

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

@kraashen kraashen linked a pull request Oct 18, 2023 that will close this issue
Open
@kraashen kraashen changed the title Persistent post run exits early when run is successful Persistent post run exits early when run is successful in Magefile targets Oct 18, 2023
@ldez ldez added area: CLI Related to CLI and removed bug Something isn't working labels Oct 18, 2023
@ldez
Copy link
Member

ldez commented Oct 24, 2023

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.

@ldez ldez added the no decision No decision to fix or not label Oct 24, 2023
@heppu
Copy link

heppu commented Oct 24, 2023

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
Copy link
Member

ldez commented Oct 24, 2023 •
edited

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
Copy link

heppu commented Oct 24, 2023

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

@ldez
Copy link
Member

ldez commented Oct 24, 2023

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 😄

@heppu
Copy link

heppu commented Oct 24, 2023

Well, I'm glad to help :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area: CLI Related to CLI no decision No decision to fix or not
Projects
None yet
Milestone
No milestone
3 participants
@heppu @ldez @kraashen