Skip to content

Commit

Permalink
fix comments
Browse files Browse the repository at this point in the history
  • Loading branch information
jiangpeipei327 committed Mar 12, 2024
1 parent a18f404 commit 3ef4227
Show file tree
Hide file tree
Showing 13 changed files with 169 additions and 213 deletions.
9 changes: 7 additions & 2 deletions .github/workflows/go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 ./...
4 changes: 2 additions & 2 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand All @@ -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
Expand Down
13 changes: 1 addition & 12 deletions config/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,4 @@ qbox/kodo: # github repository name, which will override the settings in qbox or
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
command: cppcheck
33 changes: 33 additions & 0 deletions internal/linters/c/cppcheck/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# cppcheck 语法:
cppcheck [选项] [文件或路径]<br />
如果给出目录而不是文件名,则从给定的目录递归检查 *.cpp、*.cxx、*.cc、*.c++、*.c、*.ipp、*.ixx、*.tpp 和 *.txx 文件。<br />
Options:<br />
-q, --quiet 不显示进度报告。<br />
--template='<text>' 设置错误消息的格式。 可用字段:
&emsp; {file}: file name<br />
&emsp; {line}: line number<br />
&emsp; {column}: column number<br />
&emsp; {callstack}: 显示调用堆栈。 示例: [file.c:1] -> [file.c:100]<br />
&emsp; {inconclusive:text}: 如果警告不确定,则写入文本<br />
&emsp; {severity}: severity<br />
&emsp; {message}: warning message<br />
&emsp; {id}: warning id<br />
&emsp; {cwe}: CWE id(常见弱点枚举)<br />
&emsp; {code}: 显示真实代码<br />
&emsp; \t: insert tab<br />
&emsp; \n: insert newline<br />
&emsp; \r: 插入回车符<br />
--enable=<id> 启用附加检查。 可用的 id 是:<br />
&emsp; * all:启用所有检查。 建议仅在扫描整个程序时使用--enable=all,因为这会启用 unusedFunction。<br />
&emsp; * warning:启用警告消息<br />
&emsp; * style:启用所有编码风格检查。 所有具有“样式”、“警告”、“性能”和“可移植性”严重性的消息均已启用。<br />
&emsp; * performance:启用性能消息<br />
&emsp; * portability:启用可移植性消息<br />
&emsp; * information:Enable information messages<br />
&emsp; * unusedFunction:检查是否有未使用的功能。 建议仅在扫描整个程序时启用此功能。<br />
&emsp; * missingInclude:如果缺少包含内容,则发出警告。<br />
&emsp; 如果用逗号分隔,可以给出多个 id。<br />
Example usage:<br />
递归检查当前目录,不打印进度,默认启用错误检查,输出格式,文件:行:列: 报错信息<br />
cppcheck --quiet --template='{file}:{line}:{column}: {message}' ./<br />
也可以通过设置 --enable 开启其他的检查
84 changes: 20 additions & 64 deletions internal/linters/c/cppcheck/cppcheck.go
Original file line number Diff line number Diff line change
@@ -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
}
52 changes: 52 additions & 0 deletions internal/linters/c/cppcheck/cppcheck_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
2 changes: 1 addition & 1 deletion internal/linters/linters.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
35 changes: 25 additions & 10 deletions internal/linters/linters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
}
}
24 changes: 11 additions & 13 deletions internal/linters/lua/luacheck/luacheck_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Expand All @@ -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)
}
Expand Down
8 changes: 8 additions & 0 deletions internal/linters/testdata/cppcheck_test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// +build ignore go

int main()
{
char a[10];
a[10] = 0;
return 0;
}
8 changes: 8 additions & 0 deletions internal/linters/testdata/staticcheck.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package main

import "fmt"

func main() {
str := "Hello, world!"
fmt.Printf("%d World", str)
}
Loading

0 comments on commit 3ef4227

Please sign in to comment.