Skip to content

buildozer: support multi-file copy operations #1441

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

Open
soorya38 wants to merge 1 commit into
base: main
Choose a base branch
from
+20 −1
Open

buildozer: support multi-file copy operations #1441

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
21 changes: 20 additions & 1 deletion edit/buildozer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,26 @@ func cmdDictListAdd(opts *Options, env CmdEnvironment) (*build.File, error) {
}

func copyAttributeBetweenRules(env CmdEnvironment, attrName string, from string) (*build.File, error) {
fromRule := FindRuleByName(env.File, from)
// Check if the source rule is in another package
buildFile, _, _, rule := InterpretLabelForWorkspaceLocation(NewOpts().RootDir, from)
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 19, 2026

Choose a reason for hiding this comment

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

high

The call to InterpretLabelForWorkspaceLocation uses NewOpts().RootDir, which creates a new Options object with a default (empty) RootDir. This ignores the RootDir that might have been passed to buildozer via command-line flags, which can lead to incorrect workspace root detection.

To fix this, copyAttributeBetweenRules should accept the *Options struct (passed down from cmdCopy/cmdCopyNoOverwrite) and use opts.RootDir here.

Suggested change
buildFile, _, _, rule := InterpretLabelForWorkspaceLocation(NewOpts().RootDir, from)
buildFile, _, _, rule := InterpretLabelForWorkspaceLocation(opts.RootDir, from)

var fromRule *build.Rule

if buildFile != "" && buildFile != env.File.Path {
// The rule is in another file, we need to load it
data, _, err := file.ReadFile(buildFile)
if err != nil {
return nil, fmt.Errorf("could not read file '%s': %v", buildFile, err)
}
f, err := build.Parse(buildFile, data)
if err != nil {
return nil, fmt.Errorf("could not parse file '%s': %v", buildFile, err)
}
fromRule = FindRuleByName(f, rule)
Comment on lines +818 to +828
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 19, 2026

Choose a reason for hiding this comment

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

medium

This implementation reads and parses the source BUILD file every time a copy command for a rule in that file is executed. If multiple copy commands in a single buildozer invocation reference rules from the same external file, that file will be read and parsed multiple times, which is inefficient.

For example: buildozer "copy attr1 //pkg1:rule1" //pkg2:ruleA "copy attr2 //pkg1:rule1" //pkg2:ruleB would parse pkg1's BUILD file twice.

Consider introducing a cache for parsed *build.File objects that lives for the duration of the buildozer run. This would improve performance for commands that operate on multiple rules from the same external package. The cache could be keyed by file path.

} else {
// The rule is in the same file
fromRule = FindRuleByName(env.File, from)
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 19, 2026

Choose a reason for hiding this comment

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

high

When the source rule is in the same file, you are calling FindRuleByName with from, which is the full label string (e.g., //pkg:rule). However, FindRuleByName likely expects just the rule name part. This is inconsistent with the if branch which correctly uses the parsed rule name.

This will cause lookups to fail for rules specified with an absolute label that are in the current file. You should use the rule variable here for correctness and consistency.

Suggested change
fromRule = FindRuleByName(env.File, from)
fromRule = FindRuleByName(env.File, rule)

}

if fromRule == nil {
return nil, fmt.Errorf("could not find rule '%s'", from)
}
Expand Down