Fix failed_prerequisites error #944

ksoichiro · 2020-01-27T17:16:02Z

Hi,
I investigated some similar issues - #827, #840, #885, #824, #888, ... - and I found:

unused linter is not combined into goanalysis_metalinter
because unused linter's load mode is LoadModeWholeProgram,
so two linters are executed by default.
in this situation, unused linter and goanalysis_metalinter shares the package information.
when unused linter is executed, packages in the linter context seems to change.
goanalysis_metalinter uses the changed packages in part of the analysis,
while "typecheck" linter in goanalysis_metalinter parses files by itself,
which results in error of type checking for dependent packages
by their inconsistency.
the execution order of the linters randomly change because it is
determined by range of map, this is why we do not always encounter these issues.

In this PR, I fixed this problem by cloning loaded packages in linter context
before the linter is executed to avoid sharing changed packages to another linter.
Cloning packages might not be the best solution from the perspective of
efficiency such as memory usage, but I think it's better than causing occasional failures.
UPDATE:
I found that the changed fields of the Package are Types, TypesInfo and Syntax, so I updated my changes to just clear those fields before the analysis.

claassistantio · 2020-01-27T17:16:30Z

All committers have signed the CLA.

…m bug golangci/golangci-lint#944

…ms found by the tool (#131) * [redhat] Fix a couple of typo in the JSON struct tags * [cmd2cve] Remove unused field member * [rpm] Make ToWFN actually propagate the error * [redhat] Remove unused field in struct * [rbs2nvd] Remove unusued const The UserAgent is setup as a string litteral a few lines below. * [ci]: Enable a few linter passes with golangci-lint * [ci] Remove the typecheck pass to work around a golangci-lint upstream bug golangci/golangci-lint#944 * ci: Allow the go: master build to fail

pkg/lint/runner.go

ryancurrah · 2020-01-28T13:35:59Z

~~When testing this fix I still ran into the problem...~~

WARN [runner] Can't run linter goanalysis_metalinter: fact_purity: failed prerequisites: buildssa@package/name

EDIT:
I apologize I installed the binary incorrectly. It does appear to be working as expected! I tested on a bunch of packages and I could not reproduce the issue.

Here is some stats from running golangci-lint on the same packages with the different builds. I ran go clean -cache before running each time.

github.com/golangci/golangci-lint@master:

INFO File cache stats: 151 entries of total size 1.9MiB
INFO Memory: 465 samples, avg is 69.0MB, max is 71.8MB
INFO Execution took 46.390626784s

github.com/ksoichiro/golangci-lint@fix-failed_prerequisites-error:

INFO File cache stats: 151 entries of total size 1.9MiB
INFO Memory: 450 samples, avg is 69.5MB, max is 71.4MB
INFO Execution took 44.842126638s

ernado · 2020-01-30T11:25:54Z

Can someone please run this version on kubernetes sources? I'm getting OOM-ed every time, but I'm not sure why.

ksoichiro · 2020-01-30T15:59:58Z

@ernado I've tried kubernetes source.
It detected so much errors, but there is no Can't run linter error.
(Please let me know if the operation is wrong.)

❯ $GOPATH/src/github.com/golangci/golangci-lint/golangci-lint cache clean && $GOPATH/src/github.com/golangci/golangci-lint/golangci-lint run ./... -v --timeout 10m
INFO [config_reader] Config search paths: [./ /Users/ksoichiro/go/src/github.com/kubernetes/kubernetes /Users/ksoichiro/go/src/github.com/kubernetes /Users/ksoichiro/go/src/github.com /Users/ksoichiro/go/src /Users/ksoichiro/go /Users/ksoichiro /Users /]
INFO [lintersdb] Active 10 linters: [deadcode errcheck gosimple govet ineffassign staticcheck structcheck typecheck unused varcheck]
INFO [lintersdb] Active 10 linters: [deadcode errcheck gosimple govet ineffassign staticcheck structcheck typecheck unused varcheck]
INFO [loader] Go packages loading at mode 575 (exports_file|files|name|deps|imports|types_sizes|compiled_files) took 12.00884315s
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 235.790891ms
INFO [runner/unused/goanalysis] analyzers took 15m43.699562512s with top 10 stages: buildssa: 14m54.826819572s, U1000: 48.87274294s
INFO [runner/goanalysis_metalinter/goanalysis] analyzers took 27m1.390177516s with top 10 stages: buildssa: 17m14.357674875s, fact_deprecated: 1m10.083183026s, inspect: 1m2.058881098s, ctrlflow: 33.819457748s, printf: 33.279269241s, fact_purity: 20.031427095s, ineffassign: 15.990560273s, vrp: 12.456149333s, deadcode: 6.853674289s, SA4018: 6.805596867s
...
INFO [runner/max_from_linter] 908/958 issues from linter errcheck were hidden, use --max-issues-per-linter
INFO [runner/max_from_linter] 139/189 issues from linter unused were hidden, use --max-issues-per-linter
INFO [runner/max_from_linter] 53/103 issues from linter gosimple were hidden, use --max-issues-per-linter
INFO [runner] Issues before processing: 3611, after processing: 247
INFO [runner] Processors filtering stat (out/in): filename_unadjuster: 3611/3611, skip_files: 3611/3611, autogenerated_exclude: 3581/3581, exclude-rules: 2730/2730, max_per_file_from_linter: 2511/2511, exclude: 2730/3581, max_from_linter: 247/1347, path_shortener: 247/247, path_prettifier: 3611/3611, identifier_marker: 3581/3581, nolint: 2730/2730, uniq_by_line: 2511/2730, diff: 2511/2511, max_same_issues: 1347/2511, source_code: 247/247, cgo: 3611/3611, skip_dirs: 3581/3611
INFO [runner] processing took 881.149348ms with stages: nolint: 583.597739ms, exclude: 125.628809ms, identifier_marker: 56.02235ms, path_prettifier: 51.280154ms, autogenerated_exclude: 41.31407ms, skip_dirs: 8.684261ms, max_same_issues: 7.238401ms, source_code: 6.018803ms, uniq_by_line: 543.411µs, cgo: 323.985µs, filename_unadjuster: 231.768µs, max_per_file_from_linter: 130.402µs, max_from_linter: 96.805µs, path_shortener: 37.262µs, skip_files: 425ns, diff: 403ns, exclude-rules: 300ns
INFO [runner] linters took 2m52.64497749s with stages: goanalysis_metalinter: 1m39.08785664s, unused: 1m12.675826379s
...
INFO File cache stats: 125 entries of total size 2.3MiB
INFO Memory: 643 samples, avg is 33909.5MB, max is 51655.9MB
INFO Execution took 3m4.925527s

ernado · 2020-01-30T16:05:56Z

Thank you, @ksoichiro

The 33GB is pretty big (but can be just because of unused linter), can you please also check current released version for any RAM consumption regressions?

If the memory consumption is nearly the same, LGTM

ksoichiro · 2020-01-31T00:07:52Z

Thank you for your review @ernado.
I also tried with golangci-lint v1.23.1.

INFO File cache stats: 153 entries of total size 2.9MiB
INFO Memory: 533 samples, avg is 3690.6MB, max is 40248.3MB
INFO Execution took 2m19.878515621s

It seems average memory increased so much with my fix.
I'll look into it later.

ernado · 2020-01-31T00:10:01Z

Thank you @ksoichiro!
Please make sure that both runs are with cold cache, it can affect memory usage.

ksoichiro · 2020-02-01T08:45:26Z

Sorry, I checked again and I found my comparison was not accurate.
I think this PR does not increase memory usage.

As I explained above, the execution order of linters is non-deterministic.
If unused linter is executed before goanalysis_metalinter, average memory usage seems so high, and on v1.23.1 lint is not executed completely by failed prerequisites error.
If goanalysis_metalinter is executed first and unused linter is executed next, then the average memory usage is not so high.
I don't know why the average seems so high, but this behavior is not changed by this fix.
Either way, max memory consumption is so high on kubernetes source (73409068c on master) mainly because of unused linter.

Here is the result. I cleared cache before each execution.

No.	linter execution order	golangci-lint version	memory (avg)	memory (max)	time	remarks
1	unused → goanalysis_metalinter	v1.23.1	42GB	58GB	2m39s	*1
2	unused → goanalysis_metalinter	this PR	40GB	54GB	2m49s
3	goanalysis_metalinter → unused	v1.23.1	5GB	39GB	2m17s
4	goanalysis_metalinter → unused	this PR	5GB	40GB	2m27s
5	unused	v1.23.1	11GB	40GB	1m8s	*2
6	unused	this PR	9GB	38GB	1m25s	*2
7	goanalysis_metalinter	v1.23.1	2GB	5GB	55s	*3
8	goanalysis_metalinter	this PR	2GB	5GB	56s	*3

*1: interrupted by failed prerequisites error
*2: executed with option: --disable-all -E unused
*3: executed with option: -D unused

ksoichiro · 2020-02-02T08:06:35Z

FYI, high max memory usage of unused linter seems to be seen first in 6a979fb (#699).
I think it should be handled in a separate issue/PR.

ernado

Looks like no memory usage regression, so LGTM.

Ensure that `unused` is always the last in execution order. It can speed up packages loading a bit. Refactor enabled linters set to remove extra logging. Relates: #944

Fix failed_prerequisites error

bb41e54

ksoichiro mentioned this pull request Jan 27, 2020

Can't run linter goanalysis_metalinter: failed prerequisites #827

Closed

3 tasks

nono mentioned this pull request Jan 27, 2020

WIP: Fix unused linter #914

Closed

dlespiau mentioned this pull request Jan 27, 2020

Enable some static checking with golangci-lint and fixes a few problems found by the tool facebookincubator/nvdtools#131

Merged

dlespiau added a commit to facebookincubator/nvdtools that referenced this pull request Jan 27, 2020

[ci] Remove the typecheck pass to work around a golangci-lint upstrea…

9e1b0f9

…m bug golangci/golangci-lint#944

ryancurrah reviewed Jan 27, 2020

View reviewed changes

pkg/lint/runner.go Outdated Show resolved Hide resolved

ryancurrah reviewed Jan 28, 2020

View reviewed changes

pkg/lint/runner.go Outdated Show resolved Hide resolved

Clear part of package fields before analysis

796b4ff

ernado assigned jirfag Jan 28, 2020

ernado approved these changes Feb 2, 2020

View reviewed changes

ernado merged commit 5999fb0 into golangci:master Feb 2, 2020

ksoichiro deleted the fix-failed_prerequisites-error branch February 3, 2020 00:05

jirfag added a commit that referenced this pull request May 3, 2020

clear types only after unused running

4e3bd6d

Ensure that `unused` is always the last in execution order. It can speed up packages loading a bit. Refactor enabled linters set to remove extra logging. Relates: #944

jirfag added a commit that referenced this pull request May 3, 2020

clear types only after unused running

8d63291

Ensure that `unused` is always the last in execution order. It can speed up packages loading a bit. Refactor enabled linters set to remove extra logging. Relates: #944

jirfag added a commit that referenced this pull request May 5, 2020

clear types only after unused running

07a6667

Ensure that `unused` is always the last in execution order. It can speed up packages loading a bit. Refactor enabled linters set to remove extra logging. Relates: #944

jirfag added a commit that referenced this pull request May 5, 2020

speed up a bit (#1064)

279b6d6

Ensure that `unused` is always the last in execution order. It can speed up packages loading a bit. Refactor enabled linters set to remove extra logging. Relates: #944

ldez added this to the v1.23 milestone Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failed_prerequisites error #944

Fix failed_prerequisites error #944

ksoichiro commented Jan 27, 2020 •

edited

Loading

claassistantio commented Jan 27, 2020 •

edited

Loading

ryancurrah commented Jan 28, 2020 •

edited

Loading

ernado commented Jan 30, 2020

ksoichiro commented Jan 30, 2020

ernado commented Jan 30, 2020 •

edited

Loading

ksoichiro commented Jan 31, 2020

ernado commented Jan 31, 2020

ksoichiro commented Feb 1, 2020

ksoichiro commented Feb 2, 2020

ernado left a comment

Fix failed_prerequisites error #944

Fix failed_prerequisites error #944

Conversation

ksoichiro commented Jan 27, 2020 • edited Loading

claassistantio commented Jan 27, 2020 • edited Loading

ryancurrah commented Jan 28, 2020 • edited Loading

ernado commented Jan 30, 2020

ksoichiro commented Jan 30, 2020

ernado commented Jan 30, 2020 • edited Loading

ksoichiro commented Jan 31, 2020

ernado commented Jan 31, 2020

ksoichiro commented Feb 1, 2020

ksoichiro commented Feb 2, 2020

ernado left a comment

Choose a reason for hiding this comment

ksoichiro commented Jan 27, 2020 •

edited

Loading

claassistantio commented Jan 27, 2020 •

edited

Loading

ryancurrah commented Jan 28, 2020 •

edited

Loading

ernado commented Jan 30, 2020 •

edited

Loading