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

Jump to bottom

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

wants to merge 1 commit into from

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Sep 12, 2024

Close #1832

What type of PR is this?

Feature

Other

What package or component does this PR mostly affect?

all

What does this PR do? Why is it needed?

Add an option to enable only updating existing BUILDs and not creating any new ones.

I think this is common among extension authors, normally implemented with a quick-exit within Configure if the *rule.File is nil. However all the subdirs then need to be manually traversed to find files in subdirectories with no BUILD.

I've always maintained a patch similar to this PR to avoid plugins having to do manual fs operations.

Which issues(s) does this PR fix?

Fixes #1832

Other notes for review

jbedard
jbedard commented Sep 12, 2024
View reviewed changes
walk/config.go
@@ -73,7 +86,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.

dzbarsky
dzbarsky reviewed Sep 13, 2024
View reviewed changes
walk/walk.go Outdated Show resolved Hide resolved
dzbarsky
dzbarsky reviewed Sep 13, 2024
View reviewed changes
walk/walk.go Outdated Show resolved Hide resolved
@jbedard jbedard requested a review from dzbarsky September 14, 2024 08:12
@jbedard jbedard force-pushed the 1832-gen-mode branch 4 times, most recently from 496ec9e to fb71250 Compare September 21, 2024 18:25
@jbedard jbedard force-pushed the 1832-gen-mode branch 2 times, most recently from 0e6a43f to 3569d83 Compare October 23, 2024 18:27
@jbedard jbedard mentioned this pull request Oct 23, 2024
Merged
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 23, 2024
@jbedard
refactor(cli): move gazelle gitignore and walk logic to common (#7016)
ffd8941 
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
@jbedard
refactor(cli): move gazelle gitignore and walk logic to common (#7016)
16bb135 
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
@jbedard
refactor(cli): move gazelle gitignore and walk logic to common (#7016)
35a76e4 
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
@jbedard
refactor(cli): move gazelle gitignore and walk logic to common (#7016)
662b547 
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
@jbedard
refactor(cli): move gazelle gitignore and walk logic to common (#7016)
881670f 
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
@jbedard
refactor(cli): move gazelle gitignore and walk logic to common (#7016)
cc1d2b9 
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
@jbedard
refactor(cli): move gazelle gitignore and walk logic to common (#7016)
65bf3a1 
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
@jbedard jbedard requested a review from fmeum October 25, 2024 23:31
@jbedard
Copy link
Contributor Author

jbedard commented Dec 18, 2024

I've rebased this and added a test.

@fmeum any ideas or feedback here? WDYT?

fmeum
fmeum approved these changes Dec 27, 2024
View reviewed changes
Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I find this useful and it doesn't introduce a lot of complexity, so I'm happy to merge after the minor comments have been resolved.

walk/config.go
Comment on lines +42 to +45
GenerationModeUpdate GenerationModeType = "update"

// Create: create new and update existing BUILD files
GenerationModeCreate GenerationModeType = "create"
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.

walk/config.go
@@ -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?

walk/config.go
@@ -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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidZbarsky-at DavidZbarsky-at DavidZbarsky-at left review comments

@fmeum fmeum fmeum approved these changes

@dzbarsky dzbarsky Awaiting requested review from dzbarsky

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

request: option to not generate in all directories
4 participants
@jbedard @dzbarsky @fmeum @DavidZbarsky-at