feat: fix ci / makefile #2320

rabbitprincess · 2025-02-12T01:43:51Z

Overview

fix / cleanup makefile and correcting several commands.
Related to #2319 #2316

Roasbeef

Thanks for this PR!

Changes look good for the most part. Main comment pertains to keeping the grandfathered linter commit

Roasbeef · 2025-02-12T02:08:42Z

.github/workflows/go.yml

        with:
          go-version: ${{ env.GO_VERSION }}

      - name: Check out source
-        uses: actions/checkout@v2
+        uses: actions/checkout@v4


Roasbeef · 2025-02-12T02:11:14Z

Makefile


 LINT_COMMIT := v1.18.0
-GOACC_COMMIT := 80342ae2e0fcf265e99e76bcc4efd022c7c3811b


So this is important, as it absolves any linter violations before that commit. Otherwise, we'll have to have a PR which fixes every linter violation.

In lnd, we use golangci which still supports this feature: https://github.com/lightningnetwork/lnd/pull/9382/files

Roasbeef · 2025-02-12T02:12:01Z

scripts/tidy_modules.sh

@@ -1,17 +0,0 @@
-#!/bin/bash
-
-SUBMODULES=$(find . -mindepth 2 -name "go.mod" | cut -d'/' -f2)


Why remove? Given that it's able to locate sub-modules automatically?

In the new diff, we need to enumerate them all by hand.

I just thought it was simple approach. Other commands, such as unit / unit-cover, are still done entirely by hand. It works perfectly as long as nested go.mod is not added.

Roasbeef · 2025-02-12T02:12:50Z

Makefile


-LINT = $(LINT_BIN) run -v $(LINT_WORKERS)
+LINT = $(LINT_BIN) run -v $(LINT_WORKERS) --timeout=$(LINT_TIMEOUT) --issues-exit-code=0


Why's issues-exit-code needed now?

It just exits with code 0 when a lint error occurs. It's not needed, so better to remove it.

resolved in bdf7318

Roasbeef · 2025-02-12T02:14:05Z

Makefile

@@ -14,8 +14,6 @@ GOTEST_DEV = go test -v -tags=$(DEV_TAGS)
 GOTEST := go test -v
 GOMODTIDY := go mod tidy

-GOFILES_NOVENDOR = $(shell find . -type f -name '*.go' -not -path "./vendor/*")


👍 it's been years since we vendor'd deps

rpcclient/chain_test.go

yyforyongyu · 2025-03-07T10:36:36Z

Is this still under development?

rabbitprincess · 2025-03-10T01:33:46Z

@yyforyongyu It is still in development, but it would be good to recreate the PR to follow Ideal Git Commit Structure.

yyforyongyu · 2025-03-10T06:50:02Z

@rabbitprincess Cool thanks - I think it's better to do a force push on this PR once you fix the commit structure, it's easier that way as we won't lose the context/comments from this PR.

coveralls · 2025-03-10T08:41:51Z

Pull Request Test Coverage Report for Build 16746440587

Details

0 of 0 changed or added relevant lines in 0 files are covered.
6 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.009%) to 54.8%

Files with Coverage Reduction	New Missed Lines	%
connmgr/connmanager.go	2	83.9%
database/ffldb/blockio.go	4	88.81%

Totals
Change from base Build 16582220057:	0.009%
Covered Lines:	31094
Relevant Lines:	56741

💛 - Coveralls

rabbitprincess · 2025-03-10T08:51:22Z

@yyforyongyu this pr and #2307 are force-pushed. can you review again?

GustavoStingelin

Hey @rabbitprincess, I tested your PR and ran into an issue during the lint step. It looks like the linter version used is very outdated (v1.18.0, released 6 years ago):

➜  btcd git:(feature/fix-ci) ✗ make lint
Fetching linter
go install -v github.com/golangci/golangci-lint/cmd/[email protected]
go: github.com/golangci/golangci-lint/cmd/[email protected]:
  The go.mod file for the module providing named packages contains one or
  more replace directives. It must not contain directives that would cause
  it to be interpreted differently than if it were the main module.
make: *** [Makefile:46: /home/head/go/bin/golangci-lint] Error 1

System info:

Linux 6.11.0-26-generic x86_64
Go version go1.24.2 linux/amd64

Other steps like make fmt and make unit-cover are working fine. I think we could consider bumping the linter version to something more recent.

rabbitprincess · 2025-06-12T10:07:55Z

@GustavoStingelin Sure! It's been updated to v2.1.6.

GustavoStingelin · 2025-06-12T19:48:49Z

LGTM!

@Roasbeef @yyforyongyu with the updated golangci-lint, we're now seeing 162 issues across several linters. Since the linter isn’t part of the pipeline yet, would you prefer merging this PR as-is and handling the fixes in follow-up PRs? Or should we resolve all the issues before merging?

Breakdown of issues:

errcheck: 50
ineffassign: 16
staticcheck: 50
unused: 46

GustavoStingelin · 2025-06-13T21:26:07Z

or maybe copy the .golangci.yml from lnd as mentioned previously

GustavoStingelin · 2025-07-01T14:30:05Z

@kcalvinalvin any thoughts?

Roasbeef · 2025-07-02T22:56:19Z

Since the linter isn’t part of the pipeline yet, would you prefer merging this PR as-is and handling the fixes in follow-up PRs?

Usually what we do is set a commit hash that grandfathers in any violations. So then only new PRs that introduce new linting errors get triggered.

Makefile

GustavoStingelin

@rabbitprincess please, add the file .golangci.yml to the root path with the value

version: "2"
issues:
  # Only show newly introduced problems.
  new-from-rev: 80b74d6c5a0088a66dc96df6777d21e15c04849c

rabbitprincess · 2025-07-11T09:40:25Z

@rabbitprincess please, add the file .golangci.yml to the root path with the value
version: "2"
issues:
  # Only show newly introduced problems.
  new-from-rev: 80b74d6c5a0088a66dc96df6777d21e15c04849c

Sure! It's been applied.

GustavoStingelin

tACK

GustavoStingelin · 2025-07-24T12:01:15Z

I think you will need to rebase your first commit with the changes from the last

rabbitprincess · 2025-07-25T03:31:27Z

@GustavoStingelin Sure. It's been applied.

Roasbeef

If I run make lint, I get this error:

⛰   make lint
 Linting source.
/Users/roasbeef/gocode/bin/golangci-lint run -v
panic: load embedded ruleguard rules: rules/rules.go:13: can't load fmt

goroutine 1 [running]:
github.com/go-critic/go-critic/checkers.init.22()
        /Users/roasbeef/gocode/pkg/mod/github.com/go-critic/[email protected]/checkers/embedded_rules.go:47 +0x494
make: *** [lint] Error 2

rabbitprincess · 2025-07-29T01:19:35Z

If I run make lint, I get this error:

⛰   make lint
 Linting source.
/Users/roasbeef/gocode/bin/golangci-lint run -v
panic: load embedded ruleguard rules: rules/rules.go:13: can't load fmt

goroutine 1 [running]:
github.com/go-critic/go-critic/checkers.init.22()
        /Users/roasbeef/gocode/pkg/mod/github.com/go-critic/[email protected]/checkers/embedded_rules.go:47 +0x494
make: *** [lint] Error 2

Can you check the output of go env?
It looks like go module might not be enabled in your environment (e.g., GO111MODULE=off )

GustavoStingelin · 2025-07-29T02:02:26Z

If I run make lint, I get this error:

FYI, make lint still works fine on my env (Go 1.24.4, Linux amd64, GO111MODULE='').

Roasbeef · 2025-07-31T17:58:17Z

Ah I'm on Go 1.23.6 locally. So seems that we'll need to update the top level go.mod to use Go 1.24.

Go 1.25 comes out in August, so that should line up with our policy to support the past two go versions.

GustavoStingelin · 2025-07-31T18:54:23Z

Ah I'm on Go 1.23.6 locally. So seems that we'll need to update the top level go.mod to use Go 1.24.

Go 1.25 comes out in August, so that should line up with our policy to support the past two go versions.

works well on version 1.23.6 here.

➜  btcd git:(feature/fix-ci) ✗ mise use [email protected]
mise ~/code/btcd/mise.toml tools: [email protected]

➜  btcd git:(feature/fix-ci) ✗ go version
go version go1.23.6 linux/amd64

➜  btcd git:(feature/fix-ci) ✗ rm $GOBIN/golangci-lint

➜  btcd git:(feature/fix-ci) ✗ make lint 
 Fetching linter
go install -v  github.com/golangci/golangci-lint/v2/cmd/[email protected]
 Linting source.
/home/head/.local/share/mise/installs/go/1.23.6/bin/golangci-lint run -v  --timeout=5m
INFO golangci-lint has version v2.1.6 built with go1.23.6 from (unknown, modified: ?, mod sum: "h1:LXqShFfAGM5BDzEOWD2SL1IzJAgUOqES/HRBsfKjI+w=") on (unknown) 
... omited logs
INFO [runner] linters took 116.4495ms with stages: goanalysis_metalinter: 38.478262ms 
0 issues.
INFO File cache stats: 0 entries of total size 0B 
INFO Memory: 5 samples, avg is 59.6MB, max is 94.0MB 
INFO Execution took 360.073736ms

GustavoStingelin · 2025-07-31T19:01:08Z

Makefile

 GOIMPORTS_PKG := golang.org/x/tools/cmd/goimports

-GO_BIN := ${GOPATH}/bin
+GO_BIN := ${shell go env GOPATH}/bin


Suggested change

GO_BIN := ${shell go env GOPATH}/bin

GO_BIN := ${shell go env GOBIN}

When testing with different Go versions, I found that go env GOBIN is more reliable than assuming $GOPATH/bin. In my case, using mise as a tool manager, GOBIN doesn't follow the traditional path structure.

Can I just commit this, or should I rebase it?

rebase bc your previous commit modifies this line and is in this context

@GustavoStingelin It's been applied.

cool, tACK in go1.24.5 and go1.23.1 linux/amd64

rabbitprincess marked this pull request as ready for review February 12, 2025 01:57

Roasbeef reviewed Feb 12, 2025

View reviewed changes

rabbitprincess force-pushed the feature/fix-ci branch from 479ef5a to 52363a8 Compare March 10, 2025 08:47

GustavoStingelin suggested changes Jun 9, 2025

View reviewed changes

rabbitprincess force-pushed the feature/fix-ci branch from 52363a8 to 80b74d6 Compare June 12, 2025 10:04

GustavoStingelin approved these changes Jun 24, 2025

View reviewed changes

Roasbeef requested changes Jul 2, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Roasbeef added this to the v0.25 milestone Jul 2, 2025

GustavoStingelin suggested changes Jul 10, 2025

View reviewed changes

GustavoStingelin approved these changes Jul 11, 2025

View reviewed changes

rabbitprincess force-pushed the feature/fix-ci branch from 3e1fbf3 to 770a77b Compare July 25, 2025 01:23

Roasbeef requested changes Jul 28, 2025

View reviewed changes

GustavoStingelin reviewed Jul 31, 2025

View reviewed changes

rabbitprincess added 3 commits August 5, 2025 18:39

Makefile: Update dependencies and improve installation commands

22b4663

multi: apply make fmt to all files

f190651

golangci: add configuration file for linting

106cde6

rabbitprincess force-pushed the feature/fix-ci branch from 770a77b to 106cde6 Compare August 5, 2025 09:40


		LINT_COMMIT := v1.18.0
		GOACC_COMMIT := 80342ae2e0fcf265e99e76bcc4efd022c7c3811b

		@@ -1,17 +0,0 @@
		#!/bin/bash

		SUBMODULES=$(find . -mindepth 2 -name "go.mod" \| cut -d'/' -f2)


		LINT = $(LINT_BIN) run -v $(LINT_WORKERS)
		LINT = $(LINT_BIN) run -v $(LINT_WORKERS) --timeout=$(LINT_TIMEOUT) --issues-exit-code=0

	GO_BIN := ${shell go env GOPATH}/bin
	GO_BIN := ${shell go env GOBIN}

feat: fix ci / makefile #2320

Are you sure you want to change the base?

feat: fix ci / makefile #2320

Uh oh!

Conversation

rabbitprincess commented Feb 12, 2025

Overview

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yyforyongyu commented Mar 7, 2025

Uh oh!

rabbitprincess commented Mar 10, 2025

Uh oh!

yyforyongyu commented Mar 10, 2025

Uh oh!

coveralls commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16746440587

Details

💛 - Coveralls

Uh oh!

rabbitprincess commented Mar 10, 2025

Uh oh!

GustavoStingelin left a comment

Choose a reason for hiding this comment

Uh oh!

rabbitprincess commented Jun 12, 2025

Uh oh!

GustavoStingelin commented Jun 12, 2025

Uh oh!

GustavoStingelin commented Jun 13, 2025

Uh oh!

GustavoStingelin commented Jul 1, 2025

Uh oh!

Roasbeef commented Jul 2, 2025

Uh oh!

Uh oh!

GustavoStingelin left a comment

Choose a reason for hiding this comment

Uh oh!

rabbitprincess commented Jul 11, 2025

Uh oh!

GustavoStingelin left a comment

Choose a reason for hiding this comment

Uh oh!

GustavoStingelin commented Jul 24, 2025

Uh oh!

rabbitprincess commented Jul 25, 2025

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

rabbitprincess commented Jul 29, 2025

Uh oh!

GustavoStingelin commented Jul 29, 2025

Uh oh!

Roasbeef commented Jul 31, 2025

Uh oh!

GustavoStingelin commented Jul 31, 2025

Uh oh!

Choose a reason for hiding this comment

coveralls commented Mar 10, 2025 •

edited

Loading