Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support only updating existing BUILD files #1921

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: support only updating existing BUILD files #1921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions walk/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,27 @@ import (
gzflag "github.com/bazelbuild/bazel-gazelle/flag"
)

// GenerationModeType represents one of the generation modes.
type GenerationModeType string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this private to the package?


// Generation modes
const (
// Update: update and maintain existing BUILD files
GenerationModeUpdate GenerationModeType = "update"

// Create: create new and update existing BUILD files
GenerationModeCreate GenerationModeType = "create"
Comment on lines +42 to +45
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about update_only and create_and_update? Slightly more verbose, but also more descriptive.

)

// TODO(#472): store location information to validate each exclude. They
// may be set in one directory and used in another. Excludes work on
// declared generated files, so we can't just stat.

type walkConfig struct {
excludes []string
ignore bool
follow []string
updateOnly bool
excludes []string
ignore bool
follow []string
}

const walkName = "_walk"
Expand Down Expand Up @@ -70,7 +83,7 @@ func (*Configurer) RegisterFlags(fs *flag.FlagSet, cmd string, c *config.Config)
func (*Configurer) CheckFlags(fs *flag.FlagSet, c *config.Config) error { return nil }

func (*Configurer) KnownDirectives() []string {
return []string{"exclude", "follow", "ignore"}
return []string{"generation_mode", "exclude", "follow", "ignore"}
Copy link
Contributor Author

@jbedard jbedard Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the directive named used by the Aspect plugins today. I think we need something better here that doesn't include the word "mode" which already has a meaning within gazelle.

Ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_file_action?
create_build_files yes/no?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to try to this patch at Airtable. I am thinking instead of manually specifying generation_mode to isntead do something like (pseudocode) collectionOnly := f == nil !any(file.endswith('.go') for file in ents), since we have go code spread throughout the tree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think different repos will have different preferences and we can't really make any assumptions based on which files exist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming you are in David's situation, could you still leverage generation_mode: update to simplify your language implementation or would you need to resort to create with the same hack mentioned in the linked issue?

I'm asking because adding a new directive comes with a complexity cost that may not be justified if it doesn't address many use cases. Do you see a way to simplify what a language would need to do to get this behavior without adding a new directive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed my hack described in the issue to a patch that's essentially the same as this PR. The hack required manually walking the fs (in addition to gazelle walking it) which is bad in many ways, performance being a big one.

I think no matter what a new API is needed so gazelle can generate the language.GenerateArgs with the correct files. It could be an API in the Configure phase, but today there are no APIs there that change gazelle's behaviour and adding such an API sounds far more complex then adding a directive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add docs for the new directive.

I don't have an idea, but would prefer a more descriptive term if we can find one. We already have a build_file_mode.

}

func (cr *Configurer) Configure(c *config.Config, rel string, f *rule.File) {
Expand All @@ -82,6 +95,8 @@ func (cr *Configurer) Configure(c *config.Config, rel string, f *rule.File) {
if f != nil {
for _, d := range f.Directives {
switch d.Key {
case "generation_mode":
wcCopy.updateOnly = GenerationModeType(strings.TrimSpace(d.Value)) == GenerationModeUpdate
case "exclude":
if err := checkPathMatchPattern(path.Join(rel, d.Value)); err != nil {
log.Printf("the exclusion pattern is not valid %q: %s", path.Join(rel, d.Value), err)
Expand Down
29 changes: 22 additions & 7 deletions walk/walk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func Walk(c *config.Config, cexts []config.Configurer, dirs []string, mode Mode,
//
// Traversal may skip subtrees or files based on the config.Config exclude/ignore/follow options
// as well as the UpdateFilter callbacks.
func visit(c *config.Config, cexts []config.Configurer, knownDirectives map[string]bool, updateRels *UpdateFilter, trie *pathTrie, wf WalkFunc, rel string, updateParent bool) {
func visit(c *config.Config, cexts []config.Configurer, knownDirectives map[string]bool, updateRels *UpdateFilter, trie *pathTrie, wf WalkFunc, rel string, updateParent bool) ([]string, bool) {
haveError := false

// Absolute path to the directory being visited
Expand All @@ -157,11 +157,16 @@ func visit(c *config.Config, cexts []config.Configurer, knownDirectives map[stri
haveError = true
}

configure(cexts, knownDirectives, c, rel, f)
wc := getWalkConfig(c)
collectionOnly := f == nil && getWalkConfig(c).updateOnly

// Configure the current directory if not only collecting files
if !collectionOnly {
configure(cexts, knownDirectives, c, rel, f)
}

wc := getWalkConfig(c)
if wc.isExcluded(rel) {
return
return nil, false
}

// Filter and collect files
Expand All @@ -188,19 +193,29 @@ func visit(c *config.Config, cexts []config.Configurer, knownDirectives map[stri
continue
}
if ent := resolveFileInfo(wc, dir, entRel, t.entry); ent != nil {
subdirs = append(subdirs, base)

if updateRels.shouldVisit(entRel, shouldUpdate) {
visit(c.Clone(), cexts, knownDirectives, updateRels, t, wf, entRel, shouldUpdate)
subFiles, shouldMerge := visit(c.Clone(), cexts, knownDirectives, updateRels, t, wf, entRel, shouldUpdate)
if shouldMerge {
for _, f := range subFiles {
regularFiles = append(regularFiles, path.Join(base, f))
}
} else {
subdirs = append(subdirs, base)
}
}
}
}

if collectionOnly {
return regularFiles, true
}

update := !haveError && !wc.ignore && shouldUpdate
if updateRels.shouldCall(rel, updateParent) {
genFiles := findGenFiles(wc, f)
wf(dir, rel, c, update, f, subdirs, regularFiles, genFiles)
}
return nil, false
}

// An UpdateFilter tracks which directories need to be updated
Expand Down
65 changes: 65 additions & 0 deletions walk/walk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ package walk

import (
"flag"
"fmt"
"path"
"path/filepath"
"reflect"
"testing"

"github.com/bazelbuild/bazel-gazelle/config"
Expand Down Expand Up @@ -141,6 +143,69 @@ func TestUpdateDirs(t *testing.T) {
}
}

func TestGenMode(t *testing.T) {
dir, cleanup := testtools.CreateFiles(t, []testtools.FileSpec{
{Path: "mode-create/"},
{Path: "mode-create/a.go"},
{Path: "mode-create/sub/"},
{Path: "mode-create/sub/b.go"},
{Path: "mode-create/sub/sub2/"},
{Path: "mode-create/sub/sub2/sub3/c.go"},
{Path: "mode-update/"},
{
Path: "mode-update/BUILD.bazel",
Content: "# gazelle:generation_mode update",
},
{Path: "mode-update/a.go"},
{Path: "mode-update/sub/"},
{Path: "mode-update/sub/b.go"},
{Path: "mode-update/sub/sub2/"},
{Path: "mode-update/sub/sub2/sub3/c.go"},
{Path: "mode-update/sub/sub3/"},
{Path: "mode-update/sub/sub3/BUILD.bazel"},
{Path: "mode-update/sub/sub3/d.go"},
{Path: "mode-update/sub/sub3/sub4/"},
{Path: "mode-update/sub/sub3/sub4/e.go"},
})
defer cleanup()

type visitSpec struct {
subdirs, files []string
}

t.Run("generation_mode create vs update", func(t *testing.T) {
c, cexts := testConfig(t, dir)
var visits []visitSpec
Walk(c, cexts, []string{"."}, VisitAllUpdateSubdirsMode, func(_ string, rel string, _ *config.Config, update bool, _ *rule.File, subdirs, regularFiles, _ []string) {
visits = append(visits, visitSpec{
subdirs: subdirs,
files: regularFiles,
})
})

if len(visits) != 7 {
t.Error(fmt.Sprintf("Expected 7 visits, got %v", len(visits)))
}

if !reflect.DeepEqual(visits[len(visits)-1].subdirs, []string{"mode-create", "mode-update"}) {
t.Errorf("Last visit should be root dir with 2 subdirs")
}

if len(visits[0].subdirs) != 0 || len(visits[0].files) != 1 || visits[0].files[0] != "c.go" {
t.Errorf("Leaf visit should be only files: %v", visits[0])
}
modeUpdateFiles1 := []string{"BUILD.bazel", "d.go", "sub4/e.go"}
if !reflect.DeepEqual(visits[4].files, modeUpdateFiles1) {
t.Errorf("update mode should contain files in subdirs. Want %v, got: %v", modeUpdateFiles1, visits[5].files)
}

modeUpdateFiles2 := []string{"BUILD.bazel", "a.go", "sub/b.go", "sub/sub2/sub3/c.go"}
if !reflect.DeepEqual(visits[5].files, modeUpdateFiles2) {
t.Errorf("update mode should contain files in subdirs. Want %v, got: %v", modeUpdateFiles2, visits[5].files)
}
})
}

func TestCustomBuildName(t *testing.T) {
dir, cleanup := testtools.CreateFiles(t, []testtools.FileSpec{
{
Expand Down