From 66e9ac3da6a616df97b8226230959f43533dcd9c Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 10 Sep 2015 11:38:19 -0700 Subject: [PATCH 1/8] .tools: Remove redundant r.Pass check when 'fail == 0' The previous version of this script would always pass this check, because when 'fail == 0' was true, all of the messages had passed. That lead to logic like: a. If all checks passed for the commit and the verbose flag is set, print all the messages (which all passed). b. If all checks passed for the commit and the verbose flag is not set, don't print details about any messages (which all passed). c. If any checks failed, print all the failed messages. That wasn't printing successful checks when the verbose flag was true but some checks failed. This commit shifts the logic around to: A. If the verbose flag is set, print messages for all checks, using TAP's "ok" and "not ok" to distinguish between per-check pass/fail [1]. B. If the verbose flag is not set, only print the failed messages (but still keep the "not ok" prefix). In the non-verbose case, then (b/c) and (B) look the same (modulo my "not ok" prefix adjustment). And in the verbose case, I think (A) is preferable to (a/c), because there's no reason to *not* print successful checks when you've enabled the verbose flag. [1]: http://testanything.org/ Signed-off-by: W. Trevor King --- .tools/validate.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/.tools/validate.go b/.tools/validate.go index bf337dfff..7f3f28495 100644 --- a/.tools/validate.go +++ b/.tools/validate.go @@ -74,20 +74,19 @@ func main() { results = append(results, vr...) if _, fail := vr.PassFail(); fail == 0 { fmt.Println("PASS") - if *flVerbose { - for _, r := range vr { - if r.Pass { - fmt.Printf(" - %s\n", r.Msg) - } - } - } } else { fmt.Println("FAIL") - // default, only print out failed validations - for _, r := range vr { - if !r.Pass { - fmt.Printf(" - %s\n", r.Msg) + } + for _, r := range vr { + if *flVerbose { + if r.Pass { + fmt.Printf("ok") + } else { + fmt.Printf("not ok") } + fmt.Printf(" %s\n", r.Msg) + } else if !r.Pass { + fmt.Printf("not ok %s\n", r.Msg) } } } From 650828c7b23cb2c14dc31fcc88adb3463f20eeef Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 11 Sep 2015 20:56:36 -0700 Subject: [PATCH 2/8] .tools: Make -v output comply with TAP version 13 Using TAP [1] makes it easy to consume the output of this tool in a variety of existing harnesses [2]. Only the verbose results are TAP compliant, mostly because folks aren't likely to care about verbosity if they're just piping the results into a TAP harness. If we end up wanting the non-verbose output to be TAP compliant, we'll have to switch to the trailing count approach explained in the "Unknown amount and failures" section of the spec [1], and we'll have to keep a running counter of failed checks (instead of the current len(DefaultRules)-based approach). [1]: http://testanything.org/tap-version-13-specification.html [2]: http://testanything.org/consumers.html Signed-off-by: W. Trevor King --- .tools/validate.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/.tools/validate.go b/.tools/validate.go index 7f3f28495..e9c090e39 100644 --- a/.tools/validate.go +++ b/.tools/validate.go @@ -68,8 +68,13 @@ func main() { } results := ValidateResults{} - for _, commit := range c { - fmt.Printf(" * %s %s ... ", commit["abbreviated_commit"], commit["subject"]) + + if *flVerbose { + fmt.Println("TAP version 13") + fmt.Printf("1..%d\n", len(c)*len(DefaultRules)) + } + for i, commit := range c { + fmt.Printf("# %s %s ... ", commit["abbreviated_commit"], commit["subject"]) vr := ValidateCommit(commit, DefaultRules) results = append(results, vr...) if _, fail := vr.PassFail(); fail == 0 { @@ -77,16 +82,16 @@ func main() { } else { fmt.Println("FAIL") } - for _, r := range vr { + for j, r := range vr { if *flVerbose { if r.Pass { fmt.Printf("ok") } else { fmt.Printf("not ok") } - fmt.Printf(" %s\n", r.Msg) + fmt.Printf(" %d - %s\n", i*len(DefaultRules)+j+1, r.Msg) } else if !r.Pass { - fmt.Printf("not ok %s\n", r.Msg) + fmt.Printf("not ok - %s\n", r.Msg) } } } From d51f2fff53fc6f45602a17caef43e06212663129 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 11 Sep 2015 22:42:22 -0700 Subject: [PATCH 3/8] .tools: Run 'go vet ...' on each commit Instead of just checking the tip, run 'go vet ...' on each commit in the series. This makes sure we catch errors with earlier commits even if they were fixed by subsequent commits. The bulk of the complexity here is checking out a previous commit's tree to a temporary directory, which is handled by the new GitCheckoutTree. I'd expect Git to be able to checkout an arbitrary tree to an arbitrary directory, but strangely it doesn't seem to be able to do that without side effects involving HEAD or the index. Instead, I call 'git archive ...' and pipe the results into tar to do the unpacking. You *can* unpack an archive using native Go, but there didn't seem to be a convenient tar.Reader.Extract(directory string) method. Shelling out to tar seemed more robust than writing a few dozen lines of Go, so that's what I've done. I'd expect shelling out is less portable, but we really only need this to run on Travis' Ubuntu environment [1]. We can work out a more portable approach if/when we need to support this script on a system without a compatible 'tar' command. [1]: http://docs.travis-ci.com/user/ci-environment/#Virtualization-environments Signed-off-by: W. Trevor King --- .tools/validate.go | 77 ++++++++++++++++++++++++++++++++++++++++++++++ .travis.yml | 1 - 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/.tools/validate.go b/.tools/validate.go index e9c090e39..1610f2eb3 100644 --- a/.tools/validate.go +++ b/.tools/validate.go @@ -5,6 +5,8 @@ import ( "encoding/json" "flag" "fmt" + "io" + "io/ioutil" "log" "os" "os/exec" @@ -39,6 +41,9 @@ var DefaultRules = []ValidateRule{ return vr }, // TODO add something for the cleanliness of the c.Subject + func(c CommitEntry) (vr ValidateResult) { + return ExecTree(c, "go", "vet", "./...") + }, } var ( @@ -229,3 +234,75 @@ func GitHeadCommit() (string, error) { } return strings.TrimSpace(string(output)), nil } + +// GitCheckoutTree extracts the tree associated with the given commit +// to the given directory. Unlike 'git checkout ...', it does not +// alter the HEAD. +func GitCheckoutTree(commit string, directory string) error { + pipeReader, pipeWriter := io.Pipe() + gitCmd := exec.Command("git", "archive", commit) + gitCmd.Stdout = pipeWriter + gitCmd.Stderr = os.Stderr + tarCmd := exec.Command("tar", "-xC", directory) + tarCmd.Stdin = pipeReader + tarCmd.Stderr = os.Stderr + err := gitCmd.Start() + if err != nil { + return err + } + defer gitCmd.Process.Kill() + err = tarCmd.Start() + if err != nil { + return err + } + defer tarCmd.Process.Kill() + err = gitCmd.Wait() + if err != nil { + return err + } + err = pipeWriter.Close() + if err != nil { + return err + } + err = tarCmd.Wait() + if err != nil { + return err + } + return nil +} + +// ExecTree executes a command in a checkout of the commit's tree, +// wrapping any errors in a ValidateResult object. +func ExecTree(c CommitEntry, args ...string) (vr ValidateResult) { + vr.CommitEntry = c + err := execTree(c, args...) + if err == nil { + vr.Pass = true + vr.Msg = strings.Join(args, " ") + } else { + vr.Pass = false + vr.Msg = fmt.Sprintf("%s : %s", strings.Join(args, " "), err.Error()) + } + return vr +} + +// execTree executes a command in a checkout of the commit's tree +func execTree(c CommitEntry, args ...string) error { + dir, err := ioutil.TempDir("", "go-validate-") + if err != nil { + return err + } + defer os.RemoveAll(dir) + err = GitCheckoutTree(c["commit"], dir) + if err != nil { + return err + } + cmd := exec.Command(args[0], args[1:]...) + cmd.Dir = dir + cmd.Stderr = os.Stderr + err = cmd.Run() + if err != nil { + return err + } + return nil +} diff --git a/.travis.yml b/.travis.yml index 68cba0d01..da7cff480 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,7 +12,6 @@ before_install: install: true script: - - go vet -x ./... - $HOME/gopath/bin/golint ./... - go run .tools/validate.go -range ${TRAVIS_COMMIT_RANGE} From 5dff8c56226d173fa5e63af392af6dab511a0d50 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 11 Sep 2015 23:41:26 -0700 Subject: [PATCH 4/8] .tools: Run 'golint ./...' on each commit Instead of just checking the tip, run 'golint ./...' on each commit in the series. This makes sure we catch errors with earlier commits even if they were fixed by subsequent commits. I've added ValidateResult.Detail to hold stdout from ExecTree calls, because golint just prints warnings to its stdout and always returns an exit code of 0. Then in the golint rule, I check vr.Detail and fail any result with content there. In the main block, I print YAML blocks [1] with vr.Detail, so folks consuming the TAP can figure out what golint was complaining about. The YAML block gets printed when the verbose flag is set or when there's an error (otherwise it's hard to figure out what golint was complaining about in non-verbose mode). The YAML library ensures we don't get bitten by YAML-sensitive characters in the Detail string (e.g. colons). The gopkg.in location is a nifty site allowing us to link to a specific API of the project (which is hosted on GitHub) [2]. [1]: http://testanything.org/tap-version-13-specification.html [2]: http://godoc.org/gopkg.in/yaml.v2 Signed-off-by: W. Trevor King --- .tools/validate.go | 39 +++++++++++++++++++++++++++++++++------ .travis.yml | 4 ++-- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/.tools/validate.go b/.tools/validate.go index 1610f2eb3..d1dcd8a01 100644 --- a/.tools/validate.go +++ b/.tools/validate.go @@ -12,6 +12,8 @@ import ( "os/exec" "regexp" "strings" + + yaml "gopkg.in/yaml.v2" ) // DefaultRules are the standard validation to perform on git commits @@ -44,6 +46,13 @@ var DefaultRules = []ValidateRule{ func(c CommitEntry) (vr ValidateResult) { return ExecTree(c, "go", "vet", "./...") }, + func(c CommitEntry) (vr ValidateResult) { + vr = ExecTree(c, os.ExpandEnv("$HOME/gopath/bin/golint"), "./...") + if len(vr.Detail) > 0 { + vr.Pass = false + } + return vr + }, } var ( @@ -98,6 +107,19 @@ func main() { } else if !r.Pass { fmt.Printf("not ok - %s\n", r.Msg) } + if (*flVerbose || !r.Pass) && len(r.Detail) > 0 { + m := map[string]string{"message": r.Detail} + buf, err := yaml.Marshal(m) + if err != nil { + log.Fatal(err) + } + lines := strings.Split(strings.TrimSpace(string(buf)), "\n") + fmt.Println(" ---") + for _, line := range lines { + fmt.Printf(" %s\n", line) + } + fmt.Println(" ...") + } } } _, fail := results.PassFail() @@ -124,6 +146,7 @@ type ValidateResult struct { CommitEntry CommitEntry Pass bool Msg string + Detail string } // ValidateResults is a set of results. This is type makes it easy for the following function. @@ -275,7 +298,8 @@ func GitCheckoutTree(commit string, directory string) error { // wrapping any errors in a ValidateResult object. func ExecTree(c CommitEntry, args ...string) (vr ValidateResult) { vr.CommitEntry = c - err := execTree(c, args...) + stdout, err := execTree(c, args...) + vr.Detail = strings.TrimSpace(stdout) if err == nil { vr.Pass = true vr.Msg = strings.Join(args, " ") @@ -287,22 +311,25 @@ func ExecTree(c CommitEntry, args ...string) (vr ValidateResult) { } // execTree executes a command in a checkout of the commit's tree -func execTree(c CommitEntry, args ...string) error { +func execTree(c CommitEntry, args ...string) (string, error) { dir, err := ioutil.TempDir("", "go-validate-") if err != nil { - return err + return "", err } defer os.RemoveAll(dir) err = GitCheckoutTree(c["commit"], dir) if err != nil { - return err + return "", err } + buf := bytes.NewBuffer([]byte{}) cmd := exec.Command(args[0], args[1:]...) cmd.Dir = dir + cmd.Stdout = buf cmd.Stderr = os.Stderr err = cmd.Run() + stdout := string(buf.Bytes()) if err != nil { - return err + return stdout, err } - return nil + return stdout, nil } diff --git a/.travis.yml b/.travis.yml index da7cff480..f8039daab 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,12 +6,12 @@ go: sudo: false before_install: - - go get golang.org/x/tools/cmd/vet - go get github.com/golang/lint/golint + - go get golang.org/x/tools/cmd/vet + - go get gopkg.in/yaml.v2 install: true script: - - $HOME/gopath/bin/golint ./... - go run .tools/validate.go -range ${TRAVIS_COMMIT_RANGE} From 404a6394ad14c70d0d3e92f3a6bef0a3bd799745 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sat, 12 Sep 2015 00:48:11 -0700 Subject: [PATCH 5/8] .tools: Run 'go fmt ./...' on each commit Instead of just checking the tip, run 'go fmt ./...' on each commit in the series. This makes sure we catch errors with earlier commits even if they were fixed by subsequent commits. Signed-off-by: W. Trevor King --- .tools/validate.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.tools/validate.go b/.tools/validate.go index d1dcd8a01..18445f08a 100644 --- a/.tools/validate.go +++ b/.tools/validate.go @@ -46,6 +46,9 @@ var DefaultRules = []ValidateRule{ func(c CommitEntry) (vr ValidateResult) { return ExecTree(c, "go", "vet", "./...") }, + func(c CommitEntry) (vr ValidateResult) { + return ExecTree(c, "go", "fmt", "./...") + }, func(c CommitEntry) (vr ValidateResult) { vr = ExecTree(c, os.ExpandEnv("$HOME/gopath/bin/golint"), "./...") if len(vr.Detail) > 0 { From bdaf4467ed196004a40f285afe2e8353b23093b1 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sat, 12 Sep 2015 08:32:15 -0700 Subject: [PATCH 6/8] .tools: Run 'git show --check ...' on each commit Watch for accidentally-injected trailing whitespace. The stdout for failing checks is a bit hard to read: not ok 2 - git show --check 2652b57a368e8fdd6745f2354f25c56e57326b16 --- message: "commit 2652b57a368e8fdd6745f2354f25c56e57326b16\nAuthor: W. Trevor King \nDate: Sat Sep 12 08:32:15 2015 -0700\n\n .tools: Run 'git show --check ...' on each commit\n \n Watch for accidentally-injected trailing whitespace. The stdout for\n failing checks is a bit hard to read:\n \n not ok 2 - exit status 2\n ---\n message: \"commit 09748ebfd739dbcee35a585a75f896a74aa1c2f0\\nAuthor: W. Trevor King\n \\nDate: Sat Sep 12 08:32:15 2015 -0700\\n\\n \ .tools: Run 'git\n show --check ...' on each commit\\n \\n Watch for accidentally-injected trailing\n whitespace.\\n \\n Signed-off-by: W. Trevor King \\n\\n.tools/validate.go:56:\n trailing whitespace.\\n+\\t\\t\\treturn vr \\n\"\n ...\n \n Signed-off-by: W. Trevor King \n\n.tools/validate.go:57: trailing whitespace.\n+\t\t\treturn vr \n" ... But having the command in the "not ok" line should make it easy for folks to reproduce locally if they're not collecting the YAML blocks with a TAP harness. Signed-off-by: W. Trevor King --- .tools/validate.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.tools/validate.go b/.tools/validate.go index 18445f08a..0ad146a0f 100644 --- a/.tools/validate.go +++ b/.tools/validate.go @@ -43,6 +43,22 @@ var DefaultRules = []ValidateRule{ return vr }, // TODO add something for the cleanliness of the c.Subject + func(c CommitEntry) (vr ValidateResult) { + vr.CommitEntry = c + buf := bytes.NewBuffer([]byte{}) + args := []string{"git", "show", "--check", c["commit"]} + vr.Msg = strings.Join(args, " ") + cmd := exec.Command(args[0], args[1:]...) + cmd.Stdout = buf + cmd.Stderr = os.Stderr + if err := cmd.Run(); err != nil { + vr.Pass = false + vr.Detail = string(buf.Bytes()) + return vr + } + vr.Pass = true + return vr + }, func(c CommitEntry) (vr ValidateResult) { return ExecTree(c, "go", "vet", "./...") }, From 44582cf2e9afb4dcbf4120db7d8cba26e08cedf4 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sat, 12 Sep 2015 08:44:38 -0700 Subject: [PATCH 7/8] .travis: Print commit range for debugging TRAVIS_COMMIT_RANGE Sometimes Travis gives test results for more commits than I expect it to. This should make it easy to figure out why, so we can distinguish between "Travis is giving us a surprisingly large range" and "validate.go has a buggy GitCommits". Signed-off-by: W. Trevor King --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index f8039daab..26210e91b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,5 +13,6 @@ before_install: install: true script: + - echo ${TRAVIS_COMMIT_RANGE} - go run .tools/validate.go -range ${TRAVIS_COMMIT_RANGE} From 8831b203606aa74d2bf5274108816dde7629fa6f Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sat, 12 Sep 2015 12:13:00 -0700 Subject: [PATCH 8/8] .travis: Convert TRAVIS_COMMIT_RANGE base...head to base..head Work around travis-ci/travis-ci#4596 until that is fixed upstream [1]. This avoids pulling in commits from the base tip that aren't reachable from the head tip (e.g. if master has advanced since the PR branched off, and the PR is against master). We only want to check commits that are in the head branch but not in the base branch (more details on the range syntax in [2]). Once the Travis bug does get fixed, the shell replacement will be a no-op. So we don't have to worry about checks breaking once the bug gets fixed, and can periodically poll the bug and remove the workaround at out leisure after the fix. [1]: https://github.com/travis-ci/travis-ci/issues/4596 [2]: http://git-scm.com/docs/gitrevisions#_specifying_ranges Signed-off-by: W. Trevor King --- .travis.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 26210e91b..5b9d515dc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,6 +13,5 @@ before_install: install: true script: - - echo ${TRAVIS_COMMIT_RANGE} - - go run .tools/validate.go -range ${TRAVIS_COMMIT_RANGE} - + - echo "${TRAVIS_COMMIT_RANGE} -> ${TRAVIS_COMMIT_RANGE/.../..} (travis-ci/travis-ci#4596)" + - go run .tools/validate.go -range ${TRAVIS_COMMIT_RANGE/.../..}