Skip to content

Commit 99d1cd9

Browse files
toastwaffletoastwaffle
authored
Make puku rewrite build files by default, with a flag to skip rewriting (#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
1 parent be847d8 commit 99d1cd9

22 files changed

+160
-64
lines changed

Diff for: ‎ChangeLog

+4
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
Version 1.11.0
2+
-------------
3+
* Make puku rewrite build files by default, with a flag to skip rewriting
4+
15
Version 1.10.0
26
-------------
37
* Adds a `puku version` command to report the version of puku that is installed.

Diff for: ‎VERSION

+1-1
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
1.10.0
1+
1.11.0

Diff for: ‎cmd/puku/BUILD

+1
Original file line numberDiff line numberDiff line change
@@ -22,5 +22,6 @@ go_binary(
2222
"//version",
2323
"//watch",
2424
"//work",
25+
"//options",
2526
],
2627
)

Diff for: ‎cmd/puku/puku.go

+11-9
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/please-build/puku/licences"
1515
"github.com/please-build/puku/logging"
1616
"github.com/please-build/puku/migrate"
17+
"github.com/please-build/puku/options"
1718
"github.com/please-build/puku/please"
1819
"github.com/please-build/puku/proxy"
1920
"github.com/please-build/puku/sync"
@@ -23,6 +24,8 @@ import (
2324
)
2425

2526
var opts = struct {
27+
options.Options
28+
2629
Usage string
2730
Verbosity clilogging.Verbosity `short:"v" long:"verbosity" description:"Verbosity of output (error, warning, notice, info, debug)" default:"info"`
2831

@@ -76,13 +79,13 @@ var log = logging.GetLogger()
7679
var funcs = map[string]func(conf *config.Config, plzConf *please.Config, orignalWD string) int{
7780
"fmt": func(_ *config.Config, plzConf *please.Config, orignalWD string) int {
7881
paths := work.MustExpandPaths(orignalWD, opts.Fmt.Args.Paths)
79-
if err := generate.Update(plzConf, paths...); err != nil {
82+
if err := generate.Update(plzConf, opts.Options, paths...); err != nil {
8083
log.Fatalf("%v", err)
8184
}
8285
return 0
8386
},
8487
"sync": func(_ *config.Config, plzConf *please.Config, _ string) int {
85-
g := graph.New(plzConf.BuildFileNames())
88+
g := graph.New(plzConf.BuildFileNames(), opts.Options)
8689
if opts.Sync.Write {
8790
if err := sync.Sync(plzConf, g); err != nil {
8891
log.Fatalf("%v", err)
@@ -95,20 +98,19 @@ var funcs = map[string]func(conf *config.Config, plzConf *please.Config, orignal
9598
return 0
9699
},
97100
"lint": func(_ *config.Config, plzConf *please.Config, orignalWD string) int {
98-
99101
paths := work.MustExpandPaths(orignalWD, opts.Lint.Args.Paths)
100-
if err := generate.UpdateToStdout(opts.Lint.Format, plzConf, paths...); err != nil {
102+
if err := generate.UpdateToStdout(opts.Lint.Format, plzConf, opts.Options, paths...); err != nil {
101103
log.Fatalf("%v", err)
102104
}
103105
return 0
104106
},
105107
"watch": func(_ *config.Config, plzConf *please.Config, orignalWD string) int {
106108
paths := work.MustExpandPaths(orignalWD, opts.Watch.Args.Paths)
107-
if err := generate.Update(plzConf, paths...); err != nil {
109+
if err := generate.Update(plzConf, opts.Options, paths...); err != nil {
108110
log.Fatalf("%v", err)
109111
}
110112

111-
if err := watch.Watch(plzConf, paths...); err != nil {
113+
if err := watch.Watch(plzConf, opts.Options, paths...); err != nil {
112114
log.Fatalf("%v", err)
113115
}
114116
return 0
@@ -120,19 +122,19 @@ var funcs = map[string]func(conf *config.Config, plzConf *please.Config, orignal
120122
}
121123
paths = work.MustExpandPaths(orignalWD, paths)
122124
if opts.Migrate.Write {
123-
if err := migrate.Migrate(conf, plzConf, opts.Migrate.UpdateGoMod, opts.Migrate.Args.Modules, paths); err != nil {
125+
if err := migrate.Migrate(conf, plzConf, opts.Migrate.UpdateGoMod, opts.Migrate.Args.Modules, paths, opts.Options); err != nil {
124126
log.Fatalf("%v", err)
125127
}
126128
} else {
127-
if err := migrate.MigrateToStdout(opts.Migrate.Format, conf, plzConf, opts.Migrate.UpdateGoMod, opts.Migrate.Args.Modules, paths); err != nil {
129+
if err := migrate.MigrateToStdout(opts.Migrate.Format, conf, plzConf, opts.Migrate.UpdateGoMod, opts.Migrate.Args.Modules, paths, opts.Options); err != nil {
128130
log.Fatalf("%v", err)
129131
}
130132
}
131133
return 0
132134
},
133135
"update": func(_ *config.Config, plzConf *please.Config, orignalWD string) int {
134136
paths := work.MustExpandPaths(orignalWD, opts.Licenses.Update.Args.Paths)
135-
l := licences.New(proxy.New(proxy.DefaultURL), graph.New(plzConf.BuildFileNames()))
137+
l := licences.New(proxy.New(proxy.DefaultURL), graph.New(plzConf.BuildFileNames(), opts.Options))
136138
if opts.Licenses.Update.Write {
137139
if err := l.Update(paths); err != nil {
138140
log.Fatalf("%v", err)

Diff for: ‎generate/BUILD

+2
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ go_library(
2929
"//please",
3030
"//proxy",
3131
"//trie",
32+
"//options",
3233
],
3334
)
3435

@@ -46,5 +47,6 @@ testify_test(
4647
"//please",
4748
"//proxy",
4849
"//trie",
50+
"//options",
4951
],
5052
)

Diff for: ‎generate/deps_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"github.com/stretchr/testify/require"
99

1010
"github.com/please-build/puku/config"
11+
"github.com/please-build/puku/options"
1112
"github.com/please-build/puku/please"
1213
"github.com/please-build/puku/proxy"
1314
"github.com/please-build/puku/trie"
@@ -38,7 +39,7 @@ func TestLocalDeps(t *testing.T) {
3839
conf.Parse.BuildFileName = []string{"BUILD_FILE", "BUILD_FILE.plz"}
3940
conf.Plugin.Go.ImportPath = []string{"github.com/some/module"}
4041

41-
u := newUpdater(conf)
42+
u := newUpdater(conf, options.TestOptions)
4243

4344
trgt, err := u.localDep("test_project/foo")
4445
require.NoError(t, err)

Diff for: ‎generate/generate.go

+7-6
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/please-build/puku/kinds"
1919
"github.com/please-build/puku/licences"
2020
"github.com/please-build/puku/logging"
21+
"github.com/please-build/puku/options"
2122
"github.com/please-build/puku/please"
2223
"github.com/please-build/puku/proxy"
2324
"github.com/please-build/puku/trie"
@@ -64,22 +65,22 @@ func newUpdaterWithGraph(g *graph.Graph, conf *please.Config) *updater {
6465

6566
// newUpdater initialises a new updater struct. It's intended to be only used for testing (as is
6667
// newUpdaterWithGraph). In most instances the Update function should be called directly.
67-
func newUpdater(conf *please.Config) *updater {
68-
g := graph.New(conf.BuildFileNames()).WithExperimentalDirs(conf.Parse.ExperimentalDir...)
68+
func newUpdater(conf *please.Config, opts options.Options) *updater {
69+
g := graph.New(conf.BuildFileNames(), opts).WithExperimentalDirs(conf.Parse.ExperimentalDir...)
6970

7071
return newUpdaterWithGraph(g, conf)
7172
}
7273

73-
func Update(plzConf *please.Config, paths ...string) error {
74-
u := newUpdater(plzConf)
74+
func Update(plzConf *please.Config, opts options.Options, paths ...string) error {
75+
u := newUpdater(plzConf, opts)
7576
if err := u.update(paths...); err != nil {
7677
return err
7778
}
7879
return u.graph.FormatFiles()
7980
}
8081

81-
func UpdateToStdout(format string, plzConf *please.Config, paths ...string) error {
82-
u := newUpdater(plzConf)
82+
func UpdateToStdout(format string, plzConf *please.Config, opts options.Options, paths ...string) error {
83+
u := newUpdater(plzConf, opts)
8384
if err := u.update(paths...); err != nil {
8485
return err
8586
}

Diff for: ‎generate/generate_test.go

+6-5
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"github.com/please-build/puku/config"
1010
"github.com/please-build/puku/edit"
1111
"github.com/please-build/puku/kinds"
12+
"github.com/please-build/puku/options"
1213
"github.com/please-build/puku/please"
1314
)
1415

@@ -44,7 +45,7 @@ func TestAllocateSources(t *testing.T) {
4445
},
4546
}
4647

47-
u := newUpdater(new(please.Config))
48+
u := newUpdater(new(please.Config), options.TestOptions)
4849
conf := &config.Config{PleasePath: "plz"}
4950
newRules, err := u.allocateSources(conf, "foo", files, rules)
5051
require.NoError(t, err)
@@ -75,7 +76,7 @@ func TestAddingLibDepToTest(t *testing.T) {
7576
foo.SetAttr(foo.SrcsAttr(), edit.NewStringList([]string{"foo.go"}))
7677
fooTest.SetAttr(fooTest.SrcsAttr(), edit.NewStringList([]string{"foo_test.go"}))
7778

78-
u := newUpdater(new(please.Config))
79+
u := newUpdater(new(please.Config), options.TestOptions)
7980
conf := &config.Config{PleasePath: "plz"}
8081
err := u.updateRuleDeps(conf, fooTest, []*rule{foo, fooTest}, files)
8182
require.NoError(t, err)
@@ -122,7 +123,7 @@ func TestAllocateSourcesToCustomKind(t *testing.T) {
122123
},
123124
}
124125

125-
u := newUpdater(new(please.Config))
126+
u := newUpdater(new(please.Config), options.TestOptions)
126127
conf := &config.Config{PleasePath: "plz"}
127128
newRules, err := u.allocateSources(conf, "foo", files, rules)
128129
require.NoError(t, err)
@@ -152,7 +153,7 @@ func TestAllocateSourcesToNonGoKind(t *testing.T) {
152153
},
153154
}
154155

155-
u := newUpdater(new(please.Config))
156+
u := newUpdater(new(please.Config), options.TestOptions)
156157
u.plzConf = &please.Config{}
157158
newRules, err := u.allocateSources(new(config.Config), "foo", files, rules)
158159
require.NoError(t, err)
@@ -263,7 +264,7 @@ func TestUpdateDeps(t *testing.T) {
263264
t.Run(tc.name, func(t *testing.T) {
264265
plzConf := new(please.Config)
265266
plzConf.Plugin.Go.ImportPath = []string{"github.com/this/module"}
266-
u := newUpdater(plzConf)
267+
u := newUpdater(plzConf, options.TestOptions)
267268
u.modules = tc.modules
268269
u.proxy = tc.proxy
269270

Diff for: ‎graph/BUILD

+2
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ go_library(
1818
"//edit",
1919
"//fs",
2020
"//logging",
21+
"//options",
2122
],
2223
)
2324

@@ -33,5 +34,6 @@ go_test(
3334
"///third_party/go/github.com_stretchr_testify//require",
3435
"//config",
3536
"//edit",
37+
"//options",
3638
],
3739
)

Diff for: ‎graph/graph.go

+64-24
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/please-build/puku/edit"
1717
"github.com/please-build/puku/fs"
1818
"github.com/please-build/puku/logging"
19+
"github.com/please-build/puku/options"
1920
)
2021

2122
var log = logging.GetLogger()
@@ -29,12 +30,14 @@ type Graph struct {
2930
files map[string]*build.File
3031
deps []*Dependency
3132
experimentalDirs []string
33+
opts options.Options
3234
}
3335

34-
func New(buildFileNames []string) *Graph {
36+
func New(buildFileNames []string, opts options.Options) *Graph {
3537
return &Graph{
3638
buildFileNames: buildFileNames,
3739
files: map[string]*build.File{},
40+
opts: opts,
3841
}
3942
}
4043

@@ -128,7 +131,7 @@ func (g *Graph) FormatFilesWithWriter(out io.Writer, format string) error {
128131
return err
129132
}
130133
for _, file := range g.files {
131-
if err := writeFormattedBuildFile(file, out, format); err != nil {
134+
if err := writeFormattedBuildFile(file, out, format, g.opts); err != nil {
132135
return err
133136
}
134137
}
@@ -140,7 +143,7 @@ func (g *Graph) FormatFiles() error {
140143
return err
141144
}
142145
for _, file := range g.files {
143-
if err := saveFormattedBuildFile(file); err != nil {
146+
if err := saveFormattedBuildFile(file, g.opts); err != nil {
144147
return err
145148
}
146149
}
@@ -231,11 +234,50 @@ func checkVisibility(target labels.Label, visibilities []string) bool {
231234
return false
232235
}
233236

234-
func writeFormattedBuildFile(buildFile *build.File, out io.Writer, format string) error {
237+
type nopCloser struct {
238+
io.Writer
239+
}
240+
241+
func (nopCloser) Close() error { return nil }
242+
243+
// writeFormattedBuildFile writes a build file to the given writer if puku has made meaningful changes.
244+
//
245+
// See the comment on outputFormattedBuildFile for more details.
246+
func writeFormattedBuildFile(buildFile *build.File, out io.Writer, format string, opts options.Options) error {
247+
outFn := func() (io.WriteCloser, error) {
248+
return nopCloser{out}, nil
249+
}
250+
return outputFormattedBuildFile(buildFile, outFn, format, opts)
251+
}
252+
253+
// saveFormattedBuildFile writes a build file to disk if puku has made meaningful changes.
254+
//
255+
// See the comment on outputFormattedBuildFile for more details.
256+
func saveFormattedBuildFile(buildFile *build.File, opts options.Options) error {
257+
outFn := func() (io.WriteCloser, error) {
258+
return os.Create(buildFile.Path)
259+
}
260+
261+
return outputFormattedBuildFile(buildFile, outFn, "text", opts)
262+
}
263+
264+
// outputFormattedBuildFile writes a build file to the given writer if puku has made meaningful changes.
265+
//
266+
// To avoid churn and changes to files where puku has not changed anything, checking for changes is
267+
// done by comparing the formatted build file without applying rewriting (which roughly means linter
268+
// changes). If changes do exist and skipRewriting is not true, the rewriting is applied to ensure
269+
// the resulting build file will satisfy `plz fmt`.
270+
//
271+
// This takes a function to obtain the writer because this needs to read the file to check if puku
272+
// has made any changes before it writes to it. If saveFormattedBuildFile called os.Create
273+
// proactively, the file would be truncated, and so we'd always try to write to it.
274+
func outputFormattedBuildFile(buildFile *build.File, outFn func() (io.WriteCloser, error), format string, opts options.Options) error {
235275
if len(buildFile.Stmt) == 0 {
236276
return nil
237277
}
238-
target := build.FormatWithoutRewriting(buildFile)
278+
279+
content := build.FormatWithoutRewriting(buildFile)
280+
239281
actual, err := os.ReadFile(buildFile.Path)
240282
if err != nil {
241283
if !os.IsNotExist(err) {
@@ -244,30 +286,28 @@ func writeFormattedBuildFile(buildFile *build.File, out io.Writer, format string
244286
actual = nil
245287
}
246288

247-
if !bytes.Equal(target, actual) {
248-
switch format {
249-
case "text":
250-
_, err := out.Write(target)
251-
return err
252-
case "json":
253-
e := json.NewEncoder(out)
254-
return e.Encode(struct{ Path, Content string }{Path: buildFile.Path, Content: string(target)})
255-
}
256-
}
257-
return nil
258-
}
259-
260-
func saveFormattedBuildFile(buildFile *build.File) error {
261-
if len(buildFile.Stmt) == 0 {
289+
if bytes.Equal(content, actual) {
262290
return nil
263291
}
264292

265-
f, err := os.Create(buildFile.Path)
293+
w, err := outFn()
266294
if err != nil {
267295
return err
268296
}
269-
defer f.Close()
297+
defer w.Close()
298+
299+
if !opts.SkipRewriting {
300+
content = build.Format(buildFile)
301+
}
270302

271-
_, err = f.Write(build.FormatWithoutRewriting(buildFile))
272-
return err
303+
switch format {
304+
case "text":
305+
_, err := w.Write(content)
306+
return err
307+
case "json":
308+
e := json.NewEncoder(w)
309+
return e.Encode(struct{ Path, Content string }{Path: buildFile.Path, Content: string(content)})
310+
default:
311+
return fmt.Errorf("unsupported format %q", format)
312+
}
273313
}

0 commit comments

Comments
 (0)