diff --git a/build.go b/build.go index acb16feeb..3b52fb91d 100644 --- a/build.go +++ b/build.go @@ -27,7 +27,6 @@ import ( "runtime" "strconv" "strings" - "syscall" "text/template" "time" ) @@ -154,6 +153,39 @@ var targets = map[string]target{ }, } +var ( + // fast linters complete in a fraction of a second and might as well be + // run always as part of the build + fastLinters = []string{ + "deadcode", + "golint", + "ineffassign", + "vet", + } + + // slow linters take several seconds and are run only as part of the + // "metalint" command. + slowLinters = []string{ + "gosimple", + "staticcheck", + "structcheck", + "unused", + "varcheck", + } + + // Which parts of the tree to lint + lintDirs = []string{".", "./lib/...", "./cmd/..."} + + // Messages to ignore + lintExcludes = []string{ + ".pb.go", + "should have comment", + "protocol.Vector composite literal uses unkeyed fields", + "Use DialContext instead", // Go 1.7 + "os.SEEK_SET is deprecated", // Go 1.7 + } +) + func init() { // The "syncthing" target includes a few more files found in the "etc" // and "extra" dirs. @@ -203,8 +235,6 @@ func main() { // which is what you want for maximum error checking during development. if flag.NArg() == 0 { runCommand("install", targets["all"]) - runCommand("vet", target{}) - runCommand("lint", target{}) } else { // with any command given but not a target, the target is // "syncthing". So "go run build.go install" is "go run build.go install @@ -242,6 +272,7 @@ func runCommand(cmd string, target target) { tags = []string{"noupgrade"} } install(target, tags) + metalint(fastLinters, lintDirs) case "build": var tags []string @@ -249,6 +280,7 @@ func runCommand(cmd string, target target) { tags = []string{"noupgrade"} } build(target, tags) + metalint(fastLinters, lintDirs) case "test": test("./lib/...", "./cmd/...") @@ -284,28 +316,14 @@ func runCommand(cmd string, target target) { clean() case "vet": - vet("build.go") - vet("cmd", "lib") + metalint(fastLinters, lintDirs) case "lint": - lint(".") - lint("./cmd/...") - lint("./lib/...") + metalint(fastLinters, lintDirs) case "metalint": - if isGometalinterInstalled() { - dirs := []string{".", "./cmd/...", "./lib/..."} - ok := gometalinter("deadcode", dirs, "test/util.go") - ok = gometalinter("structcheck", dirs) && ok - ok = gometalinter("varcheck", dirs) && ok - ok = gometalinter("ineffassign", dirs) && ok - ok = gometalinter("unused", dirs) && ok - ok = gometalinter("staticcheck", dirs) && ok - ok = gometalinter("unconvert", dirs) && ok - if !ok { - os.Exit(1) - } - } + metalint(fastLinters, lintDirs) + metalint(slowLinters, lintDirs) case "version": fmt.Println(getVersion()) @@ -373,6 +391,7 @@ func setup() { "github.com/tsenart/deadcode", "golang.org/x/net/html", "golang.org/x/tools/cmd/cover", + "honnef.co/go/simple/cmd/gosimple", "honnef.co/go/staticcheck/cmd/staticcheck", "honnef.co/go/unused/cmd/unused", } @@ -1009,48 +1028,6 @@ func zipFile(out string, files []archiveFile) { } } -func vet(dirs ...string) { - params := []string{"tool", "vet", "-all"} - params = append(params, dirs...) - bs, err := runError("go", params...) - - if len(bs) > 0 { - log.Printf("%s", bs) - } - - if err != nil { - if exitStatus(err) == 3 { - // Exit code 3, the "vet" tool is not installed - return - } - - // A genuine error exit from the vet tool. - log.Fatal(err) - } -} - -func lint(pkg string) { - bs, err := runError("golint", pkg) - if err != nil { - log.Println(`- No golint, not linting. Try "go get -u github.com/golang/lint/golint".`) - return - } - - analCommentPolicy := regexp.MustCompile(`exported (function|method|const|type|var) [^\s]+ should have comment`) - for _, line := range strings.Split(string(bs), "\n") { - if line == "" { - continue - } - if analCommentPolicy.MatchString(line) { - continue - } - if strings.Contains(line, ".pb.go:") { - continue - } - log.Println(line) - } -} - func macosCodesign(file string) { if pass := os.Getenv("CODESIGN_KEYCHAIN_PASS"); pass != "" { bs, err := runError("security", "unlock-keychain", "-p", pass) @@ -1070,14 +1047,16 @@ func macosCodesign(file string) { } } -func exitStatus(err error) int { - if err, ok := err.(*exec.ExitError); ok { - if ws, ok := err.ProcessState.Sys().(syscall.WaitStatus); ok { - return ws.ExitStatus() +func metalint(linters []string, dirs []string) { + ok := true + if isGometalinterInstalled() { + if !gometalinter(linters, dirs, lintExcludes...) { + ok = false } } - - return -1 + if !ok { + log.Fatal("Build succeeded, but there were lint warnings") + } } func isGometalinterInstalled() bool { @@ -1088,10 +1067,12 @@ func isGometalinterInstalled() bool { return true } -func gometalinter(linter string, dirs []string, excludes ...string) bool { - params := []string{"--disable-all"} - params = append(params, fmt.Sprintf("--deadline=%ds", 60)) - params = append(params, "--enable="+linter) +func gometalinter(linters []string, dirs []string, excludes ...string) bool { + params := []string{"--disable-all", "--concurrency=2", "--deadline=300s"} + + for _, linter := range linters { + params = append(params, "--enable="+linter) + } for _, exclude := range excludes { params = append(params, "--exclude="+exclude) @@ -1104,14 +1085,19 @@ func gometalinter(linter string, dirs []string, excludes ...string) bool { bs, _ := runError("gometalinter", params...) nerr := 0 + lines := make(map[string]struct{}) for _, line := range strings.Split(string(bs), "\n") { if line == "" { continue } - if strings.Contains(line, ".pb.go:") { + if _, ok := lines[line]; ok { continue } log.Println(line) + if strings.Contains(line, "executable file not found") { + log.Println(` - Try "go run build.go setup" to install missing tools`) + } + lines[line] = struct{}{} nerr++ } diff --git a/cmd/stcli/cmd_general.go b/cmd/stcli/cmd_general.go index 2f6be3f15..aa9add3ab 100644 --- a/cmd/stcli/cmd_general.go +++ b/cmd/stcli/cmd_general.go @@ -62,9 +62,9 @@ func generalID(c *cli.Context) { func generalStatus(c *cli.Context) { response := httpGet(c, "system/config/insync") - status := make(map[string]interface{}) + var status struct{ ConfigInSync bool } json.Unmarshal(responseToBArray(response), &status) - if status["configInSync"] != true { + if !status.ConfigInSync { die("Config out of sync") } fmt.Println("Config in sync") diff --git a/cmd/stgenfiles/main.go b/cmd/stgenfiles/main.go index 5e89150fa..11869ce25 100644 --- a/cmd/stgenfiles/main.go +++ b/cmd/stgenfiles/main.go @@ -85,12 +85,7 @@ func generateOneFile(fd io.ReadSeeker, p1 string, s int64) error { _ = os.Chmod(p1, os.FileMode(rand.Intn(0777)|0400)) t := time.Now().Add(-time.Duration(rand.Intn(30*86400)) * time.Second) - err = os.Chtimes(p1, t, t) - if err != nil { - return err - } - - return nil + return os.Chtimes(p1, t, t) } func randomName() string { diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index 762ff7551..c6d4a8172 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -381,10 +381,8 @@ func (s *apiService) String() string { } func (s *apiService) VerifyConfiguration(from, to config.Configuration) error { - if _, err := net.ResolveTCPAddr("tcp", to.GUI.Address()); err != nil { - return err - } - return nil + _, err := net.ResolveTCPAddr("tcp", to.GUI.Address()) + return err } func (s *apiService) CommitConfiguration(from, to config.Configuration) bool { diff --git a/lib/config/folderconfiguration.go b/lib/config/folderconfiguration.go index dc54b8a99..74fc7f6bf 100644 --- a/lib/config/folderconfiguration.go +++ b/lib/config/folderconfiguration.go @@ -99,10 +99,7 @@ func (f *FolderConfiguration) CreateMarker() error { func (f *FolderConfiguration) HasMarker() bool { _, err := os.Stat(filepath.Join(f.Path(), ".stfolder")) - if err != nil { - return false - } - return true + return err == nil } func (f FolderConfiguration) Description() string { diff --git a/lib/db/blockmap_test.go b/lib/db/blockmap_test.go index 27eee44e3..caa103191 100644 --- a/lib/db/blockmap_test.go +++ b/lib/db/blockmap_test.go @@ -58,10 +58,7 @@ func setup() (*Instance, *BlockFinder) { func dbEmpty(db *Instance) bool { iter := db.NewIterator(util.BytesPrefix([]byte{KeyTypeBlock}), nil) defer iter.Release() - if iter.Next() { - return false - } - return true + return !iter.Next() } func TestBlockMapAddUpdateWipe(t *testing.T) { diff --git a/lib/db/concurrency_test.go b/lib/db/concurrency_test.go index 01792be00..47378e510 100644 --- a/lib/db/concurrency_test.go +++ b/lib/db/concurrency_test.go @@ -4,7 +4,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this file, // You can obtain one at http://mozilla.org/MPL/2.0/. -// +build ignore // this is a really tedious test for an old issue +// this is a really tedious test for an old issue +// +build ignore package db_test diff --git a/lib/discover/local.go b/lib/discover/local.go index 36fff6a05..6003a2255 100644 --- a/lib/discover/local.go +++ b/lib/discover/local.go @@ -51,7 +51,7 @@ func NewLocal(id protocol.DeviceID, addr string, addrList AddressLister) (Finder Supervisor: suture.NewSimple("local"), myID: id, addrList: addrList, - localBcastTick: time.Tick(BroadcastInterval), + localBcastTick: time.NewTicker(BroadcastInterval).C, forcedBcastTick: make(chan time.Time), localBcastStart: time.Now(), cache: newCache(), diff --git a/lib/events/events_test.go b/lib/events/events_test.go index b04bae139..79768a5cb 100644 --- a/lib/events/events_test.go +++ b/lib/events/events_test.go @@ -247,22 +247,23 @@ func BenchmarkBufferedSub(b *testing.B) { } // Receive the events - done := make(chan struct{}) + done := make(chan error) go func() { - defer close(done) recv := 0 var evs []Event for i := 0; i < b.N; { evs = bs.Since(recv, evs[:0]) for _, ev := range evs { if ev.GlobalID != recv+1 { - b.Fatal("skipped event", ev.GlobalID, recv) + done <- fmt.Errorf("skipped event %v %v", ev.GlobalID, recv) + return } recv = ev.GlobalID coord <- struct{}{} } i += len(evs) } + done <- nil }() // Send the events @@ -276,7 +277,9 @@ func BenchmarkBufferedSub(b *testing.B) { <-coord } - <-done + if err := <-done; err != nil { + b.Error(err) + } b.ReportAllocs() } diff --git a/lib/ignore/cache_test.go b/lib/ignore/cache_test.go index 647d61712..ca114b04e 100644 --- a/lib/ignore/cache_test.go +++ b/lib/ignore/cache_test.go @@ -15,7 +15,7 @@ func TestCache(t *testing.T) { c := newCache(nil) res, ok := c.get("nonexistent") - if res.IsIgnored() || res.IsDeletable() || ok != false { + if res.IsIgnored() || res.IsDeletable() || ok { t.Errorf("res %v, ok %v for nonexistent item", res, ok) } @@ -25,12 +25,12 @@ func TestCache(t *testing.T) { c.set("false", 0) res, ok = c.get("true") - if !res.IsIgnored() || !res.IsDeletable() || ok != true { + if !res.IsIgnored() || !res.IsDeletable() || !ok { t.Errorf("res %v, ok %v for true item", res, ok) } res, ok = c.get("false") - if res.IsIgnored() || res.IsDeletable() || ok != true { + if res.IsIgnored() || res.IsDeletable() || !ok { t.Errorf("res %v, ok %v for false item", res, ok) } @@ -41,12 +41,12 @@ func TestCache(t *testing.T) { // Same values should exist res, ok = c.get("true") - if !res.IsIgnored() || !res.IsDeletable() || ok != true { + if !res.IsIgnored() || !res.IsDeletable() || !ok { t.Errorf("res %v, ok %v for true item", res, ok) } res, ok = c.get("false") - if res.IsIgnored() || res.IsDeletable() || ok != true { + if res.IsIgnored() || res.IsDeletable() || !ok { t.Errorf("res %v, ok %v for false item", res, ok) } diff --git a/lib/model/util.go b/lib/model/util.go index ec191dbe1..21b07cf11 100644 --- a/lib/model/util.go +++ b/lib/model/util.go @@ -37,6 +37,7 @@ func (d *deadlockDetector) Watch(name string, mut sync.Locker) { go func() { mut.Lock() + _ = 1 // empty critical section mut.Unlock() ok <- true }() diff --git a/lib/osutil/sync.go b/lib/osutil/sync.go index 97f3ec890..fd7136101 100644 --- a/lib/osutil/sync.go +++ b/lib/osutil/sync.go @@ -22,10 +22,7 @@ func SyncFile(path string) error { } defer fd.Close() // MacOS and Windows do not flush the disk cache - if err := fd.Sync(); err != nil { - return err - } - return nil + return fd.Sync() } func SyncDir(path string) error { diff --git a/lib/protocol/bufferpool.go b/lib/protocol/bufferpool.go index ccb18363d..af7e8d2d6 100644 --- a/lib/protocol/bufferpool.go +++ b/lib/protocol/bufferpool.go @@ -17,10 +17,10 @@ func (p *bufferPool) get(size int) []byte { return p.new(size) } - bs := intf.([]byte) + bs := *intf.(*[]byte) if cap(bs) < size { // Buffer was too small, leave it for someone else and allocate. - p.put(bs) + p.pool.Put(intf) return p.new(size) } @@ -43,7 +43,7 @@ func (p *bufferPool) upgrade(bs []byte, size int) []byte { // put returns the buffer to the pool func (p *bufferPool) put(bs []byte) { - p.pool.Put(bs) + p.pool.Put(&bs) } // new creates a new buffer of the requested size, taking the minimum diff --git a/lib/protocol/hello.go b/lib/protocol/hello.go index a8a95bf00..0e9cb2f03 100644 --- a/lib/protocol/hello.go +++ b/lib/protocol/hello.go @@ -105,11 +105,7 @@ func readHello(c io.Reader) (HelloResult, error) { if err := hello.UnmarshalXDR(buf); err != nil { return HelloResult{}, err } - res := HelloResult{ - DeviceName: hello.DeviceName, - ClientName: hello.ClientName, - ClientVersion: hello.ClientVersion, - } + res := HelloResult(hello) return res, ErrTooOldVersion13 case 0x00010001, 0x00010000: diff --git a/lib/protocol/protocol.go b/lib/protocol/protocol.go index b955e79d9..073c95c45 100644 --- a/lib/protocol/protocol.go +++ b/lib/protocol/protocol.go @@ -516,7 +516,7 @@ func (c *rawConnection) handleRequest(req Request) { var done chan struct{} if usePool { - buf = c.pool.Get().([]byte)[:size] + buf = (*c.pool.Get().(*[]byte))[:size] done = make(chan struct{}) } else { buf = make([]byte, size) @@ -539,7 +539,7 @@ func (c *rawConnection) handleRequest(req Request) { if usePool { <-done - c.pool.Put(buf) + c.pool.Put(&buf) } } @@ -762,11 +762,12 @@ func (c *rawConnection) close(err error) { // results in an effecting ping interval of somewhere between // PingSendInterval/2 and PingSendInterval. func (c *rawConnection) pingSender() { - ticker := time.Tick(PingSendInterval / 2) + ticker := time.NewTicker(PingSendInterval / 2) + defer ticker.Stop() for { select { - case <-ticker: + case <-ticker.C: d := time.Since(c.cw.Last()) if d < PingSendInterval/2 { l.Debugln(c.id, "ping skipped after wr", d) @@ -786,11 +787,12 @@ func (c *rawConnection) pingSender() { // but we expect pings in the absence of other messages) within the last // ReceiveTimeout. If not, we close the connection with an ErrTimeout. func (c *rawConnection) pingReceiver() { - ticker := time.Tick(ReceiveTimeout / 2) + ticker := time.NewTicker(ReceiveTimeout / 2) + defer ticker.Stop() for { select { - case <-ticker: + case <-ticker.C: d := time.Since(c.cr.Last()) if d > ReceiveTimeout { l.Debugln(c.id, "ping timeout", d) diff --git a/lib/sync/sync.go b/lib/sync/sync.go index 942cfcdb6..182b9164e 100644 --- a/lib/sync/sync.go +++ b/lib/sync/sync.go @@ -80,7 +80,7 @@ func (h holder) String() string { if h.at == "" { return "not held" } - return fmt.Sprintf("at %s goid: %d for %s", h.at, h.goid, time.Now().Sub(h.time)) + return fmt.Sprintf("at %s goid: %d for %s", h.at, h.goid, time.Since(h.time)) } type loggedMutex struct { @@ -95,7 +95,7 @@ func (m *loggedMutex) Lock() { func (m *loggedMutex) Unlock() { currentHolder := m.holder.Load().(holder) - duration := time.Now().Sub(currentHolder.time) + duration := time.Since(currentHolder.time) if duration >= threshold { l.Debugf("Mutex held for %v. Locked at %s unlocked at %s", duration, currentHolder.at, getHolder().at) } @@ -147,7 +147,7 @@ func (m *loggedRWMutex) Lock() { func (m *loggedRWMutex) Unlock() { currentHolder := m.holder.Load().(holder) - duration := time.Now().Sub(currentHolder.time) + duration := time.Since(currentHolder.time) if duration >= threshold { l.Debugf("RWMutex held for %v. Locked at %s unlocked at %s", duration, currentHolder.at, getHolder().at) } @@ -201,7 +201,7 @@ type loggedWaitGroup struct { func (wg *loggedWaitGroup) Wait() { start := time.Now() wg.WaitGroup.Wait() - duration := time.Now().Sub(start) + duration := time.Since(start) if duration >= threshold { l.Debugf("WaitGroup took %v at %s", duration, getHolder()) } diff --git a/lib/util/utils_test.go b/lib/util/utils_test.go index 342d16c35..293c79e2a 100644 --- a/lib/util/utils_test.go +++ b/lib/util/utils_test.go @@ -22,7 +22,7 @@ func TestSetDefaults(t *testing.T) { t.Error("int failed") } else if x.C != 0 { t.Errorf("float failed") - } else if x.D != false { + } else if x.D { t.Errorf("bool failed") } @@ -36,7 +36,7 @@ func TestSetDefaults(t *testing.T) { t.Error("int failed") } else if x.C != 2.2 { t.Errorf("float failed") - } else if x.D != true { + } else if !x.D { t.Errorf("bool failed") } } diff --git a/lib/versioner/staggered.go b/lib/versioner/staggered.go index 4dcacbcd7..bfe8099bb 100644 --- a/lib/versioner/staggered.go +++ b/lib/versioner/staggered.go @@ -73,11 +73,14 @@ func NewStaggered(folderID, folderPath string, params map[string]string) Version l.Debugf("instantiated %#v", s) go func() { + // TODO: This should be converted to a Serve() method. s.clean() if testCleanDone != nil { close(testCleanDone) } - for range time.Tick(time.Duration(cleanInterval) * time.Second) { + tck := time.NewTicker(time.Duration(cleanInterval) * time.Second) + defer tck.Stop() + for range tck.C { s.clean() } }() diff --git a/test/sync_test.go b/test/sync_test.go index e67fb262a..c635df208 100644 --- a/test/sync_test.go +++ b/test/sync_test.go @@ -17,6 +17,8 @@ import ( "testing" "time" + "io" + "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/protocol" "github.com/syncthing/syncthing/lib/rc"