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

fix: add local imported packages to the packages to analyze when using cache #4424

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: add local imported packages to the packages to analyze when using cache #4424

wants to merge 2 commits into from
+22 −0

Conversation

ldez
Copy link
Member

@ldez ldez commented Feb 27, 2024 •
edited
Loading

To reproduce the problem:

git clone [email protected]:gabolaev/gl-issue-reproduction.git && cd $_
golangci-lint cache clean
git checkout HEAD .; golangci-lint run; sed -i 's/some text/SOME OTHER TEXT/g' somepackage/somefile.go; golangci-lint run
# git checkout HEAD .; ./golangci-lint run; sed -i 's/some text/SOME OTHER TEXT/g' somepackage/somefile.go; ./golangci-lint run

If it works you should see:

somepackage/somefile.go:9:2: printf: (*github.com/sirupsen/logrus.Logger).Warnf format %v reads arg #1, but call has 0 args (govet)
        log.Warnf("some text %v") // this line is supposed to be reported by govet printf linter
        ^
somepackage/somefile.go:9:2: printf: (*github.com/sirupsen/logrus.Logger).Warnf format %v reads arg #1, but call has 0 args (govet)
        log.Warnf("SOME OTHER TEXT %v") // this line is supposed to be reported by govet printf linter
        ^

but if it not works:

somepackage/somefile.go:9:2: printf: (*github.com/sirupsen/logrus.Logger).Warnf format %v reads arg #1, but call has 0 args (govet)
        log.Warnf("some text %v") // this line is supposed to be reported by govet printf linter
        ^

This will impact negatively the performance of a run with a cache but it will increase the stability of reports.

Fixes #4423

@ldez ldez added bug Something isn't working area: cache labels Feb 27, 2024
@ldez ldez force-pushed the fix/cache-imports branch 2 times, most recently from 849f21e to ccf27b4 Compare February 27, 2024 21:29
bombsimon
bombsimon approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

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

Did you do any tests (manual or otherwise) on a somewhat larger codebase and/or with a lot of local imports? Are we talking millisecond increases or can this become a cardinality bomb and grow much bigger than that?

The fix looks good and is understandable so I'll approve this since I have full trust in your judgement here 😄

@ldez
Copy link
Member Author

ldez commented Feb 28, 2024 •
edited
Loading

Did you do any tests (manual or otherwise) on a somewhat larger codebase and/or with a lot of local imports?

I didn't do a benchmark because this depends on how many direct packages a package depends on, so it's difficult to create accurate benchmarks.

Are we talking millisecond increases or can this become a cardinality bomb and grow much bigger than that?

As it just adds the direct packages related to one analyzed package and not the indirect packages, the cardinality is limited.

But if a package depends on thousands of local packages (I hope nobody does that), the impact will be important on performances (only for the run after the first one) but I don't have a better solution.

EDIT: I have some doubts maybe the real problem is the linter 🤔

gabolaev
gabolaev approved these changes Mar 1, 2024
View reviewed changes
Copy link

@gabolaev gabolaev left a comment

Choose a reason for hiding this comment

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

It works, thank you!

@ldez ldez removed the request for review from Antonboom March 1, 2024 12:43
@ldez ldez added the blocked Need's direct action from maintainer label Mar 1, 2024
@ldez ldez mentioned this pull request Mar 7, 2024
Open
4 tasks
@alexandear alexandear self-requested a review March 9, 2024 16:02
alexandear
alexandear approved these changes Mar 9, 2024
View reviewed changes
Copy link
Member

@alexandear alexandear left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexandear alexandear alexandear approved these changes

@bombsimon bombsimon bombsimon approved these changes

@gabolaev gabolaev gabolaev approved these changes

Assignees
No one assigned
Labels
area: cache blocked Need's direct action from maintainer bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

govet: printf unstable behavior with imported types
4 participants
@ldez @alexandear @bombsimon @gabolaev