Make puku rewrite build files by default, with a flag to skip rewriti…

…ng (#121)

* Make puku rewrite build files by default, with a flag to skip rewriting

* Fix unused arg, and make outputting behave consistently

* Fix lint

* Fix lint (attmpt 2 -_-)

* Add a function to explain why outputFormattedBuildFile takes a closure

* Switch to passing a parameter object around instead of just the skipRewriting bool

* Bump version

ChangeLog

@@ -1,3 +1,7 @@
+Version 1.11.0
+-------------
+    * Make puku rewrite build files by default, with a flag to skip rewriting
+
 Version 1.10.0
 -------------
     * Adds a `puku version` command to report the version of puku that is installed.

VERSION

@@ -1 +1 @@
-1.10.0
+1.11.0

cmd/puku/BUILD

@@ -22,5 +22,6 @@ go_binary(
         "//version",
         "//watch",
         "//work",
+        "//options",
     ],
 )

cmd/puku/puku.go

@@ -14,6 +14,7 @@ import (
 	"github.com/please-build/puku/licences"
 	"github.com/please-build/puku/logging"
 	"github.com/please-build/puku/migrate"
+	"github.com/please-build/puku/options"
 	"github.com/please-build/puku/please"
 	"github.com/please-build/puku/proxy"
 	"github.com/please-build/puku/sync"
@@ -23,6 +24,8 @@ import (
 )
 
 var opts = struct {
+	options.Options
+
 	Usage     string
 	Verbosity clilogging.Verbosity `short:"v" long:"verbosity" description:"Verbosity of output (error, warning, notice, info, debug)" default:"info"`
 
@@ -76,13 +79,13 @@ var log = logging.GetLogger()
 var funcs = map[string]func(conf *config.Config, plzConf *please.Config, orignalWD string) int{
 	"fmt": func(_ *config.Config, plzConf *please.Config, orignalWD string) int {
 		paths := work.MustExpandPaths(orignalWD, opts.Fmt.Args.Paths)
-		if err := generate.Update(plzConf, paths...); err != nil {
+		if err := generate.Update(plzConf, opts.Options, paths...); err != nil {
 			log.Fatalf("%v", err)
 		}
 		return 0
 	},
 	"sync": func(_ *config.Config, plzConf *please.Config, _ string) int {
-		g := graph.New(plzConf.BuildFileNames())
+		g := graph.New(plzConf.BuildFileNames(), opts.Options)
 		if opts.Sync.Write {
 			if err := sync.Sync(plzConf, g); err != nil {
 				log.Fatalf("%v", err)
@@ -95,20 +98,19 @@ var funcs = map[string]func(conf *config.Config, plzConf *please.Config, orignal
 		return 0
 	},
 	"lint": func(_ *config.Config, plzConf *please.Config, orignalWD string) int {
-
 		paths := work.MustExpandPaths(orignalWD, opts.Lint.Args.Paths)
-		if err := generate.UpdateToStdout(opts.Lint.Format, plzConf, paths...); err != nil {
+		if err := generate.UpdateToStdout(opts.Lint.Format, plzConf, opts.Options, paths...); err != nil {
 			log.Fatalf("%v", err)
 		}
 		return 0
 	},
 	"watch": func(_ *config.Config, plzConf *please.Config, orignalWD string) int {
 		paths := work.MustExpandPaths(orignalWD, opts.Watch.Args.Paths)
-		if err := generate.Update(plzConf, paths...); err != nil {
+		if err := generate.Update(plzConf, opts.Options, paths...); err != nil {
 			log.Fatalf("%v", err)
 		}
 
-		if err := watch.Watch(plzConf, paths...); err != nil {
+		if err := watch.Watch(plzConf, opts.Options, paths...); err != nil {
 			log.Fatalf("%v", err)
 		}
 		return 0
@@ -120,19 +122,19 @@ var funcs = map[string]func(conf *config.Config, plzConf *please.Config, orignal
 		}
 		paths = work.MustExpandPaths(orignalWD, paths)
 		if opts.Migrate.Write {
-			if err := migrate.Migrate(conf, plzConf, opts.Migrate.UpdateGoMod, opts.Migrate.Args.Modules, paths); err != nil {
+			if err := migrate.Migrate(conf, plzConf, opts.Migrate.UpdateGoMod, opts.Migrate.Args.Modules, paths, opts.Options); err != nil {
 				log.Fatalf("%v", err)
 			}
 		} else {
-			if err := migrate.MigrateToStdout(opts.Migrate.Format, conf, plzConf, opts.Migrate.UpdateGoMod, opts.Migrate.Args.Modules, paths); err != nil {
+			if err := migrate.MigrateToStdout(opts.Migrate.Format, conf, plzConf, opts.Migrate.UpdateGoMod, opts.Migrate.Args.Modules, paths, opts.Options); err != nil {
 				log.Fatalf("%v", err)
 			}
 		}
 		return 0
 	},
 	"update": func(_ *config.Config, plzConf *please.Config, orignalWD string) int {
 		paths := work.MustExpandPaths(orignalWD, opts.Licenses.Update.Args.Paths)
-		l := licences.New(proxy.New(proxy.DefaultURL), graph.New(plzConf.BuildFileNames()))
+		l := licences.New(proxy.New(proxy.DefaultURL), graph.New(plzConf.BuildFileNames(), opts.Options))
 		if opts.Licenses.Update.Write {
 			if err := l.Update(paths); err != nil {
 				log.Fatalf("%v", err)

generate/BUILD

@@ -29,6 +29,7 @@ go_library(
         "//please",
         "//proxy",
         "//trie",
+        "//options",
     ],
 )
 
@@ -46,5 +47,6 @@ testify_test(
         "//please",
         "//proxy",
         "//trie",
+        "//options",
     ],
 )

generate/deps_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/please-build/puku/config"
+	"github.com/please-build/puku/options"
 	"github.com/please-build/puku/please"
 	"github.com/please-build/puku/proxy"
 	"github.com/please-build/puku/trie"
@@ -38,7 +39,7 @@ func TestLocalDeps(t *testing.T) {
 	conf.Parse.BuildFileName = []string{"BUILD_FILE", "BUILD_FILE.plz"}
 	conf.Plugin.Go.ImportPath = []string{"github.com/some/module"}
 
-	u := newUpdater(conf)
+	u := newUpdater(conf, options.TestOptions)
 
 	trgt, err := u.localDep("test_project/foo")
 	require.NoError(t, err)

generate/generate.go

@@ -18,6 +18,7 @@ import (
 	"github.com/please-build/puku/kinds"
 	"github.com/please-build/puku/licences"
 	"github.com/please-build/puku/logging"
+	"github.com/please-build/puku/options"
 	"github.com/please-build/puku/please"
 	"github.com/please-build/puku/proxy"
 	"github.com/please-build/puku/trie"
@@ -64,22 +65,22 @@ func newUpdaterWithGraph(g *graph.Graph, conf *please.Config) *updater {
 
 // newUpdater initialises a new updater struct. It's intended to be only used for testing (as is
 // newUpdaterWithGraph). In most instances the Update function should be called directly.
-func newUpdater(conf *please.Config) *updater {
-	g := graph.New(conf.BuildFileNames()).WithExperimentalDirs(conf.Parse.ExperimentalDir...)
+func newUpdater(conf *please.Config, opts options.Options) *updater {
+	g := graph.New(conf.BuildFileNames(), opts).WithExperimentalDirs(conf.Parse.ExperimentalDir...)
 
 	return newUpdaterWithGraph(g, conf)
 }
 
-func Update(plzConf *please.Config, paths ...string) error {
-	u := newUpdater(plzConf)
+func Update(plzConf *please.Config, opts options.Options, paths ...string) error {
+	u := newUpdater(plzConf, opts)
 	if err := u.update(paths...); err != nil {
 		return err
 	}
 	return u.graph.FormatFiles()
 }
 
-func UpdateToStdout(format string, plzConf *please.Config, paths ...string) error {
-	u := newUpdater(plzConf)
+func UpdateToStdout(format string, plzConf *please.Config, opts options.Options, paths ...string) error {
+	u := newUpdater(plzConf, opts)
 	if err := u.update(paths...); err != nil {
 		return err
 	}

generate/generate_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/please-build/puku/config"
 	"github.com/please-build/puku/edit"
 	"github.com/please-build/puku/kinds"
+	"github.com/please-build/puku/options"
 	"github.com/please-build/puku/please"
 )
 
@@ -44,7 +45,7 @@ func TestAllocateSources(t *testing.T) {
 		},
 	}
 
-	u := newUpdater(new(please.Config))
+	u := newUpdater(new(please.Config), options.TestOptions)
 	conf := &config.Config{PleasePath: "plz"}
 	newRules, err := u.allocateSources(conf, "foo", files, rules)
 	require.NoError(t, err)
@@ -75,7 +76,7 @@ func TestAddingLibDepToTest(t *testing.T) {
 	foo.SetAttr(foo.SrcsAttr(), edit.NewStringList([]string{"foo.go"}))
 	fooTest.SetAttr(fooTest.SrcsAttr(), edit.NewStringList([]string{"foo_test.go"}))
 
-	u := newUpdater(new(please.Config))
+	u := newUpdater(new(please.Config), options.TestOptions)
 	conf := &config.Config{PleasePath: "plz"}
 	err := u.updateRuleDeps(conf, fooTest, []*rule{foo, fooTest}, files)
 	require.NoError(t, err)
@@ -122,7 +123,7 @@ func TestAllocateSourcesToCustomKind(t *testing.T) {
 		},
 	}
 
-	u := newUpdater(new(please.Config))
+	u := newUpdater(new(please.Config), options.TestOptions)
 	conf := &config.Config{PleasePath: "plz"}
 	newRules, err := u.allocateSources(conf, "foo", files, rules)
 	require.NoError(t, err)
@@ -152,7 +153,7 @@ func TestAllocateSourcesToNonGoKind(t *testing.T) {
 		},
 	}
 
-	u := newUpdater(new(please.Config))
+	u := newUpdater(new(please.Config), options.TestOptions)
 	u.plzConf = &please.Config{}
 	newRules, err := u.allocateSources(new(config.Config), "foo", files, rules)
 	require.NoError(t, err)
@@ -263,7 +264,7 @@ func TestUpdateDeps(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			plzConf := new(please.Config)
 			plzConf.Plugin.Go.ImportPath = []string{"github.com/this/module"}
-			u := newUpdater(plzConf)
+			u := newUpdater(plzConf, options.TestOptions)
 			u.modules = tc.modules
 			u.proxy = tc.proxy
 

graph/BUILD

@@ -18,6 +18,7 @@ go_library(
         "//edit",
         "//fs",
         "//logging",
+        "//options",
     ],
 )
 
@@ -33,5 +34,6 @@ go_test(
         "///third_party/go/github.com_stretchr_testify//require",
         "//config",
         "//edit",
+        "//options",
     ],
 )

graph/graph.go

@@ -16,6 +16,7 @@ import (
 	"github.com/please-build/puku/edit"
 	"github.com/please-build/puku/fs"
 	"github.com/please-build/puku/logging"
+	"github.com/please-build/puku/options"
 )
 
 var log = logging.GetLogger()
@@ -29,12 +30,14 @@ type Graph struct {
 	files            map[string]*build.File
 	deps             []*Dependency
 	experimentalDirs []string
+	opts             options.Options
 }
 
-func New(buildFileNames []string) *Graph {
+func New(buildFileNames []string, opts options.Options) *Graph {
 	return &Graph{
 		buildFileNames: buildFileNames,
 		files:          map[string]*build.File{},
+		opts:           opts,
 	}
 }
 
@@ -128,7 +131,7 @@ func (g *Graph) FormatFilesWithWriter(out io.Writer, format string) error {
 		return err
 	}
 	for _, file := range g.files {
-		if err := writeFormattedBuildFile(file, out, format); err != nil {
+		if err := writeFormattedBuildFile(file, out, format, g.opts); err != nil {
 			return err
 		}
 	}
@@ -140,7 +143,7 @@ func (g *Graph) FormatFiles() error {
 		return err
 	}
 	for _, file := range g.files {
-		if err := saveFormattedBuildFile(file); err != nil {
+		if err := saveFormattedBuildFile(file, g.opts); err != nil {
 			return err
 		}
 	}
@@ -231,11 +234,50 @@ func checkVisibility(target labels.Label, visibilities []string) bool {
 	return false
 }
 
-func writeFormattedBuildFile(buildFile *build.File, out io.Writer, format string) error {
+type nopCloser struct {
+	io.Writer
+}
+
+func (nopCloser) Close() error { return nil }
+
+// writeFormattedBuildFile writes a build file to the given writer if puku has made meaningful changes.
+//
+// See the comment on outputFormattedBuildFile for more details.
+func writeFormattedBuildFile(buildFile *build.File, out io.Writer, format string, opts options.Options) error {
+	outFn := func() (io.WriteCloser, error) {
+		return nopCloser{out}, nil
+	}
+	return outputFormattedBuildFile(buildFile, outFn, format, opts)
+}
+
+// saveFormattedBuildFile writes a build file to disk if puku has made meaningful changes.
+//
+// See the comment on outputFormattedBuildFile for more details.
+func saveFormattedBuildFile(buildFile *build.File, opts options.Options) error {
+	outFn := func() (io.WriteCloser, error) {
+		return os.Create(buildFile.Path)
+	}
+
+	return outputFormattedBuildFile(buildFile, outFn, "text", opts)
+}
+
+// outputFormattedBuildFile writes a build file to the given writer if puku has made meaningful changes.
+//
+// To avoid churn and changes to files where puku has not changed anything, checking for changes is
+// done by comparing the formatted build file without applying rewriting (which roughly means linter
+// changes). If changes do exist and skipRewriting is not true, the rewriting is applied to ensure
+// the resulting build file will satisfy `plz fmt`.
+//
+// This takes a function to obtain the writer because this needs to read the file to check if puku
+// has made any changes before it writes to it. If saveFormattedBuildFile called os.Create
+// proactively, the file would be truncated, and so we'd always try to write to it.
+func outputFormattedBuildFile(buildFile *build.File, outFn func() (io.WriteCloser, error), format string, opts options.Options) error {
 	if len(buildFile.Stmt) == 0 {
 		return nil
 	}
-	target := build.FormatWithoutRewriting(buildFile)
+
+	content := build.FormatWithoutRewriting(buildFile)
+
 	actual, err := os.ReadFile(buildFile.Path)
 	if err != nil {
 		if !os.IsNotExist(err) {
@@ -244,30 +286,28 @@ func writeFormattedBuildFile(buildFile *build.File, out io.Writer, format string
 		actual = nil
 	}
 
-	if !bytes.Equal(target, actual) {
-		switch format {
-		case "text":
-			_, err := out.Write(target)
-			return err
-		case "json":
-			e := json.NewEncoder(out)
-			return e.Encode(struct{ Path, Content string }{Path: buildFile.Path, Content: string(target)})
-		}
-	}
-	return nil
-}
-
-func saveFormattedBuildFile(buildFile *build.File) error {
-	if len(buildFile.Stmt) == 0 {
+	if bytes.Equal(content, actual) {
 		return nil
 	}
 
-	f, err := os.Create(buildFile.Path)
+	w, err := outFn()
 	if err != nil {
 		return err
 	}
-	defer f.Close()
+	defer w.Close()
+
+	if !opts.SkipRewriting {
+		content = build.Format(buildFile)
+	}
 
-	_, err = f.Write(build.FormatWithoutRewriting(buildFile))
-	return err
+	switch format {
+	case "text":
+		_, err := w.Write(content)
+		return err
+	case "json":
+		e := json.NewEncoder(w)
+		return e.Encode(struct{ Path, Content string }{Path: buildFile.Path, Content: string(content)})
+	default:
+		return fmt.Errorf("unsupported format %q", format)
+	}
 }