Fix #78: log all warnings

1. Log all warnings, don't hide none of them
2. Write fatal messages (stop analysis) with error log level
3. Remove ugly timestamp counter from logrus output
4. Print nested module prefix in log
5. Make logger abstraction: no global logging anymore
6. Refactor config reading to config.FileReader struct to avoid passing
logger into every function
7. Replace exit codes hardcoding with constants in exitcodes package
8. Fail test if any warning was logged
9. Fix calculation of relative path if we analyze parent dir ../
10. Move Runner initialization from Executor to NewRunner func
11. Log every AST parsing error
12. Properly print used config file path in verbose mode
13. Print package files if only 1 package is analyzedin verbose mode,
  print not compiling packages in verbose mode
14. Forbid usage of github.com/sirupsen/logrus by DepGuard linter
15. Add default ignore pattern to folint: "comment on exported const"

Author: jirfag

.gitignore

@@ -2,3 +2,4 @@
 /*.pprof
 /dist/
 /.idea/
+/test/path

.golangci.yml

@@ -12,6 +12,12 @@ linters-settings:
   goconst:
     min-len: 2
     min-occurrences: 2
+  depguard:
+    list-type: blacklist
+    packages:
+      # logging is allowed only by logutils.Log, logrus
+      # is allowed to use only in logutils package
+      - github.com/sirupsen/logrus
 
 linters:
   enable-all: true

Makefile

@@ -1,9 +1,9 @@
 test:
-	go install ./cmd/... # needed for govet and golint
-	golangci-lint run -v
-	golangci-lint run --fast --no-config -v
-	golangci-lint run --no-config -v
-	go test -v ./...
+	go install ./cmd/... # needed for govet and golint if go < 1.10
+	GL_TEST_RUN=1 golangci-lint run -v
+	GL_TEST_RUN=1 golangci-lint run --fast --no-config -v
+	GL_TEST_RUN=1 golangci-lint run --no-config -v
+	GL_TEST_RUN=1 go test -v ./...
 
 assets:
 	svg-term --cast=183662 --out docs/demo.svg --window --width 110 --height 30 --from 2000 --to 20000 --profile Dracula --term iterm2

README.md

@@ -255,7 +255,7 @@ Flags:
                                       - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*printf?|os\.(Un)?Setenv). is not checked
                                     
                                       # golint: Annoying issue about not having a comment. The rare codebase has such comments
-                                      - (comment on exported (method|function|type)|should have( a package)? comment|comment should be of the form)
+                                      - (comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form)
                                     
                                       # golint: False positive when tests are defined in package 'test'
                                       - func name will be used as test\.Test.* by other packages, and that stutters; consider calling this
@@ -329,6 +329,12 @@ linters-settings:
   goconst:
     min-len: 2
     min-occurrences: 2
+  depguard:
+    list-type: blacklist
+    packages:
+      # logging is allowed only by logutils.Log, logrus
+      # is allowed to use only in logutils package
+      - github.com/sirupsen/logrus
 
 linters:
   enable-all: true

pkg/commands/executor.go

@@ -2,7 +2,7 @@ package commands
 
 import (
 	"github.com/golangci/golangci-lint/pkg/config"
-	"github.com/sirupsen/logrus"
+	"github.com/golangci/golangci-lint/pkg/logutils"
 	"github.com/spf13/cobra"
 )
 
@@ -14,6 +14,8 @@ type Executor struct {
 	exitCode int
 
 	version, commit, date string
+
+	log logutils.Log
 }
 
 func NewExecutor(version, commit, date string) *Executor {
@@ -22,10 +24,9 @@ func NewExecutor(version, commit, date string) *Executor {
 		version: version,
 		commit:  commit,
 		date:    date,
+		log:     logutils.NewStderrLog(""),
 	}
 
-	logrus.SetLevel(logrus.WarnLevel)
-
 	e.initRoot()
 	e.initRun()
 	e.initLinters()

pkg/commands/root.go

@@ -2,28 +2,17 @@ package commands
 
 import (
 	"fmt"
-	"log"
 	"os"
 	"runtime"
 	"runtime/pprof"
 
 	"github.com/golangci/golangci-lint/pkg/config"
 	"github.com/golangci/golangci-lint/pkg/logutils"
 	"github.com/golangci/golangci-lint/pkg/printers"
-	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
 
-func setupLog(isVerbose bool) {
-	log.SetFlags(0) // don't print time
-	if logutils.IsDebugEnabled() {
-		logrus.SetLevel(logrus.DebugLevel)
-	} else if isVerbose {
-		logrus.SetLevel(logrus.InfoLevel)
-	}
-}
-
 func (e *Executor) persistentPreRun(cmd *cobra.Command, args []string) {
 	if e.cfg.Run.PrintVersion {
 		fmt.Fprintf(printers.StdOut, "golangci-lint has version %s built from %s on %s\n", e.version, e.commit, e.date)
@@ -32,15 +21,15 @@ func (e *Executor) persistentPreRun(cmd *cobra.Command, args []string) {
 
 	runtime.GOMAXPROCS(e.cfg.Run.Concurrency)
 
-	setupLog(e.cfg.Run.IsVerbose)
+	logutils.SetupVerboseLog(e.log, e.cfg.Run.IsVerbose)
 
 	if e.cfg.Run.CPUProfilePath != "" {
 		f, err := os.Create(e.cfg.Run.CPUProfilePath)
 		if err != nil {
-			logrus.Fatal(err)
+			e.log.Fatalf("Can't create file %s: %s", e.cfg.Run.CPUProfilePath, err)
 		}
 		if err := pprof.StartCPUProfile(f); err != nil {
-			logrus.Fatal(err)
+			e.log.Fatalf("Can't start CPU profiling: %s", err)
 		}
 	}
 }
@@ -52,11 +41,11 @@ func (e *Executor) persistentPostRun(cmd *cobra.Command, args []string) {
 	if e.cfg.Run.MemProfilePath != "" {
 		f, err := os.Create(e.cfg.Run.MemProfilePath)
 		if err != nil {
-			logrus.Fatal(err)
+			e.log.Fatalf("Can't create file %s: %s", e.cfg.Run.MemProfilePath, err)
 		}
 		runtime.GC() // get up-to-date statistics
 		if err := pprof.WriteHeapProfile(f); err != nil {
-			logrus.Fatal("could not write memory profile: ", err)
+			e.log.Fatalf("Can't write heap profile: %s", err)
 		}
 	}
 
@@ -78,7 +67,7 @@ func (e *Executor) initRoot() {
 		Long:  `Smart, fast linters runner. Run it in cloud for every GitHub pull request on https://golangci.com`,
 		Run: func(cmd *cobra.Command, args []string) {
 			if err := cmd.Help(); err != nil {
-				logrus.Fatal(err)
+				e.log.Fatalf("Can't run help: %s", err)
 			}
 		},
 		PersistentPreRun:  e.persistentPreRun,
@@ -89,6 +78,10 @@ func (e *Executor) initRoot() {
 	e.rootCmd = rootCmd
 }
 
+func (e *Executor) needVersionOption() bool {
+	return e.date != ""
+}
+
 func initRootFlagSet(fs *pflag.FlagSet, cfg *config.Config, needVersionOption bool) {
 	fs.BoolVarP(&cfg.Run.IsVerbose, "verbose", "v", false, wh("verbose output"))
 	fs.StringVar(&cfg.Run.CPUProfilePath, "cpu-profile-path", "", wh("Path to CPU profile output file"))

pkg/commands/run.go

@@ -12,22 +12,16 @@ import (
 
 	"github.com/fatih/color"
 	"github.com/golangci/golangci-lint/pkg/config"
+	"github.com/golangci/golangci-lint/pkg/exitcodes"
 	"github.com/golangci/golangci-lint/pkg/lint"
 	"github.com/golangci/golangci-lint/pkg/lint/lintersdb"
 	"github.com/golangci/golangci-lint/pkg/logutils"
 	"github.com/golangci/golangci-lint/pkg/printers"
 	"github.com/golangci/golangci-lint/pkg/result"
-	"github.com/golangci/golangci-lint/pkg/result/processors"
-	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
 
-const (
-	exitCodeIfFailure = 3
-	exitCodeIfTimeout = 4
-)
-
 func getDefaultExcludeHelp() string {
 	parts := []string{"Use or not use default excludes:"}
 	for _, ep := range config.DefaultExcludePatterns {
@@ -64,7 +58,7 @@ func initFlagSet(fs *pflag.FlagSet, cfg *config.Config) {
 	// Run config
 	rc := &cfg.Run
 	fs.IntVar(&rc.ExitCodeIfIssuesFound, "issues-exit-code",
-		1, wh("Exit code when issues were found"))
+		exitcodes.IssuesFound, wh("Exit code when issues were found"))
 	fs.StringSliceVar(&rc.BuildTags, "build-tags", nil, wh("Build tags"))
 	fs.DurationVar(&rc.Deadline, "deadline", time.Minute, wh("Deadline for total work"))
 	fs.BoolVar(&rc.AnalyzeTests, "tests", true, wh("Analyze tests (*_test.go)"))
@@ -165,65 +159,76 @@ func (e *Executor) initRun() {
 	// init e.cfg by values from config: flags parse will see these values
 	// like the default ones. It will overwrite them only if the same option
 	// is found in command-line: it's ok, command-line has higher priority.
-	e.parseConfig()
 
-	// Slice options must be explicitly set for properly merging.
+	r := config.NewFileReader(e.cfg, e.log.Child("config_reader"), func(fs *pflag.FlagSet, cfg *config.Config) {
+		// Don't do `fs.AddFlagSet(cmd.Flags())` because it shares flags representations:
+		// `changed` variable inside string slice vars will be shared.
+		// Use another config variable here, not e.cfg, to not
+		// affect main parsing by this parsing of only config option.
+		initFlagSet(fs, cfg)
+
+		// Parse max options, even force version option: don't want
+		// to get access to Executor here: it's error-prone to use
+		// cfg vs e.cfg.
+		initRootFlagSet(fs, cfg, true)
+	})
+	if err := r.Read(); err != nil {
+		e.log.Fatalf("Can't read config: %s", err)
+	}
+
+	// Slice options must be explicitly set for proper merging of config and command-line options.
 	fixSlicesFlags(fs)
 }
 
+func fixSlicesFlags(fs *pflag.FlagSet) {
+	// It's a dirty hack to set flag.Changed to true for every string slice flag.
+	// It's necessary to merge config and command-line slices: otherwise command-line
+	// flags will always overwrite ones from the config.
+	fs.VisitAll(func(f *pflag.Flag) {
+		if f.Value.Type() != "stringSlice" {
+			return
+		}
+
+		s, err := fs.GetStringSlice(f.Name)
+		if err != nil {
+			return
+		}
+
+		if s == nil { // assume that every string slice flag has nil as the default
+			return
+		}
+
+		// calling Set sets Changed to true: next Set calls will append, not overwrite
+		_ = f.Value.Set(strings.Join(s, ","))
+	})
+}
+
 func (e *Executor) runAnalysis(ctx context.Context, args []string) (<-chan result.Issue, error) {
 	e.cfg.Run.Args = args
 
-	linters, err := lintersdb.GetEnabledLinters(e.cfg)
+	linters, err := lintersdb.GetEnabledLinters(e.cfg, e.log.Child("lintersdb"))
 	if err != nil {
 		return nil, err
 	}
 
-	lintCtx, err := lint.LoadContext(ctx, linters, e.cfg)
+	lintCtx, err := lint.LoadContext(ctx, linters, e.cfg, e.log.Child("load"))
 	if err != nil {
 		return nil, err
 	}
 
-	excludePatterns := e.cfg.Issues.ExcludePatterns
-	if e.cfg.Issues.UseDefaultExcludes {
-		excludePatterns = append(excludePatterns, config.GetDefaultExcludePatternsStrings()...)
-	}
-	var excludeTotalPattern string
-	if len(excludePatterns) != 0 {
-		excludeTotalPattern = fmt.Sprintf("(%s)", strings.Join(excludePatterns, "|"))
-	}
-
-	skipFilesProcessor, err := processors.NewSkipFiles(e.cfg.Run.SkipFiles)
+	runner, err := lint.NewRunner(lintCtx.ASTCache, e.cfg, e.log.Child("runner"))
 	if err != nil {
 		return nil, err
 	}
 
-	runner := lint.SimpleRunner{
-		Processors: []processors.Processor{
-			processors.NewPathPrettifier(), // must be before diff, nolint and exclude autogenerated processor at least
-			processors.NewCgo(),
-			skipFilesProcessor,
-
-			processors.NewAutogeneratedExclude(lintCtx.ASTCache),
-			processors.NewExclude(excludeTotalPattern),
-			processors.NewNolint(lintCtx.ASTCache),
-
-			processors.NewUniqByLine(),
-			processors.NewDiff(e.cfg.Issues.Diff, e.cfg.Issues.DiffFromRevision, e.cfg.Issues.DiffPatchFilePath),
-			processors.NewMaxPerFileFromLinter(),
-			processors.NewMaxSameIssues(e.cfg.Issues.MaxSameIssues),
-			processors.NewMaxFromLinter(e.cfg.Issues.MaxIssuesPerLinter),
-		},
-	}
-
 	return runner.Run(ctx, linters, lintCtx), nil
 }
 
-func setOutputToDevNull() (savedStdout, savedStderr *os.File) {
+func (e *Executor) setOutputToDevNull() (savedStdout, savedStderr *os.File) {
 	savedStdout, savedStderr = os.Stdout, os.Stderr
 	devNull, err := os.Open(os.DevNull)
 	if err != nil {
-		logrus.Warnf("can't open null device %q: %s", os.DevNull, err)
+		e.log.Warnf("Can't open null device %q: %s", os.DevNull, err)
 		return
 	}
 
@@ -235,7 +240,7 @@ func (e *Executor) runAndPrint(ctx context.Context, args []string) error {
 	if !logutils.HaveDebugTag("linters_output") {
 		// Don't allow linters and loader to print anything
 		log.SetOutput(ioutil.Discard)
-		savedStdout, savedStderr := setOutputToDevNull()
+		savedStdout, savedStderr := e.setOutputToDevNull()
 		defer func() {
 			os.Stdout, os.Stderr = savedStdout, savedStderr
 		}()
@@ -253,9 +258,11 @@ func (e *Executor) runAndPrint(ctx context.Context, args []string) error {
 		p = printers.NewJSON()
 	case config.OutFormatColoredLineNumber, config.OutFormatLineNumber:
 		p = printers.NewText(e.cfg.Output.PrintIssuedLine,
-			format == config.OutFormatColoredLineNumber, e.cfg.Output.PrintLinterName)
+			format == config.OutFormatColoredLineNumber, e.cfg.Output.PrintLinterName,
+			e.log.Child("text_printer"))
 	case config.OutFormatTab:
-		p = printers.NewTab(e.cfg.Output.PrintLinterName)
+		p = printers.NewTab(e.cfg.Output.PrintLinterName,
+			e.log.Child("tab_printer"))
 	case config.OutFormatCheckstyle:
 		p = printers.NewCheckstyle()
 	default:
@@ -288,22 +295,22 @@ func (e *Executor) executeRun(cmd *cobra.Command, args []string) {
 	defer cancel()
 
 	if needTrackResources {
-		go watchResources(ctx, trackResourcesEndCh)
+		go watchResources(ctx, trackResourcesEndCh, e.log)
 	}
 
 	if err := e.runAndPrint(ctx, args); err != nil {
-		logrus.Warnf("running error: %s", err)
-		if e.exitCode == 0 {
-			e.exitCode = exitCodeIfFailure
+		e.log.Errorf("Running error: %s", err)
+		if e.exitCode == exitcodes.Success {
+			e.exitCode = exitcodes.Failure
 		}
 	}
 
-	if e.exitCode == 0 && ctx.Err() != nil {
-		e.exitCode = exitCodeIfTimeout
+	if e.exitCode == exitcodes.Success && ctx.Err() != nil {
+		e.exitCode = exitcodes.Timeout
 	}
 }
 
-func watchResources(ctx context.Context, done chan struct{}) {
+func watchResources(ctx context.Context, done chan struct{}, log logutils.Log) {
 	startedAt := time.Now()
 
 	rssValues := []uint64{}
@@ -339,8 +346,8 @@ func watchResources(ctx context.Context, done chan struct{}) {
 
 	const MB = 1024 * 1024
 	maxMB := float64(max) / MB
-	logrus.Infof("Memory: %d samples, avg is %.1fMB, max is %.1fMB",
+	log.Infof("Memory: %d samples, avg is %.1fMB, max is %.1fMB",
 		len(rssValues), float64(avg)/MB, maxMB)
-	logrus.Infof("Execution took %s", time.Since(startedAt))
+	log.Infof("Execution took %s", time.Since(startedAt))
 	close(done)
 }