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

go1.21 support #3922

Merged
merged 10 commits into from
Aug 9, 2023
Merged

go1.21 support #3922

merged 10 commits into from
Aug 9, 2023

Conversation

ldez
Copy link
Member

@ldez ldez commented Jun 21, 2023

This PR is to evaluate and prepare golangci-lint to go1.21.

This PR will evolve during the beta and RC phases of go1.21.

@ldez ldez added the enhancement New feature or improvement label Jun 21, 2023
@ldez ldez force-pushed the feat/go1.21 branch 2 times, most recently from f07147f to 78be407 Compare June 21, 2023 18:54
@ldez
Copy link
Member Author

ldez commented Jun 21, 2023

For now, there is a problem with go-critic, I will investigate to understand the problem.

Edit: I tested to run tests of go-critic itself with go1.21 and the problem is also here.
I don't know if it's a bug of go1.21 or a problem with go-critic/ruleguard.
go-critic/go-critic#1349
quasilyte/go-ruleguard#449

@pohly
Copy link
Contributor

pohly commented Jul 4, 2023

As mentioned on Slack: if Go 1.21rc2 is installed in a module, gocritic: load embedded ruleguard rules: rules/rules.go:13: can't load fmt: setting an explicit GOROOT can fix this problem. occurs again.

@ldez
Copy link
Member Author

ldez commented Aug 8, 2023

I disabled ruleguard (one "rule" inside gocritic), only with go1.21, I think it's the best move to be able to create a release quickly.

Note: the Go version should be 1.21 inside go.mod or be defined inside the run section:

go.mod
module your/module/name

go 1.21

//...
.golangci.yml
run:
    go: '1.21'
CLI flags
--go=1.21

@ldez ldez marked this pull request as ready for review August 8, 2023 20:21
@ldez ldez changed the title WIP: go1.21 support go1.21 support Aug 8, 2023
renevo added a commit to renevo/actor that referenced this pull request Aug 9, 2023
Day 1 issues, not yet supporting 1.21: golangci/golangci-lint#3922
renevo added a commit to renevo/actor that referenced this pull request Aug 9, 2023
* Adding slog

* Disable golangci-lint

Day 1 issues, not yet supporting 1.21: golangci/golangci-lint#3922
Copy link
Member

@SVilgelm SVilgelm left a comment

Choose a reason for hiding this comment

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

LGTM

@pohly
Copy link
Contributor

pohly commented Aug 9, 2023

Disabling ruleguard does not fix the problem in Kubernetes.

I first tried a config change:

linters-settings:
  gocritic:
    disabled-checks:
      - ruleguard               # unused

Then I bumped the golangci-lint in Kubernetes to ldez:feat/go1.21 and also still got the same problem:

ERROR: level=warning msg="plugin: 'AnalyzerPlugin' plugins are deprecated, please use the new plugin signature: https://golangci-lint.run/contributing/new-linters/#create-a-plugin"
ERROR: level=error msg="[linters_context] gocritic: load embedded ruleguard rules: rules/rules.go:13: can't load fmt: setting an explicit GOROOT can fix this problem."

If I had to guess, I would say that gocritic loads ruleguard rules regardless whether the ruleguard check is enabled or not (haven't checked).

@ldez
Copy link
Member Author

ldez commented Aug 9, 2023

@pohly From my memories, the problem was already here before go1.21.

If I had to guess, I would say that gocritic loads ruleguard rules regardless whether the ruleguard check is enabled or not (haven't checked).

The load of the embedded rules is done when gocritic is enabled.

err := checkers.InitEmbeddedRules()

@pohly
Copy link
Contributor

pohly commented Aug 9, 2023

Right, I also just found that. So to me it looks like all of gocritic is broken for Kubernetes because of this problem.

@pohly
Copy link
Contributor

pohly commented Aug 9, 2023

Disabling gocritic entirely makes golangci-lint usable with 1.21 in Kubernetes, but that would be a pity.

@pohly
Copy link
Contributor

pohly commented Aug 9, 2023

From my memories, the problem was already here before go1.21.

There was a different problem with having the Go toolchain inside the Go package. That got solved. Now go1.21 broke that again, with no solution so far.

@ldez
Copy link
Member Author

ldez commented Aug 9, 2023

Have you tried with go-critic itself (standalone CLI)?

@pohly
Copy link
Contributor

pohly commented Aug 9, 2023

go-critic works with go1.20.4 and go1.21rc4 when those are installed outside of Kubernetes. It also works with _output/local/.gimme/versions/go1.21rc3.linux.amd64/bin inside the Kubernetes repo but only if go-critic was compiled with go1.20.4.

However, go-critic fails once it is compiled with go1.21rc3, no matter where the Go installation is located when invoking it. I'm a bit surprised by that because I thought I had seen a difference before when trying it with golangi-lint - perhaps I had made a mistake when testing.

@ldez
Copy link
Member Author

ldez commented Aug 9, 2023

I would like to propose moving this discussion outside of this PR, maybe on Slack.

EDIT: after discussing on the Gopher Slack, we decided to open an issue on the go-critic repo: go-critic/go-critic#1359

@ldez ldez merged commit e0a8bd8 into golangci:master Aug 9, 2023
16 checks passed
@ldez ldez deleted the feat/go1.21 branch August 9, 2023 10:09
@Dasio
Copy link

Dasio commented Aug 16, 2023

Just FYI regarding https://github.com/golangci/golangci-lint/pull/3922/files#diff-15c806aa509538190832852f439e9921a23bec2da81f95ed0e4bf13c14e5b160R44

With latest version it is still built with 1.20

golangci-lint run --version
golangci-lint has version 1.54.1 built with go1.20.7 from a9378d9 on 2023-08-11T10:14:36Z

Because of this I got errors such as

../../../../../../../../opt/homebrew/Cellar/go/1.21.0/libexec/src/slices/sort.go:67:7: undefined: min (typecheck)
		m = min(m, x[i])

It's fixed if I compile it myself using go1.21

@ldez
Copy link
Member Author

ldez commented Aug 16, 2023

Just FYI regarding https://github.com/golangci/golangci-lint/pull/3922/files#diff-15c806aa509538190832852f439e9921a23bec2da81f95ed0e4bf13c14e5b160R44

This line is to run some tests with the previous version of golangci-lint.
It's not related to the build.

The build use go1.21:

go-version: '1.21'

In your log I can see opt/homebrew/Cellar/, I guess you are on Darwin, and maybe you are using Brew: Brew doesn't use our binary, it built the binary so it depends on how Brew built it.

The official binaries:

$ golangci-lint version
golangci-lint has version 1.54.1 built with go1.21.0 from a9378d9b on 2023-08-11T12:49:16Z

@Dasio
Copy link

Dasio commented Aug 16, 2023

Oh, you are right, I'm sorry, thanks

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

Successfully merging this pull request may close these issues.

None yet

4 participants