diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
index f03bcba8..599e840c 100644
--- a/.github/workflows/go.yml
+++ b/.github/workflows/go.yml
@@ -24,8 +24,8 @@ jobs:
- name: Run go vet
run: go vet ./...
- name: Run go fmt
- run: go fmt ./...
+ run: go fmt ./...
- name: Run go test
- run: go test -v ./...
+ run: go test -v ./...
- name: Run go build
run: go build ./...
\ No newline at end of file
diff --git a/Dockerfile b/Dockerfile
index 342290f9..66f297ad 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -11,8 +11,6 @@ RUN CGO_ENABLED=0 GOOS=linux go build -o /reviewbot .
# install staticcheck lint tools
RUN GOPATH=/go go install honnef.co/go/tools/cmd/staticcheck@2023.1.6
-RUN apt-get update && apt-get install -y cppcheck
-
FROM aslan-spock-register.qiniu.io/library/ubuntu:22.04 as runner
RUN apt-get update && apt-get install -y ca-certificates \
@@ -26,6 +24,9 @@ RUN apt-get update && apt-get install -y luarocks \
&& luarocks install luacheck \
&& rm -rf /var/lib/apt/lists/*
+# install cppcheck lint tools
+RUN apt-get update && apt-get install -y cppcheck \
+ && rm -rf /var/lib/apt/lists/*
# 设置golang环境
ENV GOLANG_DOWNLOAD_URL https://go.dev/dl/go1.21.5.linux-amd64.tar.gz
diff --git a/config/config.yaml b/config/config.yaml
index 2c324044..a412a2f8 100644
--- a/config/config.yaml
+++ b/config/config.yaml
@@ -4,7 +4,7 @@
# qbox:
# staticcheck:
# enable: false
-#
+
# example2: disable staticcheck for repo
# qbox/kodo:
# staticcheck:
@@ -26,20 +26,4 @@ qbox/kodo: # github repository name, which will override the settings in qbox or
workDir: "src/qiniu.com/kodo"
command: staticcheck
args: ["-debug.no-compile-errors=true", "./..."]
- run_if_changed: ["src/qiniu.com/kodo/.*\\.go$"] # only run if changed files match the regex
-
-qbox/3rd-ffmpeg: # github repository name, which will override the settings in qbox org
- cppcheck: # repo specific settings will override the settings in qbox org
- enable: true
- command: cppcheck
- args: []
- run_if_changed: [".*\\.c$"] # only run if changed files match the regex
-
-jiangpeipei327/reviewbottest: # github repository name, which will override the settings in qbox org
- cppcheck: # repo specific settings will override the settings in qbox org
- enable: true
- command: cppcheck
- args: []
- run_if_changed: ["*.cpp"] # only run if changed files match the regex
- staticcheck: # repo specific settings will override the settings in qbox org
- enable: false
\ No newline at end of file
+ run_if_changed: ["src/qiniu.com/kodo/.*\\.go$"] # only run if changed files match the regex
\ No newline at end of file
diff --git a/internal/linters/c/cppcheck/README.md b/internal/linters/c/cppcheck/README.md
new file mode 100644
index 00000000..bc2d85a2
--- /dev/null
+++ b/internal/linters/c/cppcheck/README.md
@@ -0,0 +1,12 @@
+# Cppcheck - A tool for static C/C++ code analysis
+Syntax:
+ cppcheck [OPTIONS] [files or paths]
+If a directory is given instead of a filename, *.cpp, *.cxx, *.cc, *.c++, *.c, *.ipp, *.ixx, *.tpp, and *.txx files are checked recursively from the given directory.
+
+
+Example usage in this repo:
+Recursively check the current folder, format the error messages as file name:line number:column number: warning message and don't print progress:
+cppcheck --quiet --template='{file}:{line}:{column}: {message}' .
+
+For more information:
+ https://files.cppchecksolutions.com/manual.pdf
diff --git a/internal/linters/c/cppcheck/cppcheck.go b/internal/linters/c/cppcheck/cppcheck.go
index d466de09..ad8a7477 100644
--- a/internal/linters/c/cppcheck/cppcheck.go
+++ b/internal/linters/c/cppcheck/cppcheck.go
@@ -1,85 +1,40 @@
package cppcheck
import (
- "bytes"
- "os/exec"
+ "strings"
- "github.com/google/go-github/v57/github"
- "github.com/qiniu/x/log"
+ "github.com/qiniu/reviewbot/internal/linters"
"github.com/qiniu/x/xlog"
- "github.com/reviewbot/config"
- "github.com/reviewbot/internal/linters"
)
-var lintName = "cppcheck"
+// refer to https://cppcheck.sourceforge.io/
+var linterName = "cppcheck"
func init() {
- linters.RegisterCodeReviewHandler(lintName, cppcheckHandler)
+ linters.RegisterPullRequestHandler(linterName, cppcheckHandler)
}
-func cppcheckHandler(log *xlog.Logger, linterConfig config.Linter, agent linters.Agent, event github.PullRequestEvent) (map[string][]linters.LinterOutput, error) {
- executor, err := NewCppcheckExecutor(linterConfig.WorkDir)
- if err != nil {
- log.Errorf("init cppcheck executor failed: %v", err)
- return nil, err
- }
-
- if linters.IsEmpty(linterConfig.Args...) {
- linterConfig.Args = append([]string{}, "--quiet", "--template='{file}:{line}:{column}: {message}'", "./")
- }
+func cppcheckHandler(log *xlog.Logger, a linters.Agent) error {
- output, err := executor.Run(log, linterConfig.Args...)
- if err != nil {
- log.Errorf("cppcheck run failed: %v", err)
- return nil, err
- }
-
- parsedOutput, err := executor.Parse(log, output)
- if err != nil {
- log.Errorf("cppcheck parse output failed: %v", err)
- return nil, err
+ if linters.IsEmpty(a.LinterConfig.Args...) {
+ a.LinterConfig.Args = append([]string{}, "--quiet", "--template='{file}:{line}:{column}: {message}'", ".")
}
- return parsedOutput, nil
-}
-
-// Cppcheck is an executor that knows how to execute Cppcheck commands.
-type Cppcheck struct {
- // dir is the location of this repo.
- dir string
- // cppcheck is the path to the cppcheck binary.
- cppcheck string
+ return linters.GeneralHandler(log, a, func(l *xlog.Logger, output []byte) (map[string][]linters.LinterOutput, error) {
+ return linters.Parse(log, output, cppcheckParser)
+ })
}
-// NewCppcheckExecutor returns a new executor that knows how to execute cppcheck commands
-// TODO: with config
-func NewCppcheckExecutor(dir string) (linters.Linter, error) {
- log.Infof("cppcheck executor init")
- g, err := exec.LookPath("cppcheck")
+func cppcheckParser(line string) (*linters.LinterOutput, error) {
+ lineResult, err := linters.GeneralLineParser(line)
if err != nil {
return nil, err
- }
- return &Cppcheck{
- dir: dir,
- cppcheck: g,
- }, nil
-}
-func (e *Cppcheck) Run(log *xlog.Logger, args ...string) ([]byte, error) {
- c := exec.Command(e.cppcheck, args...)
- c.Dir = e.dir
- var out bytes.Buffer
- c.Stdout = &out
- c.Stderr = &out
- err := c.Run()
- if err != nil {
- log.Errorf("cppcheck run with status: %v, mark and continue", err)
- } else {
- log.Infof("cppcheck succeeded")
}
- return out.Bytes(), nil
-}
-
-func (e *Cppcheck) Parse(log *xlog.Logger, output []byte) (map[string][]linters.LinterOutput, error) {
- return linters.FormatLinterOutput(log, output, linters.FormatLinterLine)
+ return &linters.LinterOutput{
+ File: strings.TrimLeft(lineResult.File, "'"),
+ Line: lineResult.Line,
+ Column: lineResult.Column,
+ Message: strings.TrimRight(lineResult.Message, "'"),
+ }, nil
}
diff --git a/internal/linters/c/cppcheck/cppcheck_test.go b/internal/linters/c/cppcheck/cppcheck_test.go
new file mode 100644
index 00000000..6bc9c33e
--- /dev/null
+++ b/internal/linters/c/cppcheck/cppcheck_test.go
@@ -0,0 +1,52 @@
+package cppcheck
+
+import (
+ "testing"
+
+ "github.com/qiniu/reviewbot/internal/linters"
+)
+
+func TestFormatCppCheckLine(t *testing.T) {
+ tc := []struct {
+ input string
+ expected *linters.LinterOutput
+ }{
+ {"'cppcheck_test.c:6:7: Array 'a[10]' accessed at index 10, which is out of bounds.'", &linters.LinterOutput{
+ File: "cppcheck_test.c",
+ Line: 6,
+ Column: 7,
+ Message: "Array 'a[10]' accessed at index 10, which is out of bounds.",
+ }},
+ {"'fdk-aac-0.1.4-libMpegTPDec-src/tpdec_asc.cpp:1198:23: Variable 'bitsAvailable' is reassigned a value before the old one has been used.'", &linters.LinterOutput{
+ File: "fdk-aac-0.1.4-libMpegTPDec-src/tpdec_asc.cpp",
+ Line: 1198,
+ Column: 23,
+ Message: "Variable 'bitsAvailable' is reassigned a value before the old one has been used.",
+ }},
+ {"Checking test/tpdec_adts.c", nil},
+ }
+
+ for _, c := range tc {
+
+ output, err := cppcheckParser(c.input)
+
+ if output == nil {
+ if c.expected != nil {
+ t.Errorf("expected: %v, got: %v", c.expected, output)
+ }
+ continue
+ }
+
+ if c.expected == nil && output != nil {
+ t.Errorf("expected error, got: %v", output)
+ }
+
+ if err != nil {
+ t.Errorf("unexpected error: %v", err)
+ }
+
+ if output.File != c.expected.File || output.Line != c.expected.Line || output.Column != c.expected.Column || output.Message != c.expected.Message {
+ t.Errorf("expected: %v, got: %v", c.expected, output)
+ }
+ }
+}
diff --git a/internal/linters/linters.go b/internal/linters/linters.go
index 9a0420c0..87910ce0 100644
--- a/internal/linters/linters.go
+++ b/internal/linters/linters.go
@@ -143,7 +143,7 @@ func ExecRun(workDir, command string, args ...string) ([]byte, error) {
c := exec.Command(g, args...)
c.Dir = workDir
- return c.Output()
+ return c.CombinedOutput()
}
// GeneralParse parses the output of a linter command.
diff --git a/internal/linters/linters_test.go b/internal/linters/linters_test.go
index 73676495..8ad10be4 100644
--- a/internal/linters/linters_test.go
+++ b/internal/linters/linters_test.go
@@ -48,16 +48,6 @@ func TestFormatStaticcheckLine(t *testing.T) {
for _, c := range tc {
output, err := GeneralLineParser(c.input)
- if c.expected == nil && output != nil {
- t.Errorf("expected error, got: %v", output)
- } else {
- continue
- }
-
- if err != nil {
- t.Errorf("unexpected error: %v", err)
- }
-
if output == nil {
if c.expected != nil {
t.Errorf("expected: %v, got: %v", c.expected, output)
@@ -65,6 +55,13 @@ func TestFormatStaticcheckLine(t *testing.T) {
continue
}
+ if c.expected == nil && output != nil {
+ t.Errorf("expected error, got: %v", output)
+ }
+
+ if err != nil {
+ t.Errorf("unexpected error: %v", err)
+ }
if output.File != c.expected.File || output.Line != c.expected.Line || output.Column != c.expected.Column || output.Message != c.expected.Message {
t.Errorf("expected: %v, got: %v", c.expected, output)
}
diff --git a/internal/linters/lua/luacheck/luacheck_test.go b/internal/linters/lua/luacheck/luacheck_test.go
index c703cb36..af116a26 100644
--- a/internal/linters/lua/luacheck/luacheck_test.go
+++ b/internal/linters/lua/luacheck/luacheck_test.go
@@ -28,25 +28,25 @@ func TestFormatLuaCheckLine(t *testing.T) {
expected *linters.LinterOutput
}{
{" video/mp4/libs/mp4lib.lua:184:11: value assigned to variable mem_data is overwritten on line 202 before use", &linters.LinterOutput{
- File: " video/mp4/libs/mp4lib.lua:184:11",
+ File: "video/mp4/libs/mp4lib.lua",
Line: 184,
Column: 11,
Message: "value assigned to variable mem_data is overwritten on line 202 before use",
}},
{" utils/jsonschema.lua:723:121: line is too long (142 > 120)", &linters.LinterOutput{
- File: " utils/jsonschema.lua",
+ File: "utils/jsonschema.lua",
Line: 723,
Column: 121,
Message: "line is too long (142 > 120)",
}},
{" utils/httpc/http_simple.lua:24:1: setting read-only global variable _VERSION", &linters.LinterOutput{
- File: " utils/httpc/http_simple.lua",
+ File: "utils/httpc/http_simple.lua",
Line: 24,
Column: 1,
Message: "setting read-only global variable _VERSION",
}},
{" test/qtest_access.lua:1220:1: inconsistent indentation (SPACE followed by TAB)", &linters.LinterOutput{
- File: " test/qtest_access.lua",
+ File: "test/qtest_access.lua",
Line: 1220,
Column: 1,
Message: "inconsistent indentation (SPACE followed by TAB)",
@@ -56,23 +56,21 @@ func TestFormatLuaCheckLine(t *testing.T) {
for _, c := range tc {
output, err := luacheckParser(c.input)
+ if output == nil {
+ if c.expected != nil {
+ t.Errorf("expected: %v, got: %v", c.expected, output)
+ }
+ continue
+ }
+
if c.expected == nil && output != nil {
t.Errorf("expected error, got: %v", output)
- } else {
- continue
}
if err != nil {
t.Errorf("unexpected error: %v", err)
}
- if output == nil {
- if c.expected != nil {
- t.Errorf("expected: %v, got: %v", c.expected, output)
- }
- continue
- }
-
if output.File != c.expected.File || output.Line != c.expected.Line || output.Column != c.expected.Column || output.Message != c.expected.Message {
t.Errorf("expected: %v, got: %v", c.expected, output)
}
diff --git a/internal/linters/util.go b/internal/linters/util.go
deleted file mode 100644
index 6357d797..00000000
--- a/internal/linters/util.go
+++ /dev/null
@@ -1,108 +0,0 @@
-package linters
-
-import (
- "fmt"
- "path/filepath"
- "regexp"
- "strconv"
- "strings"
-
- "github.com/qiniu/x/log"
- "github.com/qiniu/x/xlog"
-)
-
-func IsEmpty(args ...string) bool {
- for _, arg := range args {
- if arg != "" {
- return false
- }
- }
-
- return true
-}
-
-type FormatLinterLineFunc func(line string) (*LinterOutput, error)
-
-func FormatLinterOutput(log *xlog.Logger, output []byte, FormatLinter FormatLinterLineFunc) (map[string][]LinterOutput, error) {
- lines := strings.Split(string(output), "\n")
-
- var result = make(map[string][]LinterOutput)
- for _, line := range lines {
- if line == "" {
- continue
- }
- output, err := FormatLinter(line)
- if err != nil {
- log.Warnf("unexpected linter check output: %v", line)
- // 不直接退出
- continue
- }
-
- if output == nil {
- continue
- }
-
- if isGopAutoGeneratedFile(output.File) {
- log.Infof("skip auto generated file by go+ : %s", output.File)
- continue
- }
-
- if outs, ok := result[output.File]; !ok {
- result[output.File] = []LinterOutput{*output}
- } else {
- // remove duplicate
- var existed bool
- for _, v := range outs {
- if v.File == output.File && v.Line == output.Line && v.Column == output.Column && v.Message == output.Message {
- existed = true
- break
- }
- }
-
- if !existed {
- result[output.File] = append(result[output.File], *output)
- }
- }
- }
-
- return result, nil
-}
-
-// common format LinterLine
-func FormatLinterLine(line string) (*LinterOutput, error) {
- pattern := `^(.*):(\d+):(\d+): (.*)$`
- regex, err := regexp.Compile(pattern)
- if err != nil {
- log.Errorf("compile regex failed: %v", err)
- return nil, err
- }
- matches := regex.FindStringSubmatch(line)
- if len(matches) != 5 {
- return nil, fmt.Errorf("unexpected format, original: %s", line)
- }
-
- lineNumber, err := strconv.ParseInt(matches[2], 10, 64)
- if err != nil {
- return nil, err
- }
-
- columnNumber, err := strconv.ParseInt(matches[3], 10, 64)
- if err != nil {
- return nil, err
- }
-
- return &LinterOutput{
- File: matches[1],
- Line: int(lineNumber),
- Column: int(columnNumber),
- Message: matches[4],
- }, nil
-}
-
-var gop_auto_generated_file_pattern = `^.*_autogen.*.go$`
-var gopGeneratedFileRegexp = regexp.MustCompile(gop_auto_generated_file_pattern)
-
-func isGopAutoGeneratedFile(file string) bool {
- // TODO: need a more accurate way to determine whether it is a go+ auto generated file
- return gopGeneratedFileRegexp.MatchString(filepath.Base(file))
-}
diff --git a/main.go b/main.go
index d431e008..96cc90ed 100644
--- a/main.go
+++ b/main.go
@@ -30,12 +30,12 @@ import (
gitv2 "k8s.io/test-infra/prow/git/v2"
// linters import
+ _ "github.com/qiniu/reviewbot/internal/linters/c/cppcheck"
_ "github.com/qiniu/reviewbot/internal/linters/git-flow/commit-check"
_ "github.com/qiniu/reviewbot/internal/linters/go/gofmt"
_ "github.com/qiniu/reviewbot/internal/linters/go/golangci_lint"
_ "github.com/qiniu/reviewbot/internal/linters/go/staticcheck"
_ "github.com/qiniu/reviewbot/internal/linters/lua/luacheck"
- _ "github.com/reviewbot/internal/linters/c/cppcheck"
)
type options struct {