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

CI: fix Makefile not working with go 1.18 #800

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

chappjc
Copy link
Contributor

@chappjc chappjc commented Apr 9, 2022

go get is no longer used to install or build things. Use go install
instead. Also specify a version so the module's go.mod is not
touched when installing the CI tools.

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@Roasbeef
Copy link
Member

Approved for a CI run, should be automatic after this gets in (github new contributor clause).

@Roasbeef
Copy link
Member

I wonder if we'll run into this here? golang/go#44840

@guggero
Copy link
Collaborator

guggero commented Apr 19, 2022

It looks like you need to use at least golangci-lint v1.40.0 to get this fixed. But that will require a few of the new rules that were introduced in that version to be addressed as well.

@chappjc
Copy link
Contributor Author

chappjc commented Apr 19, 2022

I'll update the linter and fix whatever. Just a few and I'll be on it...

@chappjc
Copy link
Contributor Author

chappjc commented Apr 19, 2022

Oh nevermind... #795 (comment)

@guggero
Copy link
Collaborator

guggero commented Apr 19, 2022

Oh nevermind... #795 (comment)

Actually, I just found your PR instead that aims to fix it. So I didn't start on a PR myself.

@chappjc
Copy link
Contributor Author

chappjc commented Apr 19, 2022

Oh, haha, let me try latest golangci-lint locally and see what needs to update.

@chappjc
Copy link
Contributor Author

chappjc commented Apr 19, 2022

Yikes it's a LOT, at least if you use the default enabled linters.
I have a feeling most of those except perhaps errcheck, which is super annoying, are worth satisfying, but it looks to be a lot of work.

@chappjc
Copy link
Contributor Author

chappjc commented Apr 19, 2022

But the few lint issues thrown by the current version of golangci-lint are fixed in 1d1c003 (part of #783), now cherry-picked onto this PR too

@guggero
Copy link
Collaborator

guggero commented Apr 19, 2022

If a lot of errors come from the same (new) linter, I think an option might also be to disable the linter (see .golangci.yml) for now and then fix the issues in a later commit.

@chappjc
Copy link
Contributor Author

chappjc commented Apr 19, 2022

K, lemme see how many would need to be disabled for btcwallet using golangci-ling 1.45.2, which I think is needed for go 1.18 practically

@guggero
Copy link
Collaborator

guggero commented Apr 19, 2022

I tried and it looked like v1.40.0 is the oldest version that actually compiles with golang 1.18.

@chappjc
Copy link
Contributor Author

chappjc commented Apr 19, 2022

OK wow, I'd suggest whitelisting linters (--disable-all and selective --enable) instead of the blacklisting (--enable-all and selective --disable). There are more than 20 linters (so far) that are going to be extremely difficult to satisfy.

Alternative, use the 10 default linters and add any extras you want [deadcode errcheck gosimple govet ineffassign staticcheck structcheck typecheck unused varcheck]

1.45.0 was the big go 1.18 support release and there were panics before that version. https://github.com/golangci/golangci-lint/releases/tag/v1.45.0

just some that need to be disabled
    - errlint
    - paralleltest
    - exhaustive
    - nestif
    - gocognit
    - cyclop
    - tparallel
    - testpackage
    - gci
    - errname
    - godox
    - wrapcheck
    - varnamelen
    - gomnd
    - ifshort
    - ireturn
    - nilnil
    - nlreturn
    - stylecheck
    - goerr113
    - exhaustivestruct

go get is no longer used to install or build things. Use go install
instead.  Also specify a version so the module's go.mod is not
touched when installing the CI tools.
@Roasbeef
Copy link
Member

Approved another CI run...

@Roasbeef
Copy link
Member

CI looking pretty good...

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🏉

@Roasbeef Roasbeef merged commit 5a5be82 into btcsuite:master Apr 20, 2022
@chappjc chappjc deleted the goimports-fix branch April 27, 2022 17:50
@chappjc chappjc restored the goimports-fix branch April 27, 2022 17:50
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.

3 participants