diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index f03bcba8..4ec29bad 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -21,11 +21,16 @@ jobs: with: go-version: '1.21.5' + - name: install linters + run: | + go install honnef.co/go/tools/cmd/staticcheck@latest + sudo apt install cppcheck + - 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..add6dfb2 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,8 @@ 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 # 设置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..94114717 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -32,14 +32,8 @@ qbox/3rd-ffmpeg: # github repository name, which will override the settings in q 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 + command: cppcheck \ 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..9d0a92ff --- /dev/null +++ b/internal/linters/c/cppcheck/README.md @@ -0,0 +1,33 @@ +# cppcheck 语法： +cppcheck [选项] [文件或路径]
+如果给出目录而不是文件名，则从给定的目录递归检查 *.cpp、*.cxx、*.cc、*.c++、*.c、*.ipp、*.ixx、*.tpp 和 *.txx 文件。
+Options:
+-q, --quiet 不显示进度报告。
+--template='' 设置错误消息的格式。 可用字段： +  {file}： file name
+  {line}： line number
+  {column}： column number
+  {callstack}： 显示调用堆栈。 示例： [file.c:1] -> [file.c:100]
+  {inconclusive:text}： 如果警告不确定，则写入文本
+  {severity}： severity
+  {message}： warning message
+  {id}： warning id
+  {cwe}： CWE id（常见弱点枚举）
+  {code}： 显示真实代码
+  \t： insert tab
+  \n： insert newline
+  \r： 插入回车符
+--enable= 启用附加检查。 可用的 id 是：
+  * all：启用所有检查。 建议仅在扫描整个程序时使用--enable=all，因为这会启用 unusedFunction。
+  * warning：启用警告消息
+  * style：启用所有编码风格检查。 所有具有“样式”、“警告”、“性能”和“可移植性”严重性的消息均已启用。
+  * performance：启用性能消息
+  * portability：启用可移植性消息
+  * information：Enable information messages
+  * unusedFunction：检查是否有未使用的功能。 建议仅在扫描整个程序时启用此功能。
+  * missingInclude：如果缺少包含内容，则发出警告。
+  如果用逗号分隔，可以给出多个 id。
+Example usage:
+递归检查当前目录，不打印进度，默认启用错误检查，输出格式，文件:行:列: 报错信息
+cppcheck --quiet --template='{file}:{line}:{column}: {message}' ./
+也可以通过设置 --enable 开启其他的检查 diff --git a/internal/linters/c/cppcheck/cppcheck.go b/internal/linters/c/cppcheck/cppcheck.go index d466de09..7aa8425a 100644 --- a/internal/linters/c/cppcheck/cppcheck.go +++ b/internal/linters/c/cppcheck/cppcheck.go @@ -1,85 +1,41 @@ 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, linters.GeneralParse) + 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..c52bbe5e 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) } @@ -110,3 +107,21 @@ func TestIsGopAutoGeneratedFile(t *testing.T) { } } } + +func TestExecRun(t *testing.T) { + tcs := []struct { + command string + args []string + expected string + }{ + {"cppcheck", append([]string{}, "--quiet", "--template='{file}:{line}:{column}: {message}'", "./"), "'cppcheck_test.c:6:7: Array 'a[10]' accessed at index 10, which is out of bounds.'\n"}, + {"staticcheck", append([]string{}, "-debug.no-compile-errors=true", "./..."), "staticcheck.go:7:13: Printf format %d has arg #1 of wrong type string (SA5009)\n"}, + } + for _, tc := range tcs { + output, _ := ExecRun("./testdata", tc.command, tc.args...) + sOut := string(output) + if string(output) != tc.expected { + t.Errorf("expected: %v, got: %v", tc.expected, sOut) + } + } +} 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/testdata/cppcheck_test.c b/internal/linters/testdata/cppcheck_test.c new file mode 100644 index 00000000..e678e299 --- /dev/null +++ b/internal/linters/testdata/cppcheck_test.c @@ -0,0 +1,8 @@ +// +build ignore go + +int main() +{ + char a[10]; + a[10] = 0; + return 0; +} \ No newline at end of file diff --git a/internal/linters/testdata/staticcheck.go b/internal/linters/testdata/staticcheck.go new file mode 100644 index 00000000..6a726f05 --- /dev/null +++ b/internal/linters/testdata/staticcheck.go @@ -0,0 +1,8 @@ +package main + +import "fmt" + +func main() { + str := "Hello, world!" + fmt.Printf("%d World", str) +} 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 {