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

depguard: fix GOROOT detection #3866

Merged
merged 1 commit into from
Jun 2, 2023
Merged

Conversation

ldez
Copy link
Member

@ldez ldez commented Jun 2, 2023

Following the plan that I explained here:

  • create a fork inside the golangci org
  • apply the fix
  • use the fork as a dependency (I will create a dedicated module to avoid replace directives, because replace directives are not transitive, so users that are using that use the tools pattern will not have a problem.)

Summary of issue #3862:
depguard uses build.Default.GOROOT but this value is filled correctly only if the GOROOT is defined through an env var.

$ go env | grep GOROOT
GOROOT="/usr/local/go"

$ env | grep GOROOT

$ export GOROOT=/usr/local/go

$ env | grep GOROOT
GOROOT=/usr/local/go

The "expander" cannot be overridden because it's a global variable inside an internal package, the compile method that produces the error and the analyzer constructor are not exported.


The fix I have done in our depguard fork is based on what is done inside golang.org/x/tools.

golangci/depguard@v2.0.1...ed68d37

When the PR OpenPeeDeeP/depguard#48 will be merged and when depguard will create a new release, we will drop this dependency (but we will have to keep the fork for compatibility.)

Fixes #3862

@ldez ldez added bug Something isn't working linter: update version Update version of linter labels Jun 2, 2023
@ldez ldez requested a review from bombsimon June 2, 2023 00:06
jacobbednarz added a commit to cloudflare/cloudflare-go that referenced this pull request Jun 2, 2023
Once v1.29 golangci/golangci-lint#3866 lands, we can revert this.
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.

LGTM!

Would an alternative be to roll back depguard instead to not have to keep a fork around forever? It doesn't really matter having it around so I'll approve, just curious.

yrobla added a commit to mindersec/minder that referenced this pull request Jun 2, 2023
Latest version came with problems with depguard:
OpenPeeDeeP/depguard#46

Current solution for now is pinning golangci-lint version
to do not use latest deps.

Fix is currently ongoing here:
golangci/golangci-lint#3866

But anyway it's good idea to have a pinned version and do not
consume from latest.
yrobla added a commit to mindersec/minder that referenced this pull request Jun 2, 2023
Latest version came with problems with depguard:
OpenPeeDeeP/depguard#46

Current solution for now is pinning golangci-lint version
to do not use latest deps.

Fix is currently ongoing here:
golangci/golangci-lint#3866

But anyway it's good idea to have a pinned version and do not
consume from latest.
@ldez
Copy link
Member Author

ldez commented Jun 2, 2023

I didn't roll back because the depguard v2 configuration is breaking and I think that we don't have to wait for more to break this, and because this new version of depguard fixes an annoying old bug with concurrency.

@ldez ldez merged commit dd7c3d1 into golangci:master Jun 2, 2023
@ldez ldez deleted the fix/depguard-goroot branch June 2, 2023 08:28
@elgohr
Copy link

elgohr commented Jun 2, 2023

@ldez looks like you have to convince the CI that this is okay... https://github.com/golangci/golangci-lint/actions/runs/5153483603/jobs/9280765338

@ldez
Copy link
Member Author

ldez commented Jun 2, 2023

It's just because we use the previous version inside the CI.

@nhatthm
Copy link

nhatthm commented Jun 2, 2023

@ldez Do you know how long it takes to get the fix populated in GHA? We are using the latest tag

@ldez
Copy link
Member Author

ldez commented Jun 2, 2023

the GitHub action is currently generating the GHA files https://github.com/golangci/golangci-lint/actions/runs/5153980997/jobs/9281874877

It will be available quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working linter: update version Update version of linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

depguard is enabled and fails on v1.53.0
5 participants