From e47ee39c7f86b0a495942af5d876ee241f107598 Mon Sep 17 00:00:00 2001 From: Denis Isaev Date: Tue, 5 May 2020 20:17:59 +0300 Subject: [PATCH] speed up CI and golangci-lint 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 | 13 ++++----- .golangci.example.yml | 4 +++ Makefile | 4 +-- README.md | 5 ++++ internal/cache/cache_test.go | 6 ++++ pkg/commands/config.go | 22 +++++++++++---- pkg/commands/executor.go | 46 +++++++++++++++++++++---------- pkg/commands/linters.go | 6 ++-- pkg/commands/root.go | 1 - pkg/commands/run.go | 12 ++++++++ pkg/config/config.go | 2 ++ pkg/lint/lintersdb/enabled_set.go | 8 +++++- test/enabled_linters_test.go | 6 ++-- test/linters_test.go | 9 +++--- test/run_test.go | 26 +++++++++-------- test/testshared/testshared.go | 46 +++++++++++++++++++------------ 16 files changed, 145 insertions(+), 71 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 7a998a39f383..0cae0ef88786 100644 --- a/.github/workflows/pr.yml +++ b/.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 diff --git a/.golangci.example.yml b/.golangci.example.yml index 93debadbb1d3..651a5afbdd8d 100644 --- a/.golangci.example.yml +++ b/.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: diff --git a/Makefile b/Makefile index bfe177dc2a41..f628c89cc8f9 100644 --- a/Makefile +++ b/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 diff --git a/README.md b/README.md index 22053bbbc8ce..4c82541c845b 100644 --- a/README.md +++ b/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: diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index bc2b90d54a28..514adc44aa10 100644 --- a/internal/cache/cache_test.go +++ b/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) diff --git a/pkg/commands/config.go b/pkg/commands/config.go index 807f1ca492aa..4b63e2e5239f 100644 --- a/pkg/commands/config.go +++ b/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) } diff --git a/pkg/commands/executor.go b/pkg/commands/executor.go index ffef5b0a5bdf..9581acab21f2 100644 --- a/pkg/commands/executor.go +++ b/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" @@ -31,8 +31,9 @@ import ( ) type Executor struct { - rootCmd *cobra.Command - runCmd *cobra.Command + 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) } diff --git a/pkg/commands/linters.go b/pkg/commands/linters.go index ab73de67c8f6..873dab817709 100644 --- a/pkg/commands/linters.go +++ b/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) { diff --git a/pkg/commands/root.go b/pkg/commands/root.go index c8548cd73400..f90df9901ff8 100644 --- a/pkg/commands/root.go +++ b/pkg/commands/root.go @@ -73,7 +73,6 @@ func (e *Executor) persistentPostRun(_ *cobra.Command, _ []string) { trace.Stop() } - e.releaseFileLock() os.Exit(e.exitCode) } diff --git a/pkg/commands/run.go b/pkg/commands/run.go index ec3f93ddd4cd..e4887d587a50 100644 --- a/pkg/commands/run.go +++ b/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) diff --git a/pkg/config/config.go b/pkg/config/config.go index 2315f308c1c0..021af5684a98 100644 --- a/pkg/config/config.go +++ b/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 { diff --git a/pkg/lint/lintersdb/enabled_set.go b/pkg/lint/lintersdb/enabled_set.go index 6450f1d143a7..eced95f6552d 100644 --- a/pkg/lint/lintersdb/enabled_set.go +++ b/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 diff --git a/test/enabled_linters_test.go b/test/enabled_linters_test.go index 0ade56b04dc3..702df9be7f1d 100644 --- a/test/enabled_linters_test.go +++ b/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, " ")) diff --git a/test/linters_test.go b/test/linters_test.go index 1f01814af25e..b53fc14c82bc 100644 --- a/test/linters_test.go +++ b/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", diff --git a/test/run_test.go b/test/run_test.go index 249100b5b868..e20b54fa1aca 100644 --- a/test/run_test.go +++ b/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") } diff --git a/test/testshared/testshared.go b/test/testshared/testshared.go index 0330e77a79de..8fd6185038ab 100644 --- a/test/testshared/testshared.go +++ b/test/testshared/testshared.go @@ -5,7 +5,9 @@ import ( "os" "os/exec" "strings" + "sync" "syscall" + "time" "github.com/stretchr/testify/assert" @@ -14,10 +16,10 @@ import ( ) type LintRunner struct { - t assert.TestingT - log logutils.Log - env []string - installed bool + t assert.TestingT + log logutils.Log + env []string + 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 - } - - if os.Getenv("GOLANGCI_LINT_INSTALLED") == "true" { - 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 + cmd := exec.Command("make", "-C", "..", "build") + assert.NoError(r.t, cmd.Run(), "Can't go install golangci-lint") + }) } 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...) }