fix comments

.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 ./...

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/[email protected]
 
-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

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
+    run_if_changed: ["src/qiniu.com/kodo/.*\\.go$"] # only run if changed files match the regex

internal/linters/c/cppcheck/README.md

@@ -0,0 +1,12 @@
+# Cppcheck - A tool for static C/C++ code analysis
+Syntax:<br />
+    cppcheck [OPTIONS] [files or paths]<br />
+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.<br />
+
+
+Example usage in this repo:<br />
+Recursively check the current folder, format the error messages as file name:line number:column number: warning message and don't print progress:<br />
+cppcheck --quiet --template='{file}:{line}:{column}:  {message}' .<br />
+
+For more information:<br />
+    https://files.cppchecksolutions.com/manual.pdf<br />

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
 }

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)
+		}
+	}
+}

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.

internal/linters/linters_test.go

@@ -48,23 +48,20 @@ 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)
 			}
 			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)
 		}

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)
 		}