Merge pull request #739 from mattmoor/support-linter-warnings

Enable linters to warn (via callback) instead of just failing.

docs/md/melange_build.md

@@ -42,6 +42,7 @@ melange build [flags]
       --dependency-log string       log dependencies to a specified file
       --empty-workspace             whether the build workspace should be empty
       --env-file string             file to use for preloaded environment variables
+      --fail-on-lint-warning        turns linter warnings into failures
       --generate-index              whether to generate APKINDEX.tar.gz (default true)
       --guest-dir string            directory used for the build environment guest
   -h, --help                        help for build

pkg/build/build.go

@@ -91,6 +91,7 @@ type Build struct {
 	Debug              bool
 	DebugRunner        bool
 	LogPolicy          []string
+	FailOnLintWarning  bool
 
 	EnabledBuildOptions []string
 }
@@ -252,6 +253,14 @@ func WithConfig(configFile string) Option {
 	}
 }
 
+// WithFailOnLintWarning sets whether or not to fail on linter warnings.
+func WithFailOnLintWarning(fail bool) Option {
+	return func(b *Build) error {
+		b.FailOnLintWarning = fail
+		return nil
+	}
+}
+
 // WithBuildDate sets the timestamps for the build context.
 // The string is parsed according to RFC3339.
 // An empty string is a special case and will default to
@@ -1133,9 +1142,18 @@ func (b *Build) BuildPackage(ctx context.Context) error {
 		lctx := linter.NewLinterContext(lt.pkgName, fsys, &b.Configuration, &lt.checks)
 		linters := lt.checks.GetLinters()
 
-		err = lctx.LintPackageFs(fsys, linters)
+		var innerErr error
+		err = lctx.LintPackageFs(fsys, func(err error) {
+			if b.FailOnLintWarning {
+				innerErr = err
+			} else {
+				b.Logger.Warnf("WARNING: %v", err)
+			}
+		}, linters)
 		if err != nil {
 			return fmt.Errorf("package linter error: %w", err)
+		} else if innerErr != nil {
+			return fmt.Errorf("package linter warning: %w", err)
 		}
 	}
 

pkg/cli/build.go

@@ -60,6 +60,7 @@ func Build() *cobra.Command {
 	var debug bool
 	var debugRunner bool
 	var runner string
+	var failOnLintWarning bool
 
 	cmd := &cobra.Command{
 		Use:     "build",
@@ -97,6 +98,7 @@ func Build() *cobra.Command {
 				build.WithDebugRunner(debugRunner),
 				build.WithLogPolicy(logPolicy),
 				build.WithRunner(runner),
+				build.WithFailOnLintWarning(failOnLintWarning),
 			}
 
 			if len(args) > 0 {
@@ -144,6 +146,7 @@ func Build() *cobra.Command {
 	cmd.Flags().BoolVar(&createBuildLog, "create-build-log", false, "creates a package.log file containing a list of packages that were built by the command")
 	cmd.Flags().BoolVar(&debug, "debug", false, "enables debug logging of build pipelines")
 	cmd.Flags().BoolVar(&debugRunner, "debug-runner", false, "when enabled, the builder pod will persist after the build succeeds or fails")
+	cmd.Flags().BoolVar(&failOnLintWarning, "fail-on-lint-warning", false, "turns linter warnings into failures")
 
 	return cmd
 }

pkg/linter/linter.go

@@ -40,31 +40,73 @@ func NewLinterContext(name string, fsys fs.FS, cfg *config.Configuration, chk *c
 type linterFunc func(lctx LinterContext, path string, d fs.DirEntry) error
 
 type linter struct {
-	LinterFunc linterFunc
-	Explain    string
+	LinterFunc  linterFunc
+	FailOnError bool
+	Explain     string
 }
 
 type postLinterFunc func(lctx LinterContext, fsys fs.FS) error
 
 type postLinter struct {
-	LinterFunc postLinterFunc
-	Explain    string
+	LinterFunc  postLinterFunc
+	FailOnError bool
+	Explain     string
 }
 
 var linterMap = map[string]linter{
-	"dev":        linter{devLinter, "If this package is creating /dev nodes, it should use udev instead; otherwise, remove any files in /dev"},
-	"opt":        linter{optLinter, "This package should be a -compat package"},
-	"setuidgid":  linter{isSetUidOrGidLinter, "Unset the setuid/setgid bit on the relevant files, or remove this linter"},
-	"srv":        linter{srvLinter, "This package should be a -compat package"},
-	"tempdir":    linter{tempDirLinter, "Remove any offending files in temporary dirs in the pipeline"},
-	"usrlocal":   linter{usrLocalLinter, "This package should be a -compat package"},
-	"varempty":   linter{varEmptyLinter, "Remove any offending files in /var/empty in the pipeline"},
-	"worldwrite": linter{worldWriteableLinter, "Change the permissions of any world-writeable files in the package, disable the linter, or make this a -compat package"},
-	"strip":      linter{strippedLinter, "Properly strip all binaries in the pipeline"},
+	"dev": linter{
+		LinterFunc:  devLinter,
+		FailOnError: false,
+		Explain:     "If this package is creating /dev nodes, it should use udev instead; otherwise, remove any files in /dev",
+	},
+	"opt": linter{
+		LinterFunc:  optLinter,
+		FailOnError: false,
+		Explain:     "This package should be a -compat package",
+	},
+	"setuidgid": linter{
+		LinterFunc:  isSetUidOrGidLinter,
+		FailOnError: false,
+		Explain:     "Unset the setuid/setgid bit on the relevant files, or remove this linter",
+	},
+	"srv": linter{
+		LinterFunc:  srvLinter,
+		FailOnError: false,
+		Explain:     "This package should be a -compat package",
+	},
+	"tempdir": linter{
+		LinterFunc:  tempDirLinter,
+		FailOnError: false,
+		Explain:     "Remove any offending files in temporary dirs in the pipeline",
+	},
+	"usrlocal": linter{
+		LinterFunc:  usrLocalLinter,
+		FailOnError: false,
+		Explain:     "This package should be a -compat package",
+	},
+	"varempty": linter{
+		LinterFunc:  varEmptyLinter,
+		FailOnError: false,
+		Explain:     "Remove any offending files in /var/empty in the pipeline",
+	},
+	"worldwrite": linter{
+		LinterFunc:  worldWriteableLinter,
+		FailOnError: false,
+		Explain:     "Change the permissions of any world-writeable files in the package, disable the linter, or make this a -compat package",
+	},
+	"strip": linter{
+		LinterFunc:  strippedLinter,
+		FailOnError: false,
+		Explain:     "Properly strip all binaries in the pipeline",
+	},
 }
 
 var postLinterMap = map[string]postLinter{
-	"empty": postLinter{emptyPostLinter, "Verify that this package is supposed to be empty; if it is, disable this linter; otherwise check the build"},
+	"empty": postLinter{
+		LinterFunc:  emptyPostLinter,
+		FailOnError: false,
+		Explain:     "Verify that this package is supposed to be empty; if it is, disable this linter; otherwise check the build",
+	},
 }
 
 var isDevRegex = regexp.MustCompile("^dev/")
@@ -251,7 +293,7 @@ func emptyPostLinter(_ LinterContext, fsys fs.FS) error {
 	return fmt.Errorf("Package is empty but no-provides is not set")
 }
 
-func (lctx LinterContext) LintPackageFs(fsys fs.FS, linters []string) error {
+func (lctx LinterContext) LintPackageFs(fsys fs.FS, warn func(error), linters []string) error {
 	// If this is a compat package, do nothing.
 	if isCompatPackageRegex.MatchString(lctx.pkgname) {
 		return nil
@@ -278,7 +320,10 @@ func (lctx LinterContext) LintPackageFs(fsys fs.FS, linters []string) error {
 
 			err = linter.LinterFunc(lctx, path, d)
 			if err != nil {
-				return fmt.Errorf("Linter %s failed at path %q: %w; suggest: %s", linterName, path, err, linter.Explain)
+				if linter.FailOnError {
+					return fmt.Errorf("Linter %s failed at path %q: %w; suggest: %s", linterName, path, err, linter.Explain)
+				}
+				warn(err)
 			}
 		}
 
@@ -294,7 +339,10 @@ func (lctx LinterContext) LintPackageFs(fsys fs.FS, linters []string) error {
 		linter := postLinterMap[linterName]
 		err := linter.LinterFunc(lctx, fsys)
 		if err != nil {
-			return fmt.Errorf("Linter %s failed; suggest: %s", linterName, linter.Explain)
+			if linter.FailOnError {
+				return fmt.Errorf("Linter %s failed; suggest: %s", linterName, linter.Explain)
+			}
+			warn(err)
 		}
 	}
 

pkg/linter/linter_test.go

@@ -46,7 +46,11 @@ func Test_emptyLinter(t *testing.T) {
 	assert.Equal(t, linters, []string{"empty"})
 	fsys := os.DirFS(dir)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 }
 
 func Test_usrLocalLinter(t *testing.T) {
@@ -75,7 +79,11 @@ func Test_usrLocalLinter(t *testing.T) {
 	assert.Equal(t, linters, []string{"usrlocal"})
 	fsys := os.DirFS(dir)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 }
 
 func Test_varEmptyLinter(t *testing.T) {
@@ -105,7 +113,11 @@ func Test_varEmptyLinter(t *testing.T) {
 	assert.Equal(t, linters, []string{"varempty"})
 	fsys := os.DirFS(dir)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 }
 
 func Test_devLinter(t *testing.T) {
@@ -135,7 +147,11 @@ func Test_devLinter(t *testing.T) {
 	assert.Equal(t, linters, []string{"dev"})
 	fsys := os.DirFS(dir)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 }
 
 func Test_optLinter(t *testing.T) {
@@ -165,7 +181,11 @@ func Test_optLinter(t *testing.T) {
 	assert.Equal(t, linters, []string{"opt"})
 	fsys := os.DirFS(dir)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 }
 
 func Test_srvLinter(t *testing.T) {
@@ -195,7 +215,11 @@ func Test_srvLinter(t *testing.T) {
 	assert.Equal(t, linters, []string{"srv"})
 	fsys := os.DirFS(dir)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 }
 
 func Test_tempDirLinter(t *testing.T) {
@@ -237,7 +261,11 @@ func Test_tempDirLinter(t *testing.T) {
 	_, err = os.Create(filename)
 	assert.NoError(t, err)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 	os.Remove(filename)
 
 	// Test /var/tmp check
@@ -292,7 +320,11 @@ func Test_setUidGidLinter(t *testing.T) {
 	assert.Equal(t, linters, []string{"setuidgid"})
 	fsys := os.DirFS(dir)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 }
 
 func Test_worldWriteLinter(t *testing.T) {
@@ -321,7 +353,11 @@ func Test_worldWriteLinter(t *testing.T) {
 	assert.Equal(t, linters, []string{"worldwrite"})
 	fsys := os.DirFS(dir)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.NoError(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.False(t, called)
 
 	// Create test file
 	filePath := filepath.Join(usrLocalDirPath, "test.txt")
@@ -333,21 +369,33 @@ func Test_worldWriteLinter(t *testing.T) {
 	assert.NoError(t, err)
 
 	// Linter should not trigger
-	assert.NoError(t, lctx.LintPackageFs(fsys, linters))
+	called = false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.False(t, called)
 
 	// Set writeable bit (but not executable bit)
 	err = os.Chmod(filePath, 0776)
 	assert.NoError(t, err)
 
 	// Linter should trigger
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called = false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 
 	// Set writeable and executable bit
 	err = os.Chmod(filePath, 0777)
 	assert.NoError(t, err)
 
 	// Linter should trigger
-	assert.Error(t, lctx.LintPackageFs(fsys, linters))
+	called = false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.True(t, called)
 }
 
 func Test_disableDefaultLinter(t *testing.T) {
@@ -378,5 +426,9 @@ func Test_disableDefaultLinter(t *testing.T) {
 	linters := cfg.Package.Checks.GetLinters()
 	fsys := os.DirFS(dir)
 	lctx := NewLinterContext(cfg.Package.Name, fsys, &cfg, &cfg.Package.Checks)
-	assert.NoError(t, lctx.LintPackageFs(fsys, linters))
+	called := false
+	assert.NoError(t, lctx.LintPackageFs(fsys, func(err error) {
+		called = true
+	}, linters))
+	assert.False(t, called)
 }