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

add extra linters from gopls #4829

Merged
merged 6 commits into from
Apr 11, 2024
Merged

Conversation

tonistiigi
Copy link
Member

@tonistiigi tonistiigi commented Apr 9, 2024

depends on #4828

Some useful linters seem to be only available as part of gopls. I couldn't figure out a better way of including them as this addition to linters Dockerfile - only unusedparams has a main pkg in upstream.

@@ -5,6 +5,9 @@ ARG ALPINE_VERSION=3.19
ARG XX_VERSION=1.4.0
ARG PROTOLINT_VERSION=0.45.0
ARG GOLANGCI_LINT_VERSION=1.57.1
ARG GOPLS_VERSION=v0.20.0
# disabled: deprecated unusedvariable simplifyrange
Copy link
Member Author

Choose a reason for hiding this comment

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

simplifyrange can't be enabled as secrets.pb.go generated code invalidates it atm. and there doesn't seem to be a way to disable checks on these files like you can do in golangci-lint.

unusedvariable disabled because it is disabled in gopls by default as well. It doesn't actually match anything in buildkit.

deprecated should be manually checked periodically but I don't think we can keep it as a requirement in CI.

Copy link
Member

@crazy-max crazy-max Apr 9, 2024

Choose a reason for hiding this comment

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

deprecated should be manually checked periodically but I don't think we can keep it as a requirement in CI.

If you want this to be included on schedule event only on ci this is possible.

@@ -1,88 +1 @@
//go:build !linux
Copy link
Member Author

Choose a reason for hiding this comment

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

@profnandaa When you are adding runc wcow support you likely need to add this file back, but atm I didn't find any way this file can be called. Only importer from cmd/oci_worker already had linux-only build tags.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Noted, thanks for letting me know.

@tonistiigi tonistiigi force-pushed the gopls-linters branch 2 times, most recently from 00edd3c to 80a5ce1 Compare April 9, 2024 01:13
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi tonistiigi marked this pull request as ready for review April 9, 2024 14:23
@tonistiigi
Copy link
Member Author

The new CI job is quite long with all the platform combinations added. Options:

  • Split the platforms into multiple bake targets and separate from current lint matrix
  • Move to separate bake target with fewer platforms
  • Disable some more linters that don't currently have an effect (only unusedparams, overflows and stdmethods found issues in current codebase).

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

LGTM

I can look at ci stuff as follow-up

@@ -1,3 +1,6 @@
//go:build linux
// +build linux
Copy link
Member

Choose a reason for hiding this comment

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

Can we just use the _linux.go suffix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is it better in this case where there are no other files with other suffixes?

Btw, this is so that go vet ./... does not try to build this package for windows.

@tonistiigi
Copy link
Member Author

@AkihiroSuda Is this ready to merge?

@AkihiroSuda AkihiroSuda merged commit 5a19fb4 into moby:master Apr 11, 2024
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants