From 7f127d6e7c265bf3303f7866907271cb91fa50e5 Mon Sep 17 00:00:00 2001 From: Thomas Kohler Date: Sun, 19 Oct 2025 18:36:20 +0200 Subject: [PATCH] fmt: introduce golangci-lint and go vet use CI with action --- .github/workflows/golangci-lint-install.bak | 26 ++ .github/workflows/golangci-lint-matrix.yml | 59 ++++ .golangci.yml | 308 ++++++++++++++++++++ Makefile | 45 ++- adafruit4650/device_test.go | 1 + cmd/convert2bin/convert2bin.go | 2 +- hts221/hts221_generic.go | 2 - pcf8523/pcf8523.go | 1 + pcf8523/pcf8523_test.go | 1 + tmc2209/address.go | 2 +- 10 files changed, 441 insertions(+), 6 deletions(-) create mode 100644 .github/workflows/golangci-lint-install.bak create mode 100644 .github/workflows/golangci-lint-matrix.yml create mode 100644 .golangci.yml diff --git a/.github/workflows/golangci-lint-install.bak b/.github/workflows/golangci-lint-install.bak new file mode 100644 index 000000000..9c2a2f818 --- /dev/null +++ b/.github/workflows/golangci-lint-install.bak @@ -0,0 +1,26 @@ +name: golangci-lint +on: + push: + tags: + - v* + branches: + - dev + pull_request: +permissions: + contents: read + # Optional: allow read access to pull request. Use with `only-new-issues` option. + # pull-requests: read +jobs: + lint: + runs-on: ubuntu-latest + name: "Lint all" + steps: + - uses: actions/checkout@v5 + - uses: actions/setup-go@v6 + with: + go-version: '1.22' + cache: false + - name: "Install golangci-lint v2.5.0" + run: curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.5.0 + - name: "Run linter by calling 'make fmt_check'" + run: make fmt_check diff --git a/.github/workflows/golangci-lint-matrix.yml b/.github/workflows/golangci-lint-matrix.yml new file mode 100644 index 000000000..180a81379 --- /dev/null +++ b/.github/workflows/golangci-lint-matrix.yml @@ -0,0 +1,59 @@ +name: golangci-lint +on: + push: + tags: + - v* + branches: + - dev + pull_request: +permissions: + contents: read + # Optional: allow read access to pull request. Use with `only-new-issues` option. + # pull-requests: read +jobs: + collectpackages: + name: "Collect all packages to a list" + runs-on: ubuntu-latest + outputs: + packages: "${{ steps.create-package-list.outputs.packages }}" + steps: + - uses: actions/checkout@v5 + - name: "Collect by calling 'make print_collected_packages'" + id: create-package-list + run: | + pkgs=$(make print_collected_packages) + # replace space by comma, add double quotes and copy to output + echo "packages=[\"${pkgs// /\",\"}\"]" >> $GITHUB_OUTPUT + - name: "Debug the collected packages as JSON array" + run: echo "The collected packages are ${{ steps.create-package-list.outputs.packages }}" + + golangci: + needs: collectpackages + runs-on: ubuntu-latest + strategy: + matrix: + package: "${{ fromJson(needs.collectpackages.outputs.packages) }}" + name: "Lint ${{ matrix.package }}" + steps: + - uses: actions/checkout@v5 + - uses: actions/setup-go@v6 + with: + go-version: '1.22' + cache: false + - name: golangci-lint + uses: golangci/golangci-lint-action@v8 + with: + version: v2.5.0 + working-directory: ${{ matrix.package }} + + # Optional: golangci-lint command line arguments. + # Note: exclude arguments, e.g. --exclude-files="my_file", will not affect the "typecheck" linter, + # at least since v1.61.0 - use build tags instead. + #args: --exclude-files="platforms/digispark/digispark_adaptor.go" + + # Optional: show only new issues if it's a pull request. The default value is `false`. + # only-new-issues: true + + # Optional: if set to true then the all caching functionality will be complete disabled, + # takes precedence over all other caching options. + # skip-cache: true diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 000000000..d1a1453d0 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,308 @@ +# note: GolangCI-Lint also searches for config files in all directories from the directory of the first analyzed path up to the root. +version: "2" +run: + build-tags: + - utils + + # If set we pass it to "go list -mod={option}". From "go help modules": + # If invoked with -mod=readonly, the go command is disallowed from the implicit + # automatic updating of go.mod described above. Instead, it fails when any changes + # to go.mod are needed. This setting is most useful to check that go.mod does + # not need updates, such as in a continuous integration and testing system. + # If invoked with -mod=vendor, the go command assumes that the vendor + # directory holds the correct copies of dependencies and ignores + # the dependency descriptions in go.mod. + # + # Allowed values: readonly|vendor|mod + # By default, it isn't set. + modules-download-mode: readonly + + # https://golangci-lint.run/usage/linters/#enabled-by-default + # note: typecheck can not be disabled, it is used to check code compilation +linters: + default: all + disable: + # deprecated: none + - wsl # deprecated (since v2.2.0) due to: new major version. Replaced by wsl_v5 + # not used for this go version: none + # not used for any reason + - err113 # not used (we allow error creation at return statement) + - forbidigo # not used (we allow print statements) + - ginkgolinter # not needed (enforces standards of using ginkgo and gomega) + - godot # not used (seems to be counting peas) + - godox # not used (we have many TODOs) + - gosmopolitan # not needed (report i18n/l10n anti-patterns) + - importas # not needed yet (there is no alias rule at the moment) + - loggercheck # not needed (relates to kitlog, klog, logr, zap) + - mnd # good, but very annoying when writing drivers with bit shifting etc. + - noinlineerr # this violates best practices in other style guides + - paralleltest # not used + - promlinter # not needed (prometheus metrics naming) + - rowserrcheck # not needed (sql related) + - sqlclosecheck # not needed (sql related) + - testpackage # not needed, we use the same name for test package to have access to unexposed items + - varnamelen # we love short names, if useful + - zerologlint # not needed (related to zerolog package) + # can be automatically fixed by "-fix" + - embeddedstructfieldcheck + - gocritic + - misspell + - nlreturn + # important and fast to fix ASAP + - asciicheck # never thought that the "C" in ds18b20.go:22:2 is internally the Cyrillic capital letter "ES" (U+0421) + - dupl # rarely needed, mostly a mistake, should be justified manually + - dogsled # rarely needed, mostly a mistake, should be justified manually + - exptostd # usage of experimental packages should be justified manually + - funcorder # common style + - gochecknoglobals # such global vars are needed rarely, e.g. to prevent huge heap allocations, but should be justified manually + - gochecknoinits # we should really not use it (source of hard to find bugs), a manual justification is mandatory + - govet # mostly mistakes, should be justified manually + - inamedparam # interface declarations should really have the parameters named + - ineffassign # very important - unused variables complicate code review (the reviewer often overlooks other problems) + - iotamixing # protects against mistakes + - ireturn # rarely needed, e.g. for test setups or functional options (not desirable in this repo), but should be justified manually + - lll + - noctx # how to fix can be found online + - perfsprint # will reduce count of usage of fmt-package + - predeclared # we should really not use already declared Go-identifiers for our own functions/variables + - recvcheck # simple to repair + - staticcheck # mostly simple to repair, big benefit + - thelper + - unconvert + - unused # should be fixed, because reduce bugs + - unparam # simplifies code + - wastedassign # same as "ineffassign" + # important to fix ASAP, but maybe more effort + - errcheck # very important - leads to many trouble, when errors are dropped silently + - errorlint # hard to find bugs, if error wrapping is used + - exhaustive # if we allow incomplete usage of enum switch this should be justified manually, e.g. "bma42x.go:120:2" is a valid finding + - interfacebloat # should be justified manually + - nakedret # should be fixed together with "nonamedreturns" (possible source of bugs) + - nonamedreturns # should be fixed, because reduce bugs + # useful for the future + - cyclop # useful (better understandable code) + - errname # useful for common style + - exhaustruct # useful in general, but for device drivers too often done by intention than a mistake + - funlen # useful (reduce bugs, simplify code, better understandable code) + - forcetypeassert # useful (prevents panic, possibility to return error instead) + - gocognit # useful (better understandable code) + - godoclint # useful, we should try it later + - goheader # useful, if we introduce a common header (e.g. for copyright) + - gosec # very important (can lead to sporadic fails), but too many findings to fix it ASAP + - intrange # introduced with go 1.22, will simplify the range syntax (opinionated) + - nestif # useful (reduce bugs, simplify code, better understandable code) + - whitespace # more common style, but could become annoying + - wrapcheck # error strings from interface methods are very poor/generic and not very helpful for debugging problems + - wsl_v5 # more common style, but could become annoying + + exclusions: + generated: lax + presets: + - comments + - common-false-positives + - legacy + - std-error-handling + + settings: + depguard: + # Rules to apply. + # + # Variables: + # - File Variables + # you can still use and exclamation mark ! in front of a variable to say not to use it. + # Example !$test will match any file that is not a go test file. + # + # `$all` - matches all go files + # `$test` - matches all go test files + # + # - Package Variables + # + # `$gostd` - matches all of go's standard library (Pulled from `GOROOT`) + # + # Default: Only allow $gostd in all files. + rules: + main: + # Packages that are not allowed where the value is a suggestion. + deny: + - pkg: github.com/pkg/errors + desc: Should be replaced by standard lib errors package + + dupword: + # Keywords for detecting duplicate words. + # If this list is not empty, only the words defined in this list will be detected. + # Default: [] + keywords: + - the + - and + - a + + errorlint: + # Default: true + # %v should be used by default over %w, see https://github.com/uber-go/guide/blob/master/style.md#error-wrapping + errorf: false + # Permit more than 1 %w verb, valid per Go 1.20 (Requires errorf:true) + # Default: true + errorf-multi: false + + gocritic: + disabled-checks: + - assignOp # very opinionated + - appendAssign # mostly used by intention + + nolintlint: + # Enable to require an explanation of nonzero length after each nolint directive. + # Default: false + require-explanation: true + # Enable to require nolint directives to mention the specific linter being suppressed. + # Default: false + require-specific: true + + + revive: + # Revive handles the default rules in a way that can be unexpected: + # - If there are no explicit rules, the default rules are used. + # - If there is at least one explicit rule, the default rules are not used. + # Run `GL_DEBUG=revive golangci-lint run --enable-only=revive` to see default, all available rules, and enabled rules. + # We enable all available rules and disable explicitly. + # Default: false + enable-all-rules: true + # Because revive's error description is pretty good, we can decide to activate this linter-rules instead of other linters, see the + # list at beginning of this document + rules: + - name: add-constant + disabled: true # if needed, mnd is used instead + - name: argument-limit + disabled: true # TODO: should be activated after some code is fixed/justified, maybe already covered by other linters + - name: bare-return + disabled: true # nakedret is used instead + - name: bool-literal-in-expr + disabled: true # TODO: can be activated after some code is fixed/justified + - name: cognitive-complexity + disabled: true # if needed, gocognit is used instead + - name: comment-spacings + disabled: true # will be fixed by formatters, see below + - name: confusing-naming + disabled: true # for driver development function names, which differs only by capitalization, seems to be OK + - name: confusing-results + disabled: true # TODO: should be activated after some code is fixed/justified + - name: cyclomatic + disabled: true # cyclop is used instead + - name: deep-exit + disabled: true # TODO: should be activated after some code is fixed/justified + - name: early-return + disabled: true # TODO: should be activated after some code is fixed (helps avoiding deep nested blocks) + - name: empty-block + disabled: true # TODO: should be activated after some code is fixed/justified + - name: empty-lines + disabled: true # will be fixed by formatters, see below + - name: enforce-switch-style + disabled: true # TODO: should be activated after some code is fixed (reduce hard to find bugs) + - name: error-strings + disabled: true # other linters already detect those issues + - name: exported + disabled: true # TODO: should be activated after some code is fixed (helps to find better names) + - name: flag-parameter + disabled: true # sounds useful, but when working with drivers, we often need boolean flags for some configuration + - name: function-length + disabled: true # funlen is used instead + - name: function-result-limit + arguments: [5] # 4 is often needed, e.g. for accelerometers, TODO: 5 was chosen for one finding + - name: get-return + disabled: true # TODO: should be activated after some code is fixed (helps to find better names) + - name: identical-switch-branches + disabled: true # first finding was a false positive + - name: identical-ifelseif-branches + disabled: true # first finding was a false positive, TODO: try to activate it, seems to be useful + - name: if-return + disabled: true # TODO: can be activated after some code is fixed (shorter code) + - name: increment-decrement + disabled: true # TODO: code unification, but opinionated + - name: import-shadowing + disabled: true # TODO: should be activated after some code is fixed (protects against mistakes) + - name: indent-error-flow + disabled: true # TODO: should be activated after some code is fixed (less complex code) + - name: line-length-limit + disabled: true # lll is used instead + - name: max-public-structs + disabled: true # not useful for the driver development + - name: modifies-value-receiver + disabled: true # TODO: should be activated after some code is fixed/justified (protects against mistakes) + - name: nested-structs + disabled: true # TODO: should be activated after some code is fixed/justified (less complex code) + - name: package-comments + disabled: true # TODO: can be enabled after issues are fixed + - name: package-directory-mismatch + disabled: true # TODO: should be enabled after issues are fixed/justified (ensure common golang style) + - name: range + disabled: true # TODO: can be enabled after issues are fixed (simpler code) + - name: redefines-builtin-id + disabled: true # predeclared is used instead + - name: receiver-naming + disabled: true # already covered by other linters, TODO: recheck this setting after others are enabled + - name: time-date + disabled: true # TODO: can be enabled after issues are fixed/justified + - name: unchecked-type-assertion + disabled: true # forcetypeassert is used instead + - name: unexported-naming + disabled: true # TODO: should be enabled after issues are fixed (really bad style) + - name: unexported-return + disabled: true # in most cases this is done by intention and if not, compiler errors will show the mistake + - name: unhandled-error + disabled: true # errcheck is used instead + - name: unnecessary-format + disabled: true # TODO: should be enabled after issues are fixed (helps avoiding fmt package) + - name: unnecessary-stmt + disabled: true # TODO: should be enabled after issues are fixed + - name: unused-parameter + disabled: true # TODO: "unused" is used instead, but check again after unused is activated + - name: unused-receiver + disabled: true # TODO: "unused" is used instead, but check again after unused is activated + - name: use-any + disabled: true # TODO: code unification, but opinionated + - name: use-errors-new + disabled: true # TODO: should be enabled after issues are fixed (helps avoiding fmt package) + - name: use-fmt-print + disabled: true # no, we try to avoid the fmt package, whenever possible + - name: useless-break + disabled: true # TODO: should be enabled after issues are fixed, but only one of this or "unnecessary-stmt" is needed + - name: useless-fallthrough + disabled: true # TODO: should be enabled after issues are fixed + - name: var-declaration + disabled: true # already covered by other linters + - name: var-naming + disabled: true # TODO: maybe better by manual justification + # https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#unexported-return + # disable this rule, because sometimes it has its justification + #- name: unexported-return + # severity: warning + # disabled: true + + +formatters: + enable: + # TODO: can be automatically fixed by "-fix" + #- gci + #- gofmt + #- gofumpt + - goimports + + exclusions: + generated: lax + + settings: + gci: + # Section configuration to compare against. + # Section names are case-insensitive and may contain parameters in (). + # The default order of sections is `standard > default > custom > blank > dot`, + # If `custom-order` is `true`, it follows the order of `sections` option. + # Default: ["standard", "default"] + sections: + - standard # Standard section: captures all standard packages. + - default # Default section: contains all imports that could not be matched to another section type. + - prefix(tinygo.org/x/drivers/) # Custom section: groups all imports with the specified Prefix. + #- blank # Blank section: contains all blank imports. This section is not present unless explicitly enabled. + #- dot # Dot section: contains all dot imports. This section is not present unless explicitly enabled. + # Enable custom order of sections. + # If `true`, make the section order the same as the order of `sections`. + # Default: false + custom-order: true diff --git a/Makefile b/Makefile index c05ef645b..2ca742186 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,16 @@ +# sub-directory used for build +BUILD_DIR := build +# find all packages without any dependency to underlying tiny-go packages, e.g. "machine" or "device/arm" +BUILD_TAGS_CHECK := m5stack_core2,microbit,xiao_ble +ALL_WITHOUT_MACHINE := $(shell go list -e -tags $(BUILD_TAGS_CHECK) -f '{{.Dir}},{{.Deps}}' ./... | awk -F, '$$2 !~ /machine/ && $$2 !~ /device\/arm/ {print $$1}') +# exclude anything found in build output and "image" directory, and exclude some further folders, which contains problematic dependencies in sub-folders +EXCLUDE_PACKAGES = $(CURDIR)/$(BUILD_DIR)/% $(CURDIR)/image/% $(CURDIR)/touch $(CURDIR)/waveshare-epd +ALL_TO_CHECK := $(filter-out $(EXCLUDE_PACKAGES),$(ALL_WITHOUT_MACHINE)) + +.PHONY: clean fmt-check smoke-test unit-test test check fmt_check fmt_fix $(ALL_TO_CHECK) clean: - @rm -rf build + @rm -rf $(BUILD_DIR) FMT_PATHS = ./ @@ -9,7 +19,7 @@ fmt-check: XTENSA ?= 1 smoke-test: - @mkdir -p build + @mkdir -p $(BUILD_DIR) @go run ./smoketest.go -xtensa=$(XTENSA) smoketest.sh @@ -26,3 +36,34 @@ unit-test: @go test -v $(addprefix ./,$(TESTS)) test: clean fmt-check unit-test smoke-test + +fmt_quick_check: + @# a very fast check before build, but depends on accessibility of all imports + @# switch off the "stdmethods" analyzer is needed due to finding: + @# at24cx/at24cx.go:57:18: method WriteByte(eepromAddress uint16, value uint8) error should have signature WriteByte(byte) error + @# at24cx/at24cx.go:67:18: method ReadByte(eepromAddress uint16) (uint8, error) should have signature ReadByte() (byte, error) + @# switch off the "shift" analyzer is needed due to finding: + @#tmc5160/registers.go:1939:16: m.CUR_A (16 bits) too small for shift of 16 + @#tmc5160/registers.go:1996:16: m.X3 (8 bits) too small for shift of 27 + @#tmc5160/registers.go:1996:27: m.X2 (8 bits) too small for shift of 24 + @#tmc5160/registers.go:1996:38: m.X1 (8 bits) too small for shift of 21 + @#tmc5160/registers.go:1996:49: m.W3 (8 bits) too small for shift of 18 + @#tmc5160/registers.go:1996:60: m.W2 (8 bits) too small for shift of 16 + @#tmc5160/registers.go:1996:71: m.W1 (8 bits) too small for shift of 14 + @#tmc5160/registers.go:1996:82: m.W0 (8 bits) too small for shift of 12 + go vet -tags $(BUILD_TAGS_CHECK) -stdmethods=false -shift=false $(ALL_TO_CHECK) + +fmt_check: + @# a complete format check, but depends on accessibility of all imports + golangci-lint -v run $(ALL_TO_CHECK) + +fmt_fix: + @# an automatic reformat and complete format check, but depends on accessibility of all imports + @#TODO: activate when ready + @#gofumpt -l -w $(ALL_TO_CHECK) + golangci-lint -v run $(ALL_TO_CHECK) --fix + +print_collected_packages: + @#this target is used to unify mechanism in CI with the local one, see ".github/workflows/golangci-lint.yml" + @#we need additional exclude the root folder here, because will be checked recursive when used as working directory + @echo $(filter-out $(CURDIR),$(ALL_TO_CHECK)) diff --git a/adafruit4650/device_test.go b/adafruit4650/device_test.go index 9cf56f939..fffcdb230 100644 --- a/adafruit4650/device_test.go +++ b/adafruit4650/device_test.go @@ -12,6 +12,7 @@ import ( "os" "testing" "time" + "tinygo.org/x/drivers" "tinygo.org/x/tinyfont" "tinygo.org/x/tinyfont/freemono" diff --git a/cmd/convert2bin/convert2bin.go b/cmd/convert2bin/convert2bin.go index c7cfaa1f8..620d0acfe 100644 --- a/cmd/convert2bin/convert2bin.go +++ b/cmd/convert2bin/convert2bin.go @@ -19,7 +19,7 @@ func main() { func run(args []string) error { if len(args) < 2 { - return fmt.Errorf("usage: %s FILE") + return fmt.Errorf("usage: %s FILE", args[0]) } b, err := ioutil.ReadFile(args[1]) diff --git a/hts221/hts221_generic.go b/hts221/hts221_generic.go index a5d58f9ec..e1693d403 100644 --- a/hts221/hts221_generic.go +++ b/hts221/hts221_generic.go @@ -2,8 +2,6 @@ package hts221 -import "tinygo.org/x/drivers" - // Configure sets up the HTS221 device for communication. func (d *Device) Configure() { // read calibration data diff --git a/pcf8523/pcf8523.go b/pcf8523/pcf8523.go index 333c6e4cf..75eb0c15c 100644 --- a/pcf8523/pcf8523.go +++ b/pcf8523/pcf8523.go @@ -5,6 +5,7 @@ package pcf8523 import ( "time" + "tinygo.org/x/drivers" ) diff --git a/pcf8523/pcf8523_test.go b/pcf8523/pcf8523_test.go index ef103c4fb..f9c05033c 100644 --- a/pcf8523/pcf8523_test.go +++ b/pcf8523/pcf8523_test.go @@ -4,6 +4,7 @@ import ( "encoding/hex" "testing" "time" + "tinygo.org/x/drivers/tester" ) diff --git a/tmc2209/address.go b/tmc2209/address.go index d637f45f4..78c96d467 100644 --- a/tmc2209/address.go +++ b/tmc2209/address.go @@ -92,7 +92,7 @@ func ReadRegister(comm RegisterComm, driverIndex uint8, register uint8) (uint32, // Read the register value using the comm interface value, err := comm.ReadRegister(register, driverIndex) - log.Printf("Request read ", register, driverIndex, value) + log.Print("Request read ", register, driverIndex, value) if err != nil { return 0, err }