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.

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

Switch from deprecated gometalinter to golangci/golangci-lint-action #975

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

slonopotamus
Copy link
Contributor

@slonopotamus slonopotamus commented Mar 17, 2021

Subj, as requested by @katiewasnothere.


  1. This thing finds several violations. Unfortunately, I'm not going to fix them, so feel free to pick changes from this PR and apply fixes on top.
  2. It doesn't report file/line where linter detected an issue neither in GitHub Actions job results nor in build log. There's an open issue (github action log output doesn't print the affected file name & line golangci/golangci-lint-action#119), but I find current behavior extremely confusing for contributors. Possibly you want to wait until this issue is fixed. I am actually not sure how I see errors file/line info at all for current PR.
  3. It definitely requires a documentation on "how to run linter locally" for contributors.

@dcantah
Copy link
Contributor

dcantah commented Mar 17, 2021

Man does it really not show the line numbers...

@dcantah
Copy link
Contributor

dcantah commented Mar 17, 2021

Most of them can be figured out pretty easily just by symbols mentioned but some of them are awful.

@slonopotamus
Copy link
Contributor Author

Man does it really not show the line numbers...

I'm not joking, this is the way it currently works! golangci/golangci-lint-action#119 suggests that errors would be visible in diff. But it is possible for errors to arise in the code that already exists due to code changes. And you can't see them anywhere.

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, bummed about the line numbers issue though. We'll work on fixing those.

Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slonopotamus Welp.. That's annoying. We'll chew away at fixing these then but the actual change looks fine to me.

@thaJeztah
Copy link
Contributor

Man does it really not show the line numbers...

Ah, yes, I've seen that on other repositories as well; really annoying

golangci/golangci-lint-action#119 suggests that errors would be visible in diff.

I have a feeling that's sometimes "random" (perhaps it works once it has a good "base" result, after which it can comment on new changes only, but it's a hassle if linters are updated and start producing errors on existing code)

@thaJeztah
Copy link
Contributor

thaJeztah commented Mar 17, 2021

Alas, thought I'd give it a quick look, but I see it's not possible to run it on a Linux machine (some code may be missing a build-tag, causing it to be imported, but fail on Linux);

$ docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.38.0 golangci-lint run -v
level=info msg="[config_reader] Config search paths: [./ /app / /root]"
level=info msg="[lintersdb] Active 10 linters: [deadcode errcheck gosimple govet ineffassign staticcheck structcheck typecheck unused varcheck]"
level=info msg="[loader] Go packages loading at mode 575 (compiled_files|deps|name|types_sizes|exports_file|files|imports) took 43.78349407s"
level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 312.323114ms"
level=info msg="[linters context/goanalysis] analyzers took 29.510010324s with top 10 stages: buildir: 19.047299839s, fact_deprecated: 2.455168594s, fact_purity: 1.693131699s, inspect: 1.269189209s, nilness: 575.496716ms, ctrlflow: 520.745104ms, printf: 477.085181ms, SA5012: 334.924374ms, typedness: 260.446627ms, isgenerated: 220.780037ms"
level=warning msg="[runner] Can't run linter goanalysis_metalinter: S1010: failed prerequisites: [(inspect@github.com/Microsoft/hcsshim/internal/interop, isgenerated@github.com/Microsoft/hcsshim/internal/interop): analysis skipped: errors in package: [/app/internal/interop/interop.go:13:17: UTF16ToString not declared by package syscall /app/internal/interop/interop.go:14:2: undeclared name: coTaskMemFree]]"
level=info msg="[linters context/goanalysis] analyzers took 2.486335783s with top 10 stages: buildir: 1.773297674s, isgenerated: 315.369011ms, unused: 303.664154ms, directives: 94.004944ms"
level=warning msg="[runner] Can't run linter unused: buildir: analysis skipped: errors in package: [/app/internal/interop/interop.go:13:17: UTF16ToString not declared by package syscall /app/internal/interop/interop.go:14:2: undeclared name: coTaskMemFree]"
level=info msg="[runner] processing took 4.172µs with stages: max_same_issues: 888ns, nolint: 633ns, skip_dirs: 435ns, skip_files: 308ns, cgo: 268ns, max_from_linter: 227ns, filename_unadjuster: 200ns, path_prettifier: 123ns, autogenerated_exclude: 120ns, uniq_by_line: 118ns, path_prefixer: 115ns, source_code: 113ns, identifier_marker: 112ns, exclude: 107ns, diff: 106ns, path_shortener: 104ns, max_per_file_from_linter: 55ns, sort_results: 51ns, severity-rules: 46ns, exclude-rules: 43ns"
level=info msg="[runner] linters took 13.97535622s with stages: goanalysis_metalinter: 12.568891682s, unused: 1.406346264s"
level=error msg="Running error: buildir: analysis skipped: errors in package: [/app/internal/interop/interop.go:13:17: UTF16ToString not declared by package syscall /app/internal/interop/interop.go:14:2: undeclared name: coTaskMemFree]"
level=info msg="Memory: 569 samples, avg is 127.7MB, max is 476.2MB"
level=info msg="Execution took 58.391537511s"

@katiewasnothere
Copy link
Contributor

Alas, thought I'd give it a quick look, but I see it's not possible to run it on a Linux machine (some code may be missing a build-tag, causing it to be imported, but fail on Linux);

$ docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.38.0 golangci-lint run -v

@thaJeztah Are you sure the correct GOOS was set for building the package? I tried with the linux installation instructions here in wsl and it worked fine.

@dcantah
Copy link
Contributor

dcantah commented Mar 18, 2021

@slonopotamus Forgot to mention, we were going to wait to fix up all of the linter issues before checking this in but you might have deduced that from these #976 #977. Thanks again

@thaJeztah
Copy link
Contributor

@thaJeztah Are you sure the correct GOOS was set for building the package? I tried with the linux installation instructions here in wsl and it worked fine.

Oh! Good one; I didn't try setting GOOS. Still wondering what's best; explicitly exclude files from !windows or not (I recall some changes were made in that area recently)

@slonopotamus Forgot to mention, we were going to wait to fix up all of the linter issues before checking this in but you might have deduced that from these #976 #977. Thanks again

Ah, great 👍

@katiewasnothere
Copy link
Contributor

@slonopotamus could you rebase this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants