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

revive complains about missing package comment since golangci-lint 1.44.1 #2610

Closed
4 tasks done
horgh opened this issue Feb 22, 2022 · 8 comments · Fixed by #2611
Closed
4 tasks done

revive complains about missing package comment since golangci-lint 1.44.1 #2610

horgh opened this issue Feb 22, 2022 · 8 comments · Fixed by #2611
Labels
bug Something isn't working

Comments

@horgh
Copy link

horgh commented Feb 22, 2022

Welcome

  • Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
  • Yes, I've searched similar issues on GitHub and didn't find any.
  • Yes, I've included all information below (version, config, etc).
  • Yes, I've tried with the standalone linter if available. (https://golangci-lint.run/usage/linters/)

Description of the problem

I am seeing errors like this since updating to golangci-lint 1.44.2. I tested 1.44.1 and the issue is there as well. 1.44.0 is fine. I also tried running revive 1.1.4 standalone and I could not reproduce it.

pkg/geoipupdate/version.go:1:1: package-comments: should have a package comment, unless it's in another file for this package (revive)
package geoipupdate

// Version defines current geoipupdate version.
const Version = "4.9.0"

For example in this test run in my project: https://github.com/maxmind/geoipupdate/runs/5292690662?check_suite_focus=true

The packages in question have a package comment in another file in the package.

I also noticed it complained that main did not have a package comment, which doesn't seem necessary either, though seems less of an issue.

Version of golangci-lint

wstorey@penguin:~/geoipupdate (horgh/lint)$ golangci-lint version
golangci-lint has version 1.44.2 built from d58dbde5 on 2022-02-17T20:58:06Z

Configuration file

[run]
  deadline = "10m"

  tests = true

[linters]
  disable-all = true
  enable = [
    "asciicheck",
    "bidichk",
    "bodyclose",
    "containedctx",
    "contextcheck",
    "deadcode",
    "depguard",
    "durationcheck",
    "errcheck",
    # Seems to cause stack overflows :-(
    # "errchkjson",
    "errname",
    "errorlint",
    "exhaustive",
    "exportloopref",
    "forbidigo",
    # We tried this liner but most places we do forced type asserts are
    # pretty safe, e.g., an atomic.Value when everything is encapsulated
    # in a small package.
    # "forcetypeassert",
    "goconst",
    "gocyclo",
    "gocritic",
    "godot",
    "gofumpt",
    "gomodguard",
    "gosec",
    "gosimple",
    "govet",
    "grouper",
    "ineffassign",
    "lll",
    "makezero",
    # Maintainability Index. Seems like it could be a good idea, but a
    # lot of things fail and we would need to make some decisions about
    # what to allow.
    # "maintidx",
    "misspell",
    "nakedret",
    "nilerr",
    "noctx",
    "nolintlint",
    "predeclared",
    "revive",
    "rowserrcheck",
    # https://github.com/golangci/golangci-lint/issues/287
    # "safesql",
    "sqlclosecheck",
    "staticcheck",
    "structcheck",
    "stylecheck",
    "tenv",
    "tparallel",
    "typecheck",
    "unconvert",
    "unparam",
    "unused",
    "varcheck",
    "vetshadow",
    "wastedassign",
    "wrapcheck",
  ]

# Please note that we only use depguard for stdlib as gomodguard only
# supports modules currently. See https://github.com/ryancurrah/gomodguard/issues/12
[linters-settings.depguard]
  list-type = "blacklist"
  include-go-root = true
  packages = [
    # We should use github.com/pkg/errors instead
    "errors",
  ]

[linters-settings.errcheck]
    # Ignoring Close so that we don't have to have a bunch of
    # `defer func() { _ = r.Close() }()` constructs when we
    # don't actually care about the error.
    ignore = "Close,fmt:.*"

[linters-settings.errorlint]
    errorf = true
    asserts = true
    comparison = true

[linters-settings.exhaustive]
    default-signifies-exhaustive = true

[linters-settings.gocritic]
    enabled-checks = [
        "appendAssign",
        "appendCombine",
        "argOrder",
        "assignOp",
        "badCall",
        "badCond",
        "badLock",
        "badRegexp",
        "badSorting",
        "boolExprSimplify",
        "builtinShadow",
        "builtinShadowDecl",
        "captLocal",
        "caseOrder",
        "codegenComment",
        "commentedOutCode",
        "commentedOutImport",
        "commentFormatting",
        "defaultCaseOrder",
        # Revive's defer rule already captures this. This caught no extra cases.
        # "deferInLoop",
        "deferUnlambda",
        "deprecatedComment",
        "docStub",
        "dupArg",
        "dupBranchBody",
        "dupCase",
        "dupImport",
        "dupSubExpr",
        "dynamicFmtString",
        "elseif",
        "emptyDecl",
        "emptyFallthrough",
        "emptyStringTest",
        "equalFold",
        "evalOrder",
        "exitAfterDefer",
        "exposedSyncMutex",
        "externalErrorReassign",
        # Given that all of our code runs on Linux and the / separate should
        # work fine, this seems less important.
        # "filepathJoin",
        "flagDeref",
        "flagName",
        "hexLiteral",
        "ifElseChain",
        "importShadow",
        "indexAlloc",
        "initClause",
        "mapKey",
        "methodExprCall",
        "nestingReduce",
        "newDeref",
        "nilValReturn",
        "octalLiteral",
        "offBy1",
        "paramTypeCombine",
        "preferDecodeRune",
        "preferFilepathJoin",
        "preferFprint",
        "preferStringWriter",
        "preferWriteByte",
        "ptrToRefParam",
        "rangeExprCopy",
        "rangeValCopy",
        "redundantSprint",
        "regexpMust",
        "regexpPattern",
        # This might be good, but I don't think we want to encourage
        # significant changes to regexes as we port stuff from Perl.
        # "regexpSimplify",
        "ruleguard",
        "singleCaseSwitch",
        "sliceClear",
        "sloppyLen",
        # This seems like it might also be good, but a lot of existing code
        # fails.
        # "sloppyReassign",
        "returnAfterHttpError",
        "sloppyTypeAssert",
        "sortSlice",
        "sprintfQuotedString",
        "sqlQuery",
        "stringXbytes",
        "switchTrue",
        "syncMapLoadAndDelete",
        "timeExprSimplify",
        "tooManyResultsChecker",
        "truncateCmp",
        "typeAssertChain",
        "typeDefFirst",
        "typeSwitchVar",
        "typeUnparen",
        "underef",
        "unlabelStmt",
        "unlambda",
        # I am not sure we would want this linter and a lot of existing
        # code fails.
        # "unnamedResult",
        "unnecessaryBlock",
        "unnecessaryDefer",
        "unslice",
        "valSwap",
        "weakCond",
        "wrapperFunc",
        "yodaStyleExpr",
        # This requires explanations for "nolint" directives. This would be
        # nice for gosec ones, but I am not sure we want it generally unless
        # we can get the false positive rate lower.
        # "whyNoLint"
    ]

[linters-settings.gofumpt]
    extra-rules = true
    lang-version = "1.16"

[linters-settings.gomodguard]
  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/avct/uasurfer"]
    recommendations = ["github.com/xavivars/uasurfer"]
    reason = "The original avct module appears abandoned."

  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/BurntSushi/toml"]
    recommendations = ["github.com/pelletier/go-toml"]
    reason = "This library panics frequently on invalid input."

  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/gofrs/uuid"]
    recommendations = ["github.com/google/uuid"]

  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/satori/go.uuid"]
    recommendations = ["github.com/google/uuid"]

  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/lib/pq"]
    recommendations = ["github.com/jackc/pgx"]
    reason = "This library is no longer actively maintained."

  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/neilotoole/errgroup"]
    recommendations = ["golang.org/x/sync/errgroup"]
    reason = "This library can lead to subtle deadlocks in certain use cases."

  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/pariz/gountries"]
    reason = "This library's data is not actively maintained. Use GeoInfo data."

  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/RackSec/srslog"]
    recommendations = ["github.com/RackSec/srslog"]
    reason = "This library's data is not actively maintained."

  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/ua-parser/uap-go/uaparser"]
    recommendations = ["github.com/xavivars/uasurfer"]
    reason = "The performance of this library is absolutely abysmal."

  [[linters-settings.gomodguard.blocked.modules]]
  [linters-settings.gomodguard.blocked.modules."github.com/ugorji/go/codec"]
    recommendations = ["encoding/json", "github.com/mailru/easyjson"]
    reason = "This library is poorly maintained. We should default to using encoding/json and use easyjson where performance really matters."

  [[linters-settings.gomodguard.blocked.versions]]
  [linters-settings.gomodguard.blocked.versions."github.com/jackc/pgx"]
    version = "< 4.0.0"
    reason = "Use github.com/jackc/pgx/v4"

[linters-settings.govet]
    "enable-all" = true

[linters-settings.lll]
    line-length = 120
    tab-width = 4

[linters-settings.nolintlint]
    allow-leading-space = false
    allow-unused = false
    allow-no-explanation = ["lll", "misspell"]
    require-explanation = true
    require-specific = true

[linters-settings.revive]
    ignore-generated-header = true
    severity = "warning"

    # This might be nice but it is so common that it is hard
    # to enable.
    # [[linters-settings.revive.rules]]
    # name = "add-constant"

    # [[linters-settings.revive.rules]]
    # name = "argument-limit"

    [[linters-settings.revive.rules]]
    name = "atomic"

    [[linters-settings.revive.rules]]
    name = "bare-return"

    [[linters-settings.revive.rules]]
    name = "blank-imports"

    [[linters-settings.revive.rules]]
    name = "bool-literal-in-expr"

    [[linters-settings.revive.rules]]
    name = "call-to-gc"

    # [[linters-settings.revive.rules]]
    # name = "cognitive-complexity"

    # Probably a good rule, but we have a lot of names that
    # only have case differences.
    # [[linters-settings.revive.rules]]
    # name = "confusing-naming"

    [[linters-settings.revive.rules]]
    name = "confusing-results"

    [[linters-settings.revive.rules]]
    name = "constant-logical-expr"

    [[linters-settings.revive.rules]]
    name = "context-as-argument"

    [[linters-settings.revive.rules]]
    name = "context-keys-type"

    # [[linters-settings.revive.rules]]
    # name = "cyclomatic"

    [[linters-settings.revive.rules]]
    name = "deep-exit"

    [[linters-settings.revive.rules]]
    name = "defer"

    [[linters-settings.revive.rules]]
    name = "dot-imports"

    [[linters-settings.revive.rules]]
    name = "duplicated-imports"

    [[linters-settings.revive.rules]]
    name = "early-return"

    [[linters-settings.revive.rules]]
    name = "empty-block"

    [[linters-settings.revive.rules]]
    name = "empty-lines"

    [[linters-settings.revive.rules]]
    name = "errorf"

    [[linters-settings.revive.rules]]
    name = "error-naming"

    [[linters-settings.revive.rules]]
    name = "error-return"

    [[linters-settings.revive.rules]]
    name = "error-strings"

    [[linters-settings.revive.rules]]
    name = "exported"

    # [[linters-settings.revive.rules]]
    # name = "file-header"

    # We have a lot of flag parameters. This linter probably makes
    # a good point, but we would need some cleanup or a lot of nolints.
    # [[linters-settings.revive.rules]]
    # name = "flag-parameter"

    # [[linters-settings.revive.rules]]
    # name = "function-result-limit"

    [[linters-settings.revive.rules]]
    name = "get-return"

    [[linters-settings.revive.rules]]
    name = "identical-branches"

    [[linters-settings.revive.rules]]
    name = "if-return"

    [[linters-settings.revive.rules]]
    name = "imports-blacklist"

    [[linters-settings.revive.rules]]
    name = "import-shadowing"

    [[linters-settings.revive.rules]]
    name = "increment-decrement"

    [[linters-settings.revive.rules]]
    name = "indent-error-flow"

    # [[linters-settings.revive.rules]]
    # name = "line-length-limit"

    # [[linters-settings.revive.rules]]
    # name = "max-public-structs"

    [[linters-settings.revive.rules]]
    name = "modifies-parameter"

    [[linters-settings.revive.rules]]
    name = "modifies-value-receiver"

    # We frequently use nested structs, particularly in tests.
    # [[linters-settings.revive.rules]]
    # name = "nested-structs"

    [[linters-settings.revive.rules]]
    name = "optimize-operands-order"

    [[linters-settings.revive.rules]]
    name = "package-comments"

    [[linters-settings.revive.rules]]
    name = "range"

    [[linters-settings.revive.rules]]
    name = "range-val-address"

    [[linters-settings.revive.rules]]
    name = "range-val-in-closure"

    [[linters-settings.revive.rules]]
    name = "receiver-naming"

    [[linters-settings.revive.rules]]
    name = "redefines-builtin-id"

    [[linters-settings.revive.rules]]
    name = "string-of-int"

    [[linters-settings.revive.rules]]
    name = "struct-tag"

    [[linters-settings.revive.rules]]
    name = "superfluous-else"

    [[linters-settings.revive.rules]]
    name = "time-naming"

    [[linters-settings.revive.rules]]
    name = "unconditional-recursion"

    [[linters-settings.revive.rules]]
    name = "unexported-naming"

    [[linters-settings.revive.rules]]
    name = "unexported-return"

    # This is covered elsewhere and we want to ignore some
    # functions such as fmt.Fprintf.
    # [[linters-settings.revive.rules]]
    # name = "unhandled-error"

    [[linters-settings.revive.rules]]
    name = "unnecessary-stmt"

    [[linters-settings.revive.rules]]
    name = "unreachable-code"

    [[linters-settings.revive.rules]]
    name = "unused-parameter"

    # We generally have unused receivers in tests for meeting the
    # requirements of an interface.
    # [[linters-settings.revive.rules]]
    # name = "unused-receiver"

    [[linters-settings.revive.rules]]
    name = "useless-break"

    [[linters-settings.revive.rules]]
    name = "var-declaration"

    [[linters-settings.revive.rules]]
    name = "var-naming"

    [[linters-settings.revive.rules]]
    name = "waitgroup-by-value"

[linters-settings.unparam]
    check-exported = true

[linters-settings.wrapcheck]
    "ignoreSigs" = [
        ".Errorf(",
        "errgroup.NewMultiError(",
        "errors.New(",
        ".Wait(",
        ".WithMessage(",
        ".WithMessagef(",
        ".WithStack(",
        ".Wrap(",
        ".Wrapf(",
        "v4.Retry(",
        "v4.RetryNotify(",
    ]

[issues]
exclude-use-default = false

  # This goes off for MD5 usage, which we use heavily
  [[issues.exclude-rules]]
  text = "weak cryptographic primitive"
  linters = ["gosec"]

  [[issues.exclude-rules]]
  linters = [
    "gocritic"
  ]
  # For some reason the imports stuff in ruleguard doesn't work in golangci-lint.
  # Perhaps it has an outdated version or something
  path = "_test.go"
  text = "ruleguard: Prefer the alternative Context method instead"

  [[issues.exclude-rules]]
  linters = [
    "gocritic"
  ]
  ## The nolintlint linter behaves oddly with ruleguard rules
  source = "// *no-ruleguard"


  [[issues.exclude-rules]]
  linters = [
    "gosec"
  ]
  # G306 - "Expect WriteFile permissions to be 0600 or less".
  text = "G306"

  [[issues.exclude-rules]]
  linters = [
    "govet"
  ]
  # we want to enable almost all govet rules. It is easier to just filter out
  # the ones we don't want:
  #
  # * fieldalignment - way too noisy. Although it is very useful in particular
  #   cases where we are trying to use as little memory as possible, having
  #   it go off on every struct isn't helpful.
  # * shadow - although often useful, it complains about _many_ err
  #   shadowing assignments and some others where shadowing is clear.
  text = "^(fieldalignment|shadow)"

  [[issues.exclude-rules]]
  linters = [
    "govet"
  ]
  text = "shadow: declaration of \"err\" shadows declaration"

  [[issues.exclude-rules]]
  linters = [
    "contextcheck",
    "nilerr",
    "wrapcheck",
  ]
  path = "_test.go"

  [[issues.exclude-rules]]
  linters = [
    "stylecheck",
  ]
  # ST1016 - methods on the same type should have the same receiver name.
  #    easyjson doesn't interact well with this.
  text = "ST1016"

  [[issues.exclude-rules]]
  linters = [
    "staticcheck",
  ]
  # SA5008: unknown JSON option "intern" - easyjson specific option.
  text = 'SA5008: unknown JSON option "intern"'

  [[issues.exclude-rules]]
  linters = [
    "wrapcheck"
  ]
  text = "github.com/maxmind/geoipupdate"

  [[issues.exclude-rules]]
  linters = [
    "wrapcheck",
  ]
  path = "_easyjson.go"

  [[issues.exclude-rules]]
  linters = [
    "gocritic",
  ]
  source = "Chmod|WriteFile"
  text = "octalLiteral"

Go environment

go version go1.17.7 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/wstorey/.cache/go-build"
GOENV="/home/wstorey/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/wstorey/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/wstorey/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/wstorey/bin/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/wstorey/bin/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.7"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/wstorey/geoipupdate/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3541699771=/tmp/go-build -gno-record-gcc-switches"

Verbose output of running

wstorey@penguin:~/geoipupdate (horgh/lint)$ golangci-lint cache clean
wstorey@penguin:~/geoipupdate (horgh/lint)$ golangci-lint run -v
INFO [config_reader] Config search paths: [./ /home/wstorey/geoipupdate /home/wstorey /home /]
INFO [config_reader] Used config file .golangci.toml
INFO [lintersdb] Active 48 linters: [asciicheck bidichk bodyclose containedctx contextcheck deadcode depguard durationcheck errcheck errname errorlint exhaustive exportloopref forbidigo goconst gocritic gocyclo godot gofumpt gomodguard gosec gosimple govet grouper ineffassign lll makezero misspell nakedret nilerr noctx nolintlint predeclared revive rowserrcheck sqlclosecheck staticcheck structcheck stylecheck tenv tparallel typecheck unconvert unparam unused varcheck wastedassign wrapcheck]
INFO [loader] Go packages loading at mode 575 (exports_file|files|imports|name|types_sizes|deps|compiled_files) took 161.49081ms
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 2.043217ms
INFO [linters context/goanalysis] analyzers took 16.690070895s with top 10 stages: gocritic: 4.987684727s, buildir: 3.814613442s, buildssa: 654.912324ms, wastedassign: 549.040957ms, the_only_name: 543.453052ms, exhaustive: 525.883617ms, bidichk: 407.544995ms, inspect: 322.505771ms, godot: 300.879013ms, nilness: 290.722731ms
INFO [runner/max_same_issues] 5/8 issues with text "package-comments: should have a package comment, unless it's in another file for this package" were hidden, use --max-same-issues
INFO [runner] Issues before processing: 60, after processing: 3
INFO [runner] Processors filtering stat (out/in): max_from_linter: 3/3, filename_unadjuster: 60/60, exclude: 60/60, nolint: 8/34, diff: 8/8, path_prefixer: 3/3, skip_files: 60/60, skip_dirs: 60/60, identifier_marker: 60/60, severity-rules: 3/3, max_same_issues: 3/8, path_shortener: 3/3, sort_results: 3/3, cgo: 60/60, path_prettifier: 60/60, autogenerated_exclude: 60/60, exclude-rules: 34/60, uniq_by_line: 8/8, max_per_file_from_linter: 8/8, source_code: 3/3
INFO [runner] processing took 5.865401ms with stages: nolint: 3.022995ms, identifier_marker: 1.077587ms, exclude-rules: 828.492µs, path_prettifier: 401.658µs, autogenerated_exclude: 294.529µs, source_code: 113.184µs, skip_dirs: 64.366µs, max_same_issues: 40.425µs, uniq_by_line: 7.827µs, cgo: 5.178µs, filename_unadjuster: 2.707µs, path_shortener: 1.513µs, max_from_linter: 1.338µs, max_per_file_from_linter: 1.159µs, skip_files: 826ns, diff: 528ns, severity-rules: 428ns, exclude: 291ns, sort_results: 268ns, path_prefixer: 102ns
INFO [runner] linters took 4.50578492s with stages: goanalysis_metalinter: 4.499824686s
cmd/geoipupdate/args.go:1:1: package-comments: should have a package comment, unless it's in another file for this package (revive)
package main

import (
        "log"
        "os"

        flag "github.com/spf13/pflag"
)

// Args are command line arguments.
type Args struct {
        ConfigFile        string
        DatabaseDirectory string
        StackTrace        bool
        Verbose           bool
}

func getArgs() *Args {
        configFile := flag.StringP(
                "config-file",
                "f",
                defaultConfigFile,
                "Configuration file",
        )
        databaseDirectory := flag.StringP(
                "database-directory",
                "d",
                "",
                "Store databases in this directory (uses config if not specified)",
        )
        help := flag.BoolP("help", "h", false, "Display help and exit")
        stackTrace := flag.Bool("stack-trace", false, "Show a stack trace along with any error message.")
        verbose := flag.BoolP("verbose", "v", false, "Use verbose output")
        displayVersion := flag.BoolP("version", "V", false, "Display the version and exit")

        flag.Parse()

        if *help {
                printUsage()
        }
        if *displayVersion {
                log.Printf("geoipupdate %s", version)
                //nolint: revive // deep exit from main package
                os.Exit(0)
        }

        if *configFile == "" {
                log.Printf("You must provide a configuration file.")
                printUsage()
        }

        return &Args{
                ConfigFile:        *configFile,
                DatabaseDirectory: *databaseDirectory,
                StackTrace:        *stackTrace,
                Verbose:           *verbose,
        }
}

func printUsage() {
        log.Printf("Usage: %s <arguments>\n", os.Args[0])
        flag.PrintDefaults()
        //nolint: revive // deep exit from main package
        os.Exit(1)
}
cmd/geoipupdate/version.go:4:1: package-comments: should have a package comment, unless it's in another file for this package (revive)
package main

import "runtime/debug"

func init() {
        if info, ok := debug.ReadBuildInfo(); ok && info.Main.Version != "(devel)" {
                version = info.Main.Version
        }
}
pkg/geoipupdate/database/local_file_writer.go:1:1: package-comments: should have a package comment, unless it's in another file for this package (revive)
package database

import (
        "crypto/md5"
        "fmt"
        "hash"
        "io"
        "log"
        "os"
        "path/filepath"
        "strings"
        "time"

        "github.com/gofrs/flock"
        "github.com/pkg/errors"
)

// LocalFileDatabaseWriter is a database.Writer that stores the database to the
// local file system.
type LocalFileDatabaseWriter struct {
        filePath      string
        lockFilePath  string
        verbose       bool
        lock          *flock.Flock
        oldHash       string
        fileWriter    io.Writer
        temporaryFile *os.File
        md5Writer     hash.Hash
}

// NewLocalFileDatabaseWriter create a LocalFileDatabaseWriter. It creates the
// necessary lock and temporary files to protect the database from concurrent
// writes.
func NewLocalFileDatabaseWriter(filePath, lockFilePath string, verbose bool) (*LocalFileDatabaseWriter, error) {
        dbWriter := &LocalFileDatabaseWriter{
                filePath:     filePath,
                lockFilePath: lockFilePath,
                verbose:      verbose,
        }

        var err error
        if dbWriter.lock, err = CreateLockFile(lockFilePath, verbose); err != nil {
                return nil, err
        }
        if err = dbWriter.createOldMD5Hash(); err != nil {
                return nil, err
        }

        temporaryFilename := fmt.Sprintf("%s.temporary", dbWriter.filePath)
        //nolint:gosec // We want the permission to be world readable
        dbWriter.temporaryFile, err = os.OpenFile(
                temporaryFilename,
                os.O_WRONLY|os.O_CREATE|os.O_TRUNC,
                0o644,
        )
        if err != nil {
                return nil, errors.Wrap(err, "error creating temporary file")
        }
        dbWriter.md5Writer = md5.New()
        dbWriter.fileWriter = io.MultiWriter(dbWriter.md5Writer, dbWriter.temporaryFile)

        return dbWriter, nil
}

func (writer *LocalFileDatabaseWriter) createOldMD5Hash() error {
        currentDatabaseFile, err := os.Open(writer.filePath)
        if err != nil {
                if os.IsNotExist(err) {
                        writer.oldHash = ZeroMD5
                        return nil
                }
                return errors.Wrap(err, "error opening database")
        }

        defer func() {
                err := currentDatabaseFile.Close()
                if err != nil {
                        log.Println(errors.Wrap(err, "error closing database"))
                }
        }()
        oldHash := md5.New()
        if _, err := io.Copy(oldHash, currentDatabaseFile); err != nil {
                return errors.Wrap(err, "error calculating database hash")
        }
        writer.oldHash = fmt.Sprintf("%x", oldHash.Sum(nil))
        if writer.verbose {
                log.Printf("Calculated MD5 sum for %s: %s", writer.filePath, writer.oldHash)
        }
        return nil
}

// Write writes to the temporary file.
func (writer *LocalFileDatabaseWriter) Write(p []byte) (int, error) {
        n, err := writer.fileWriter.Write(p)
        if err != nil {
                return 0, errors.Wrap(err, "error writing")
        }
        return n, nil
}

// Close closes the temporary file and releases the file lock.
func (writer *LocalFileDatabaseWriter) Close() error {
        err := writer.temporaryFile.Close()
        if err != nil {
                var perr *os.PathError
                if !errors.As(err, &perr) || !errors.Is(perr.Err, os.ErrClosed) {
                        return errors.Wrap(err, "error closing temporary file")
                }
        }

        if err := os.Remove(writer.temporaryFile.Name()); err != nil && !os.IsNotExist(err) {
                return errors.Wrap(err, "error removing temporary file")
        }
        if err := writer.lock.Unlock(); err != nil {
                return errors.Wrap(err, "error releasing lock file")
        }
        return nil
}

// ValidHash checks that the temporary file's MD5 matches the given hash.
func (writer *LocalFileDatabaseWriter) ValidHash(expectedHash string) error {
        actualHash := fmt.Sprintf("%x", writer.md5Writer.Sum(nil))
        if !strings.EqualFold(actualHash, expectedHash) {
                return errors.Errorf("md5 of new database (%s) does not match expected md5 (%s)", actualHash, expectedHash)
        }
        return nil
}

// SetFileModificationTime sets the database's file access and modified times
// to the given time.
func (writer *LocalFileDatabaseWriter) SetFileModificationTime(lastModified time.Time) error {
        if err := os.Chtimes(writer.filePath, lastModified, lastModified); err != nil {
                return errors.Wrap(err, "error setting times on file")
        }
        return nil
}

// Commit renames the temporary file to the name of the database file and syncs
// the directory.
func (writer *LocalFileDatabaseWriter) Commit() error {
        if err := writer.temporaryFile.Sync(); err != nil {
                return errors.Wrap(err, "error syncing temporary file")
        }
        if err := writer.temporaryFile.Close(); err != nil {
                return errors.Wrap(err, "error closing temporary file")
        }
        if err := os.Rename(writer.temporaryFile.Name(), writer.filePath); err != nil {
                return errors.Wrap(err, "error moving database into place")
        }

        // fsync the directory. http://austingroupbugs.net/view.php?id=672
        dh, err := os.Open(filepath.Dir(writer.filePath))
        if err != nil {
                return errors.Wrap(err, "error opening database directory")
        }

        // We ignore Sync errors as they primarily happen on file systems that do
        // not support sync.
        _ = dh.Sync()

        if err := dh.Close(); err != nil {
                return errors.Wrap(err, "closing directory")
        }
        return nil
}

// GetHash returns the hash of the current database file.
func (writer *LocalFileDatabaseWriter) GetHash() string {
        return writer.oldHash
}
INFO File cache stats: 22 entries of total size 59.6KiB
INFO Memory: 47 samples, avg is 279.3MB, max is 547.8MB
INFO Execution took 4.687589506s

Code example or link to a public repository

https://github.com/maxmind/geoipupdate

@horgh horgh added the bug Something isn't working label Feb 22, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 22, 2022

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

@butuzov
Copy link
Member

butuzov commented Feb 22, 2022

You have 'package-comments' enabled check, but if it's just not works as it suppose to I suggest to visit 'revive' linter repo, and leave bug report there.

@butuzov butuzov added declined and removed bug Something isn't working labels Feb 22, 2022
@butuzov butuzov closed this as completed Feb 22, 2022
@horgh
Copy link
Author

horgh commented Feb 22, 2022

Thanks for looking! I agree it sounds like revive might have an issue, but see this output which makes me think it might be how golangci-lint is using it:

wstorey@penguin:~/foo$ ls
bar.go  foo.go  go.mod

wstorey@penguin:~/foo$ cat bar.go
package foo

// Bar returns bar.
func Bar() string { return "bar" }

wstorey@penguin:~/foo$ cat foo.go
// Package foo is a nice package.
package foo

// Foo returns foo.
func Foo() string { return "foo" }

wstorey@penguin:~/foo$ golangci-lint version
golangci-lint has version 1.44.2 built from d58dbde5 on 2022-02-17T20:58:06Z

wstorey@penguin:~/foo$ golangci-lint run
bar.go:1:1: package-comments: should have a package comment, unless it's in another file for this package (revive)
package foo

// Bar returns bar.
func Bar() string { return "bar" }

wstorey@penguin:~/foo$ ~/t/golangci-lint-1.44.0-linux-amd64/golangci-lint version
golangci-lint has version 1.44.0 built from 617470fa on 2022-01-25T11:31:17Z

wstorey@penguin:~/foo$ ~/t/golangci-lint-1.44.0-linux-amd64/golangci-lint run
wstorey@penguin:~/foo$ 

and

wstorey@penguin:~/foo$ ~/t/revive -version
Version:        1.1.4
Commit:         d4fbc9244093baaa4cdbcc310fde154a01dfc172
Built           2022-02-15T22:59:06Z by goreleaser
wstorey@penguin:~/foo$ ~/t/revive ./...
wstorey@penguin:~/foo$ 

@ldez ldez added question Further information is requested and removed declined labels Feb 22, 2022
@ldez
Copy link
Member

ldez commented Feb 22, 2022

I changed your sample (removed the package comment in Foo.go), I recreate a quick revive configuration file.
And it seems that the package-comments doesn't work with revive as a binary.

$ cat revive.toml                                                               
ignoreGeneratedHeader = false
severity = "warning"
confidence = 0.8
errorCode = 1
warningCode = 2


[rule.package-comments]
$ revive -config revive.toml ./...

@butuzov
Copy link
Member

butuzov commented Feb 22, 2022

@ldez can you check with confidence 0.1?

@ldez
Copy link
Member

ldez commented Feb 22, 2022

Yes, my problem was related to confidence.

cat revive.toml
ignoreGeneratedHeader = false
severity = "error"
confidence = 0.2
errorCode = 1
warningCode = 2

[rule.package-comments]
$ revive -config revive.toml ./...
foo.go:1:1: should have a package comment, unless it's in another file for this package
bar.go:1:1: should have a package comment, unless it's in another file for this package

And with the original example:

revive -config revive.toml ./...
bar.go:1:1: should have a package comment, unless it's in another file for this package

So the weird behavior is not related to golangci-lint.

@ldez
Copy link
Member

ldez commented Feb 22, 2022

The difference between v1.44.0 and v1.44.1 is related to the default confidence.
I will fix that.

@ldez ldez added bug Something isn't working and removed question Further information is requested labels Feb 22, 2022
@ldez ldez reopened this Feb 22, 2022
@ldez ldez added question Further information is requested bug Something isn't working and removed bug Something isn't working question Further information is requested labels Feb 22, 2022
@ldez
Copy link
Member

ldez commented Feb 22, 2022

the workaround is to define confidence to 0.8.

[linters-settings.revive]
    severity = "warning"
    confidence = 0.8
linters-settings:
  revive:
    severity: warning
    confidence: 0.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants