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

Simplify Makefiles, make golangci-lint work again #1615

Merged
merged 8 commits into from
May 25, 2023

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented May 24, 2023

This simplifies Makefiles, removing some old hacks, and makes golangci-linter work again. Please see individual commits for details.

Fixes: #1614

TODO (not in this PR):

  • re-add the disabled linters, going back to (at least) golangci-lint default set;
  • get rid of tests/tools entirely.

Fixes: 4e3bb7e
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Quite a few were missing, one (lint) was out of (alphabetic) order.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Remove some old time artifacts, such as:
 - GO111MODULES, GO_PROXY, and MOD_VENDOR (no longer needed since go 1.14);
 - GIT_BRANCH (not needed since 2016, commit d855327);
 - EPOCH_TEST_COMMIT (not needed since 2017, commit 230b9ab);

except that -mod=vendor is needed for go build in tests/tools.

Also remove:
 - PROJECT_ROOT (and use of "go list" for tests);
 - sources (go build is efficient so we can use it every time without
   using make to compare sources timestamps).

This makes makefiles less cluttered, and should also fix the following
golangci-lint error observed in CI:

> running gomoddirectives failed: failed to get module file: command
> go list: exit status 1: go: list -m cannot be used with GO111MODULE=off:
> if you are not using go modules it is suggested to disable this linter"

As a result, golangci-lint was effectively disabled.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Without the fix, building can complain like this:

> # golang.org/x/sys/unix
> vendor/golang.org/x/sys/unix/syscall.go:83:16: unsafe.Slice requires go1.17 or later (-lang was set to go1.16; check go.mod)

Fixes: a3204cf
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes the following issue with golangci-lint:

> ERRO Running error: no such linter "cyclop"

Instead of enabling all the linters and then disabling many of
them, start with the default set of enabled linters, and disable
some that are currently giving errors.

This should make upgrading golangci-lint easier, as it maintains a
sensitive set of linters enabled by default (and enabling all linters by
default means we will enable some new and unknown linters every time
we bump golangci-lint version).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Mostly for readability.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin marked this pull request as ready for review May 24, 2023 01:55
@kolyshkin
Copy link
Contributor Author

OK, golangci-lint is now working: https://cirrus-ci.com/task/6600612519870464 (check "Run test").

@kolyshkin kolyshkin mentioned this pull request May 24, 2023
@TomSweeneyRedHat
Copy link
Member

@cevich PTAL

@cevich
Copy link
Member

cevich commented May 24, 2023

Wow @kolyshkin this is really nice work and a very welcome set of improvements. The only thing I'm slightly unsure of is the GO111MODULES change...

@TomSweeneyRedHat Do we need to maintain compatibility with (very) old golang due to backport possibility for some ancient (still supported) release? I'm guessing not (they're probably pinned to specific commits), but wanted to double-check.

@TomSweeneyRedHat
Copy link
Member

Thanks for the eyeballs @cevich

As far as the old ancient Go goes, it's not outside of the realm of possibilities that we'd need to backport to an older version, but I'd say it's not very likely. I wouldn't make older go a gating factor for this nor any other PR.

Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

Thanks Tom. K, LGTM then.

@kolyshkin
Copy link
Contributor Author

Wow @kolyshkin this is really nice work and a very welcome set of improvements. The only thing I'm slightly unsure of is the GO111MODULES change...

This is not needed since about go 1.14 (which is kinda ancient), and we've already made changes like this in other github.com/containers/* repos, e.g. containers/common#993

@rhatdan
Copy link
Member

rhatdan commented May 25, 2023

LGTM

@rhatdan rhatdan merged commit d3c5f2a into containers:main May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ci: golangci-lint is not working
4 participants