jenkins: avoid using goimports and distribute load better (#2359)

.golangci.yml

@@ -20,7 +20,8 @@ linters:
     - deadcode    # find code that is not used
     #TODO#- unparam     # check for unused parameters
     - nakedret    # check for naked returns
-    - goimports   # fix import order
+    #- goimports   # fix import order, disabled because it's slow
+    - gofmt
     - misspell    # check spelling
     - unconvert   # remove unnecessary conversions
     - scopelint   # checks for unpinned variables
@@ -28,6 +29,7 @@ linters:
     #TODO#- maligned    # check for better memory usage
     #TODO#- prealloc    # easy optimizations
   disable:
+    - goimports
     - goconst     # check for things that could be replaced by constants
     - gocyclo     # needs tweaking
     - depguard    # unused

Jenkinsfile.gerrit

@@ -46,7 +46,7 @@ pipeline {
                         sh 'go run ./scripts/protobuf.go --protoc=$HOME/protoc/bin/protoc lint'
                         sh 'go run ./scripts/protobuf.go --protoc=$HOME/protoc/bin/protoc check-lock'
                         sh 'bash ./scripts/check-dbx-version.sh'
-                        sh 'golangci-lint -j=4 run'
+                        sh 'golangci-lint -j=2 run'
                         sh 'go run scripts/check-mod-tidy.go -mod .build/go.mod.orig'
                         sh 'make check-satellite-config-lock'
                     }

Jenkinsfile.public

@@ -21,7 +21,7 @@ pipeline {
                 // download dependencies
                 sh 'go mod download'
 
-                sh 'go install -v -race ./...'
+                sh 'make -j2 build-packages'
                 sh 'make install-sim'
 
                 sh 'service postgresql start'
@@ -39,7 +39,7 @@ pipeline {
                         sh 'go run ./scripts/protobuf.go --protoc=$HOME/protoc/bin/protoc lint'
                         sh 'go run ./scripts/protobuf.go --protoc=$HOME/protoc/bin/protoc check-lock'
                         sh 'bash ./scripts/check-dbx-version.sh'
-                        sh 'golangci-lint -j=4 run'
+                        sh 'golangci-lint -j=2 run --verbose'
                         sh 'go run scripts/check-mod-tidy.go -mod .build/go.mod.orig'
                         sh 'make check-satellite-config-lock'
                     }

Makefile

@@ -71,6 +71,13 @@ proto: ## Rebuild protobuf files
 	go run scripts/protobuf.go install
 	go run scripts/protobuf.go generate
 
+.PHONY: build-packages
+build-packages: build-packages-race build-packages-normal ## Test docker images locally
+build-packages-race:
+	go install -v ./...
+build-packages-normal:
+	go install -v -race ./...
+
 ##@ Simulator
 
 .PHONY: install-sim

pkg/overlay/selection_test.go

@@ -41,7 +41,7 @@ func TestOffline(t *testing.T) {
 
 		result, err = service.KnownUnreliableOrOffline(ctx, []storj.NodeID{
 			planet.StorageNodes[0].ID(),
-			storj.NodeID{1, 2, 3, 4}, //note that this succeeds by design
+			{1, 2, 3, 4}, //note that this succeeds by design
 			planet.StorageNodes[2].ID(),
 		})
 		require.NoError(t, err)

satellite/metainfo/metainfo_test.go

@@ -661,10 +661,10 @@ func createTestPointer(t *testing.T) *pb.Pointer {
 		Remote: &pb.RemoteSegment{
 			Redundancy: rs,
 			RemotePieces: []*pb.RemotePiece{
-				&pb.RemotePiece{
+				{
 					PieceNum: 0,
 				},
-				&pb.RemotePiece{
+				{
 					PieceNum: 1,
 				},
 			},

scripts/check-imports.go

@@ -13,6 +13,7 @@ import (
 	"go/token"
 	"io"
 	"os"
+	"reflect"
 	"runtime"
 	"sort"
 	"strconv"
@@ -58,6 +59,7 @@ func main() {
 
 	fmt.Println("checking import order:")
 
+	// load all packages
 	seen := map[*packages.Package]bool{}
 	pkgs := []*packages.Package{}
 
@@ -81,51 +83,205 @@ func main() {
 		visit(pkg)
 	}
 
+	// sort the packages
 	sort.Slice(pkgs, func(i, k int) bool { return pkgs[i].ID < pkgs[k].ID })
-	correct := true
-	incorrectPkgs := []string{}
+
+	var misgrouped, unsorted []Imports
 	for _, pkg := range pkgs {
-		if !correctPackage(pkg) {
-			incorrectPkgs = append(incorrectPkgs, pkg.PkgPath)
-			correct = false
+		pkgmisgrouped, pkgunsorted := verifyPackage(os.Stderr, pkg)
+
+		misgrouped = append(misgrouped, pkgmisgrouped...)
+		unsorted = append(unsorted, pkgunsorted...)
+	}
+
+	exitCode := 0
+	if len(misgrouped) > 0 {
+		exitCode = 1
+
+		fmt.Fprintln(os.Stderr)
+		fmt.Fprintln(os.Stderr, "Imports are not in the standard grouping [std storj other]:")
+		for _, imports := range misgrouped {
+			fmt.Fprintln(os.Stderr, "\t"+imports.Path, imports.Classes())
 		}
 	}
 
-	if !correct {
-		fmt.Fprintln(os.Stderr)
-		fmt.Fprintln(os.Stderr, "Error: imports are not in the correct order for package/s: ")
-		for _, pkg := range incorrectPkgs {
-			fmt.Fprintln(os.Stderr, "\t"+pkg)
-		}
+	if len(unsorted) > 0 {
+		exitCode = 1
 
 		fmt.Fprintln(os.Stderr)
-		fmt.Fprintln(os.Stderr, "Correct order should be: \n\tstd packages -> external packages -> storj.io packages.")
-		os.Exit(1)
+		fmt.Fprintln(os.Stderr, "Imports are not sorted:")
+		for _, imports := range unsorted {
+			fmt.Fprintln(os.Stderr, "\t"+imports.Path)
 		}
+	}
+
+	os.Exit(exitCode)
 }
 
-func correctPackage(pkg *packages.Package) bool {
+func verifyPackage(stderr io.Writer, pkg *packages.Package) (misgrouped, unsorted []Imports) {
 	// ignore generated test binaries
 	if strings.HasSuffix(pkg.ID, ".test") {
-		return true
+		return
 	}
 
-	correct := true
 	for i, file := range pkg.Syntax {
 		path := pkg.CompiledGoFiles[i]
-		if !correctImports(pkg.Fset, path, file) {
-			if !isGenerated(path) { // ignore generated files
-				fmt.Fprintln(os.Stderr, path)
-				correct = false
-			} else {
-				fmt.Fprintln(os.Stderr, "(ignoring generated)", path)
+
+		imports := LoadImports(pkg.Fset, path, file)
+
+		ordered := true
+		sorted := true
+		for _, section := range imports.Decls {
+			if !section.IsGrouped() {
+				ordered = false
+			}
+			if !section.IsSorted() {
+				sorted = false
 			}
 		}
+
+		if !ordered || !sorted {
+			if isGenerated(path) {
+				fmt.Fprintln(stderr, "(ignoring generated)", path)
+				continue
 			}
-	return correct
+		}
+
+		if !ordered {
+			misgrouped = append(misgrouped, imports)
+		}
+		if !sorted {
+			unsorted = append(unsorted, imports)
+		}
+	}
+
+	return
 }
 
-func correctImports(fset *token.FileSet, name string, f *ast.File) bool {
+// Imports defines all imports for a single file.
+type Imports struct {
+	Path      string
+	Generated bool
+	Decls     []ImportDecl
+}
+
+// Classes returns all import groupings
+func (imports Imports) Classes() [][]Class {
+	var classes [][]Class
+	for _, decl := range imports.Decls {
+		classes = append(classes, decl.Classes())
+	}
+	return classes
+}
+
+// ImportDecl defines a single import declaration
+type ImportDecl []ImportGroup
+
+// allowedGroups lists all valid groupings
+var allowedGroups = [][]Class{
+	{Standard},
+	{Storj},
+	{Other},
+	{Standard, Storj},
+	{Standard, Other},
+	{Other, Storj},
+	{Standard, Other, Storj},
+}
+
+// IsGrouped returns whether the grouping is allowed.
+func (decls ImportDecl) IsGrouped() bool {
+	classes := decls.Classes()
+	for _, allowedGroup := range allowedGroups {
+		if reflect.DeepEqual(allowedGroup, classes) {
+			return true
+		}
+	}
+	return false
+}
+
+// Classes returns each group class.
+func (decl ImportDecl) Classes() []Class {
+	classes := make([]Class, len(decl))
+	for i := range classes {
+		classes[i] = decl[i].Class()
+	}
+	return classes
+}
+
+// IsSorted returns whether the group is sorted.
+func (decls ImportDecl) IsSorted() bool {
+	for _, decl := range decls {
+		if !decl.IsSorted() {
+			return false
+		}
+	}
+	return true
+}
+
+// ImportGroup defines a single import statement.
+type ImportGroup struct {
+	Specs []*ast.ImportSpec
+	Paths []string
+}
+
+// IsSorted returns whether the group is sorted.
+func (group ImportGroup) IsSorted() bool {
+	return sort.StringsAreSorted(group.Paths)
+}
+
+// Class returns the classification of this import group.
+func (group ImportGroup) Class() Class {
+	var class Class
+	for _, path := range group.Paths {
+		class |= ClassifyImport(path)
+	}
+	return class
+}
+
+// Class defines a bitset of import classification
+type Class byte
+
+// Class defines three different groups
+const (
+	// Standard is all go standard packages
+	Standard Class = 1 << iota
+	// Storj is imports that start with `storj.io`
+	Storj
+	// Other is everything else
+	Other
+)
+
+// ClassifyImport classifies an import path to a class.
+func ClassifyImport(pkgPath string) Class {
+	if strings.HasPrefix(pkgPath, "storj.io/") {
+		return Storj
+	}
+	if stdlib[pkgPath] {
+		return Standard
+	}
+	return Other
+}
+
+// String returns contents of the class.
+func (class Class) String() string {
+	var s []string
+	if class&Standard != 0 {
+		s = append(s, "std")
+	}
+	if class&Storj != 0 {
+		s = append(s, "storj")
+	}
+	if class&Other != 0 {
+		s = append(s, "other")
+	}
+	return strings.Join(s, "|")
+}
+
+// LoadImports loads import groups from a given fileset.
+func LoadImports(fset *token.FileSet, name string, f *ast.File) Imports {
+	var imports Imports
+	imports.Path = name
+
 	for _, d := range f.Decls {
 		d, ok := d.(*ast.GenDecl)
 		if !ok || d.Tok != token.IMPORT {
@@ -139,7 +295,7 @@ func correctImports(fset *token.FileSet, name string, f *ast.File) bool {
 			continue
 		}
 
-		// Identify and sort runs of specs on successive lines.
+		// identify specs on successive lines
 		lastGroup := 0
 		specgroups := [][]ast.Spec{}
 		for i, s := range d.Specs {
@@ -149,82 +305,28 @@ func correctImports(fset *token.FileSet, name string, f *ast.File) bool {
 				lastGroup = i
 			}
 		}
-
 		specgroups = append(specgroups, d.Specs[lastGroup:])
 
-		if !correctOrder(specgroups) {
-			return false
-		}
-	}
-	return true
-}
-
-func correctOrder(specgroups [][]ast.Spec) bool {
-	if len(specgroups) == 0 {
-		return true
-	}
-
-	// remove std group from beginning
-	std, other, storj := countGroup(specgroups[0])
-	if std > 0 {
-		if other+storj != 0 {
-			return false
-		}
-		specgroups = specgroups[1:]
-	}
-
-	if len(specgroups) == 0 {
-		return true
-	}
-
-	// remove storj.io group from the end
-	std, other, storj = countGroup(specgroups[len(specgroups)-1])
-	if storj > 0 {
-		if std+other > 0 {
-			return false
-		}
-		specgroups = specgroups[:len(specgroups)-1]
-	}
-	if len(specgroups) == 0 {
-		return true
-	}
-
-	// check that we have a center group for misc stuff
-	if len(specgroups) != 1 {
-		return false
-	}
-
-	std, other, storj = countGroup(specgroups[0])
-	return other >= 0 && std+storj == 0
-}
-
-func printAll(groups [][]ast.Spec) {
-	defer fmt.Println("---")
-	for i, group := range groups {
-		fmt.Println("===", i)
-		for _, imp := range group {
-			imp := imp.(*ast.ImportSpec)
-			fmt.Println("\t", imp.Path.Value)
-		}
-	}
-}
-
-func countGroup(p []ast.Spec) (std, other, storj int) {
-	for _, imp := range p {
-		imp := imp.(*ast.ImportSpec)
-		path, err := strconv.Unquote(imp.Path.Value)
+		// convert ast.Spec-s groups into import groups
+		var decl ImportDecl
+		for _, specgroup := range specgroups {
+			var group ImportGroup
+			for _, importSpec := range specgroup {
+				importSpec := importSpec.(*ast.ImportSpec)
+				path, err := strconv.Unquote(importSpec.Path.Value)
 				if err != nil {
 					panic(err)
 				}
-		if strings.HasPrefix(path, "storj.io/") {
-			storj++
-		} else if stdlib[path] {
-			std++
-		} else {
-			other++
+				group.Specs = append(group.Specs, importSpec)
+				group.Paths = append(group.Paths, path)
 			}
+			decl = append(decl, group)
 		}
-	return std, other, storj
+
+		imports.Decls = append(imports.Decls, decl)
+	}
+
+	return imports
 }
 
 var root = runtime.GOROOT()

storagenode/orders/db_test.go

@@ -90,7 +90,7 @@ func TestOrders(t *testing.T) {
 		require.NoError(t, err)
 
 		expectedGrouped := map[storj.NodeID][]*orders.Info{
-			satellite0.ID: []*orders.Info{
+			satellite0.ID: {
 				{Limit: limit, Order: order},
 			},
 		}