speed up CI and golangci-lint (#1070)

Run CI on mac os only with go1.13 and on windows only on go1.14.
Speed up tests. Introduce --allow-parallel-runners.
Block on parallel run lock 5s instead of 60s.
Don't invalidate analysis cache for minor config changes.

.github/workflows/pr.yml

@@ -23,17 +23,12 @@ jobs:
   tests-on-windows:
     needs: golangci-lint # run after golangci-lint action to not produce duplicated errors
     runs-on: windows-latest
-    strategy:
-      matrix:
-        golang:
-          - 1.13
-          - 1.14
     steps:
       - uses: actions/checkout@v2
       - name: Install Go
         uses: actions/setup-go@v2
         with:
-          go-version: ${{ matrix.golang }}
+          go-version: 1.14 # test only the latest go version to speed up CI
       - name: Run tests on Windows
         run: make.exe test
         continue-on-error: true
@@ -47,7 +42,11 @@ jobs:
           - 1.14
         os:
           - ubuntu-latest
-          - macos-latest
+        include:
+          - os: macos-latest
+            # test only the one go version on Mac OS to speed up CI
+            # TODO: use the latet go version after https://github.com/golang/go/issues/38824
+            golang: 1.13
     steps:
       - uses: actions/checkout@v2
       - name: Install Go

.golangci.example.yml

@@ -53,6 +53,10 @@ run:
   # the dependency descriptions in go.mod.
   modules-download-mode: readonly|release|vendor
 
+  # Allow multiple parallel golangci-lint instances running.
+  # If false (default) - golangci-lint acquires file lock on start.
+  allow-parallel-runners: false
+
 
 # output configuration options
 output:

Makefile

@@ -29,9 +29,7 @@ clean:
 test: export GOLANGCI_LINT_INSTALLED = true
 test: build
 	GL_TEST_RUN=1 ./golangci-lint run -v
-	GL_TEST_RUN=1 ./golangci-lint run --fast --no-config -v --skip-dirs 'test/testdata_etc,internal/(cache|renameio|robustio)'
-	GL_TEST_RUN=1 ./golangci-lint run --no-config -v --skip-dirs 'test/testdata_etc,internal/(cache|renameio|robustio)'
-	GL_TEST_RUN=1 go test -v ./...
+	GL_TEST_RUN=1 go test -v -parallel 2 ./...
 .PHONY: test
 
 test_race: build_race

README.md

@@ -546,6 +546,7 @@ Flags:
                                          - (^|/)builtin($|/)
                                         (default true)
       --skip-files strings             Regexps of files to skip
+      --allow-parallel-runners         Allow multiple parallel golangci-lint instances running. If false (default) - golangci-lint acquires file lock on start.
   -E, --enable strings                 Enable specific linter
   -D, --disable strings                Disable specific linter
       --disable-all                    Disable all linters
@@ -679,6 +680,10 @@ run:
   # the dependency descriptions in go.mod.
   modules-download-mode: readonly|release|vendor
 
+  # Allow multiple parallel golangci-lint instances running.
+  # If false (default) - golangci-lint acquires file lock on start.
+  allow-parallel-runners: false
+
 
 # output configuration options
 output:

internal/cache/cache_test.go

@@ -20,6 +20,8 @@ func init() {
 }
 
 func TestBasic(t *testing.T) {
+	t.Parallel()
+
 	dir, err := ioutil.TempDir("", "cachetest-")
 	if err != nil {
 		t.Fatal(err)
@@ -65,6 +67,8 @@ func TestBasic(t *testing.T) {
 }
 
 func TestGrowth(t *testing.T) {
+	t.Parallel()
+
 	dir, err := ioutil.TempDir("", "cachetest-")
 	if err != nil {
 		t.Fatal(err)
@@ -151,6 +155,8 @@ func dummyID(x int) [HashSize]byte {
 }
 
 func TestCacheTrim(t *testing.T) {
+	t.Parallel()
+
 	dir, err := ioutil.TempDir("", "cachetest-")
 	if err != nil {
 		t.Fatal(err)

pkg/commands/config.go

@@ -35,22 +35,32 @@ func (e *Executor) initConfig() {
 	cmd.AddCommand(pathCmd)
 }
 
+func (e *Executor) getUsedConfig() string {
+	usedConfigFile := viper.ConfigFileUsed()
+	if usedConfigFile == "" {
+		return ""
+	}
+
+	prettyUsedConfigFile, err := fsutils.ShortestRelPath(usedConfigFile, "")
+	if err != nil {
+		e.log.Warnf("Can't pretty print config file path: %s", err)
+		return usedConfigFile
+	}
+
+	return prettyUsedConfigFile
+}
+
 func (e *Executor) executePathCmd(_ *cobra.Command, args []string) {
 	if len(args) != 0 {
 		e.log.Fatalf("Usage: golangci-lint config path")
 	}
 
-	usedConfigFile := viper.ConfigFileUsed()
+	usedConfigFile := e.getUsedConfig()
 	if usedConfigFile == "" {
 		e.log.Warnf("No config file detected")
 		os.Exit(exitcodes.NoConfigFileDetected)
 	}
 
-	usedConfigFile, err := fsutils.ShortestRelPath(usedConfigFile, "")
-	if err != nil {
-		e.log.Warnf("Can't pretty print config file path: %s", err)
-	}
-
 	fmt.Println(usedConfigFile)
 	os.Exit(0)
 }

pkg/commands/executor.go

@@ -5,10 +5,10 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/json"
-	"fmt"
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/fatih/color"
@@ -33,6 +33,7 @@ import (
 type Executor struct {
 	rootCmd    *cobra.Command
 	runCmd     *cobra.Command
+	lintersCmd *cobra.Command
 
 	exitCode              int
 	version, commit, date string
@@ -55,6 +56,7 @@ type Executor struct {
 }
 
 func NewExecutor(version, commit, date string) *Executor {
+	startedAt := time.Now()
 	e := &Executor{
 		cfg:       config.NewDefault(),
 		version:   version,
@@ -66,9 +68,6 @@ func NewExecutor(version, commit, date string) *Executor {
 
 	e.debugf("Starting execution...")
 	e.log = report.NewLogWrapper(logutils.NewStderrLog(""), &e.reportData)
-	if ok := e.acquireFileLock(); !ok {
-		e.log.Fatalf("Parallel golangci-lint is running")
-	}
 
 	// to setup log level early we need to parse config from command line extra time to
 	// find `-v` option
@@ -121,6 +120,7 @@ func NewExecutor(version, commit, date string) *Executor {
 
 	// Slice options must be explicitly set for proper merging of config and command-line options.
 	fixSlicesFlags(e.runCmd.Flags())
+	fixSlicesFlags(e.lintersCmd.Flags())
 
 	e.EnabledLintersSet = lintersdb.NewEnabledSet(e.DBManager,
 		lintersdb.NewValidator(e.DBManager), e.log.Child("lintersdb"), e.cfg)
@@ -139,7 +139,7 @@ func NewExecutor(version, commit, date string) *Executor {
 	if err = e.initHashSalt(version); err != nil {
 		e.log.Fatalf("Failed to init hash salt: %s", err)
 	}
-	e.debugf("Initialized executor")
+	e.debugf("Initialized executor in %s", time.Since(startedAt))
 	return e
 }
 
@@ -191,27 +191,39 @@ func computeBinarySalt(version string) ([]byte, error) {
 }
 
 func computeConfigSalt(cfg *config.Config) ([]byte, error) {
-	configBytes, err := json.Marshal(cfg)
+	// We don't hash all config fields to reduce meaningless cache
+	// invalidations. At least, it has a huge impact on tests speed.
+
+	lintersSettingsBytes, err := json.Marshal(cfg.LintersSettings)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to json marshal config")
+		return nil, errors.Wrap(err, "failed to json marshal config linter settings")
 	}
 
+	var configData bytes.Buffer
+	configData.WriteString("linters-settings=")
+	configData.Write(lintersSettingsBytes)
+	configData.WriteString("\nbuild-tags=%s" + strings.Join(cfg.Run.BuildTags, ","))
+
 	h := sha256.New()
-	if n, err := h.Write(configBytes); n != len(configBytes) {
-		return nil, fmt.Errorf("failed to hash config bytes: wrote %d/%d bytes, error: %s", n, len(configBytes), err)
-	}
+	h.Write(configData.Bytes()) //nolint:errcheck
 	return h.Sum(nil), nil
 }
 
 func (e *Executor) acquireFileLock() bool {
+	if e.cfg.Run.AllowParallelRunners {
+		e.debugf("Parallel runners are allowed, no locking")
+		return true
+	}
+
 	lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock")
 	e.debugf("Locking on file %s...", lockFile)
 	f := flock.New(lockFile)
-	ctx, finish := context.WithTimeout(context.Background(), time.Minute)
+	const totalTimeout = 5 * time.Second
+	const retryDelay = time.Second
+	ctx, finish := context.WithTimeout(context.Background(), totalTimeout)
 	defer finish()
 
-	timeout := time.Second * 3
-	if ok, _ := f.TryLockContext(ctx, timeout); !ok {
+	if ok, _ := f.TryLockContext(ctx, retryDelay); !ok {
 		return false
 	}
 
@@ -220,6 +232,10 @@ func (e *Executor) acquireFileLock() bool {
 }
 
 func (e *Executor) releaseFileLock() {
+	if e.cfg.Run.AllowParallelRunners {
+		return
+	}
+
 	if err := e.flock.Unlock(); err != nil {
 		e.debugf("Failed to unlock on file: %s", err)
 	}

pkg/commands/linters.go

@@ -11,13 +11,13 @@ import (
 )
 
 func (e *Executor) initLinters() {
-	lintersCmd := &cobra.Command{
+	e.lintersCmd = &cobra.Command{
 		Use:   "linters",
 		Short: "List current linters configuration",
 		Run:   e.executeLinters,
 	}
-	e.rootCmd.AddCommand(lintersCmd)
-	e.initRunConfiguration(lintersCmd)
+	e.rootCmd.AddCommand(e.lintersCmd)
+	e.initRunConfiguration(e.lintersCmd)
 }
 
 func (e *Executor) executeLinters(_ *cobra.Command, args []string) {

pkg/commands/root.go

@@ -73,7 +73,6 @@ func (e *Executor) persistentPostRun(_ *cobra.Command, _ []string) {
 		trace.Stop()
 	}
 
-	e.releaseFileLock()
 	os.Exit(e.exitCode)
 }
 

pkg/commands/run.go

@@ -106,6 +106,10 @@ func initFlagSet(fs *pflag.FlagSet, cfg *config.Config, m *lintersdb.Manager, is
 	fs.BoolVar(&rc.UseDefaultSkipDirs, "skip-dirs-use-default", true, getDefaultDirectoryExcludeHelp())
 	fs.StringSliceVar(&rc.SkipFiles, "skip-files", nil, wh("Regexps of files to skip"))
 
+	const allowParallelDesc = "Allow multiple parallel golangci-lint instances running. " +
+		"If false (default) - golangci-lint acquires file lock on start."
+	fs.BoolVar(&rc.AllowParallelRunners, "allow-parallel-runners", false, wh(allowParallelDesc))
+
 	// Linters settings config
 	lsc := &cfg.LintersSettings
 
@@ -251,6 +255,14 @@ func (e *Executor) initRun() {
 		Use:   "run",
 		Short: welcomeMessage,
 		Run:   e.executeRun,
+		PreRun: func(_ *cobra.Command, _ []string) {
+			if ok := e.acquireFileLock(); !ok {
+				e.log.Fatalf("Parallel golangci-lint is running")
+			}
+		},
+		PostRun: func(_ *cobra.Command, _ []string) {
+			e.releaseFileLock()
+		},
 	}
 	e.rootCmd.AddCommand(e.runCmd)
 

pkg/config/config.go

@@ -150,6 +150,8 @@ type Run struct {
 	SkipFiles          []string `mapstructure:"skip-files"`
 	SkipDirs           []string `mapstructure:"skip-dirs"`
 	UseDefaultSkipDirs bool     `mapstructure:"skip-dirs-use-default"`
+
+	AllowParallelRunners bool `mapstructure:"allow-parallel-runners"`
 }
 
 type LintersSettings struct {

pkg/lint/lintersdb/enabled_set.go

@@ -1,6 +1,7 @@
 package lintersdb
 
 import (
+	"os"
 	"sort"
 	"strings"
 
@@ -29,6 +30,7 @@ func NewEnabledSet(m *Manager, v *Validator, log logutils.Log, cfg *config.Confi
 }
 
 func (es EnabledSet) build(lcfg *config.Linters, enabledByDefaultLinters []*linter.Config) map[string]*linter.Config {
+	es.debugf("Linters config: %#v", lcfg)
 	resultLintersSet := map[string]*linter.Config{}
 	switch {
 	case len(lcfg.Presets) != 0:
@@ -82,7 +84,11 @@ func (es EnabledSet) GetEnabledLintersMap() (map[string]*linter.Config, error) {
 		return nil, err
 	}
 
-	return es.build(&es.cfg.Linters, es.m.GetAllEnabledByDefaultLinters()), nil
+	enabledLinters := es.build(&es.cfg.Linters, es.m.GetAllEnabledByDefaultLinters())
+	if os.Getenv("GL_TEST_RUN") == "1" {
+		es.verbosePrintLintersStatus(enabledLinters)
+	}
+	return enabledLinters, nil
 }
 
 // GetOptimizedLinters returns enabled linters after optimization (merging) of multiple linters

test/enabled_linters_test.go

@@ -170,9 +170,12 @@ func TestEnabledLinters(t *testing.T) {
 		},
 	}
 
+	runner := testshared.NewLintRunner(t)
 	for _, c := range cases {
 		c := c
 		t.Run(c.name, func(t *testing.T) {
+			t.Parallel()
+
 			runArgs := []string{"-v"}
 			if !c.noImplicitFast {
 				runArgs = append(runArgs, "--fast")
@@ -180,8 +183,7 @@ func TestEnabledLinters(t *testing.T) {
 			if c.args != "" {
 				runArgs = append(runArgs, strings.Split(c.args, " ")...)
 			}
-			runArgs = append(runArgs, minimalPkg)
-			r := testshared.NewLintRunner(t).RunWithYamlConfig(c.cfg, runArgs...)
+			r := runner.RunCommandWithYamlConfig(c.cfg, "linters", runArgs...)
 			sort.StringSlice(c.el).Sort()
 
 			expectedLine := fmt.Sprintf("Active %d linters: [%s]", len(c.el), strings.Join(c.el, " "))

test/linters_test.go

@@ -19,7 +19,7 @@ func runGoErrchk(c *exec.Cmd, files []string, t *testing.T) {
 	output, err := c.CombinedOutput()
 	assert.Error(t, err)
 	_, ok := err.(*exec.ExitError)
-	assert.True(t, ok)
+	assert.True(t, ok, err)
 
 	// TODO: uncomment after deprecating go1.11
 	// assert.Equal(t, exitcodes.IssuesFound, exitErr.ExitCode())
@@ -48,9 +48,9 @@ func testSourcesFromDir(t *testing.T, dir string) {
 
 	for _, s := range sources {
 		s := s
-		t.Run(filepath.Base(s), func(t *testing.T) {
-			t.Parallel()
-			testOneSource(t, s)
+		t.Run(filepath.Base(s), func(subTest *testing.T) {
+			subTest.Parallel()
+			testOneSource(subTest, s)
 		})
 	}
 }
@@ -101,6 +101,7 @@ func saveConfig(t *testing.T, cfg map[string]interface{}) (cfgPath string, finis
 func testOneSource(t *testing.T, sourcePath string) {
 	args := []string{
 		"run",
+		"--allow-parallel-runners",
 		"--disable-all",
 		"--print-issued-lines=false",
 		"--print-linter-name=false",

test/run_test.go

@@ -99,9 +99,10 @@ func TestCgoOk(t *testing.T) {
 }
 
 func TestCgoWithIssues(t *testing.T) {
-	testshared.NewLintRunner(t).Run("--no-config", "--disable-all", "-Egovet", getTestDataDir("cgo_with_issues")).
+	r := testshared.NewLintRunner(t)
+	r.Run("--no-config", "--disable-all", "-Egovet", getTestDataDir("cgo_with_issues")).
 		ExpectHasIssue("Printf format %t has arg cs of wrong type")
-	testshared.NewLintRunner(t).Run("--no-config", "--disable-all", "-Estaticcheck", getTestDataDir("cgo_with_issues")).
+	r.Run("--no-config", "--disable-all", "-Estaticcheck", getTestDataDir("cgo_with_issues")).
 		ExpectHasIssue("SA5009: Printf format %t has arg #1 of wrong type")
 }
 
@@ -138,31 +139,32 @@ func TestLineDirectiveProcessedFilesFullLoading(t *testing.T) {
 }
 
 func TestLintFilesWithLineDirective(t *testing.T) {
-	testshared.NewLintRunner(t).Run("-Edupl", "--disable-all", "--config=testdata/linedirective/dupl.yml", getTestDataDir("linedirective")).
+	r := testshared.NewLintRunner(t)
+	r.Run("-Edupl", "--disable-all", "--config=testdata/linedirective/dupl.yml", getTestDataDir("linedirective")).
 		ExpectHasIssue("21-23 lines are duplicate of `testdata/linedirective/hello.go:25-27` (dupl)")
-	testshared.NewLintRunner(t).Run("-Egofmt", "--disable-all", "--no-config", getTestDataDir("linedirective")).
+	r.Run("-Egofmt", "--disable-all", "--no-config", getTestDataDir("linedirective")).
 		ExpectHasIssue("File is not `gofmt`-ed with `-s` (gofmt)")
-	testshared.NewLintRunner(t).Run("-Egoimports", "--disable-all", "--no-config", getTestDataDir("linedirective")).
+	r.Run("-Egoimports", "--disable-all", "--no-config", getTestDataDir("linedirective")).
 		ExpectHasIssue("File is not `goimports`-ed (goimports)")
-	testshared.NewLintRunner(t).
+	r.
 		Run("-Egomodguard", "--disable-all", "--config=testdata/linedirective/gomodguard.yml", getTestDataDir("linedirective")).
 		ExpectHasIssue("import of package `github.com/ryancurrah/gomodguard` is blocked because the module is not " +
 			"in the allowed modules list. (gomodguard)")
-	testshared.NewLintRunner(t).Run("-Eineffassign", "--disable-all", "--no-config", getTestDataDir("linedirective")).
+	r.Run("-Eineffassign", "--disable-all", "--no-config", getTestDataDir("linedirective")).
 		ExpectHasIssue("ineffectual assignment to `x` (ineffassign)")
-	testshared.NewLintRunner(t).Run("-Elll", "--disable-all", "--config=testdata/linedirective/lll.yml", getTestDataDir("linedirective")).
+	r.Run("-Elll", "--disable-all", "--config=testdata/linedirective/lll.yml", getTestDataDir("linedirective")).
 		ExpectHasIssue("line is 57 characters (lll)")
-	testshared.NewLintRunner(t).Run("-Emisspell", "--disable-all", "--no-config", getTestDataDir("linedirective")).
+	r.Run("-Emisspell", "--disable-all", "--no-config", getTestDataDir("linedirective")).
 		ExpectHasIssue("is a misspelling of `language` (misspell)")
-	testshared.NewLintRunner(t).Run("-Ewsl", "--disable-all", "--no-config", getTestDataDir("linedirective")).
+	r.Run("-Ewsl", "--disable-all", "--no-config", getTestDataDir("linedirective")).
 		ExpectHasIssue("block should not start with a whitespace (wsl)")
 }
 
 func TestSkippedDirsNoMatchArg(t *testing.T) {
 	dir := getTestDataDir("skipdirs", "skip_me", "nested")
-	r := testshared.NewLintRunner(t).Run("--print-issued-lines=false", "--no-config", "--skip-dirs", dir, "-Egolint", dir)
+	res := testshared.NewLintRunner(t).Run("--print-issued-lines=false", "--no-config", "--skip-dirs", dir, "-Egolint", dir)
 
-	r.ExpectExitCode(exitcodes.IssuesFound).
+	res.ExpectExitCode(exitcodes.IssuesFound).
 		ExpectOutputEq("testdata/skipdirs/skip_me/nested/with_issue.go:8:9: `if` block ends with " +
 			"a `return` statement, so drop this `else` and outdent its block (golint)\n")
 }

test/testshared/testshared.go

@@ -5,7 +5,9 @@ import (
 	"os"
 	"os/exec"
 	"strings"
+	"sync"
 	"syscall"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 
@@ -17,7 +19,7 @@ type LintRunner struct {
 	t           assert.TestingT
 	log         logutils.Log
 	env         []string
-	installed bool
+	installOnce sync.Once
 }
 
 func NewLintRunner(t assert.TestingT, environ ...string) *LintRunner {
@@ -31,17 +33,14 @@ func NewLintRunner(t assert.TestingT, environ ...string) *LintRunner {
 }
 
 func (r *LintRunner) Install() {
-	if r.installed {
-		return
-	}
-
+	r.installOnce.Do(func() {
 		if os.Getenv("GOLANGCI_LINT_INSTALLED") == "true" {
 			return
 		}
 
 		cmd := exec.Command("make", "-C", "..", "build")
 		assert.NoError(r.t, cmd.Run(), "Can't go install golangci-lint")
-	r.installed = true
+	})
 }
 
 type RunResult struct {
@@ -82,10 +81,18 @@ func (r *RunResult) ExpectHasIssue(issueText string) *RunResult {
 }
 
 func (r *LintRunner) Run(args ...string) *RunResult {
+	newArgs := append([]string{"--allow-parallel-runners"}, args...)
+	return r.RunCommand("run", newArgs...)
+}
+
+func (r *LintRunner) RunCommand(command string, args ...string) *RunResult {
 	r.Install()
 
-	runArgs := append([]string{"run"}, args...)
-	r.log.Infof("../golangci-lint %s", strings.Join(runArgs, " "))
+	runArgs := append([]string{command}, args...)
+	defer func(startedAt time.Time) {
+		r.log.Infof("ran [../golangci-lint %s] in %s", strings.Join(runArgs, " "), time.Since(startedAt))
+	}(time.Now())
+
 	cmd := exec.Command("../golangci-lint", runArgs...)
 	cmd.Env = append(os.Environ(), r.env...)
 	out, err := cmd.CombinedOutput()
@@ -114,6 +121,11 @@ func (r *LintRunner) Run(args ...string) *RunResult {
 }
 
 func (r *LintRunner) RunWithYamlConfig(cfg string, args ...string) *RunResult {
+	newArgs := append([]string{"--allow-parallel-runners"}, args...)
+	return r.RunCommandWithYamlConfig(cfg, "run", newArgs...)
+}
+
+func (r *LintRunner) RunCommandWithYamlConfig(cfg, command string, args ...string) *RunResult {
 	f, err := ioutil.TempFile("", "golangci_lint_test")
 	assert.NoError(r.t, err)
 	f.Close()
@@ -133,5 +145,5 @@ func (r *LintRunner) RunWithYamlConfig(cfg string, args ...string) *RunResult {
 	assert.NoError(r.t, err)
 
 	pargs := append([]string{"-c", cfgPath}, args...)
-	return r.Run(pargs...)
+	return r.RunCommand(command, pargs...)
 }