buildozer: support multi-file copy operations #1441
Conversation
Modify copyAttributeBetweenRules to handle cross-package labels. When a label refers to a rule in a different file, the source file is loaded and parsed on-demand to retrieve the attribute. This enables copy and copy_no_overwrite commands to work across files.
Summary of ChangesHello @soorya38, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the capabilities of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great enhancement, adding support for cross-file copy operations in buildozer. The overall approach is solid. I've found a couple of high-severity bugs related to workspace root detection and rule lookups for same-file absolute labels that should be addressed. I've also included a suggestion to improve performance by caching parsed files to avoid redundant I/O. With these changes, the new functionality will be more robust and efficient.
| 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) |
There was a problem hiding this comment.
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.
| buildFile, _, _, rule := InterpretLabelForWorkspaceLocation(NewOpts().RootDir, from) | |
| buildFile, _, _, rule := InterpretLabelForWorkspaceLocation(opts.RootDir, from) |
| fromRule = FindRuleByName(f, rule) | ||
| } else { | ||
| // The rule is in the same file | ||
| fromRule = FindRuleByName(env.File, from) |
There was a problem hiding this comment.
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.
| fromRule = FindRuleByName(env.File, from) | |
| fromRule = FindRuleByName(env.File, 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) |
There was a problem hiding this comment.
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.
1 participant
This PR significantly enhances the
copy(andcopy_no_overwrite) command in buildozer by adding support for cross-file operations.Previously, buildozer assumed that the source rule for a copy operation existed in the same BUILD file as the target rule. This change removes that limitation.
Related issue: #1436
What’s Changed
When the source rule label refers to a different package/file (for example):
buildozer 'copy deps //pkg1:rule1' //pkg2:rule2buildozer will now:
This enables copying or moving attributes across multiple BUILD files, addressing a known limitation in current behavior.
Testing
Local Reproduction
Verified using a script that creates two packages (
pkg1,pkg2) and successfully copies thedepsattribute from a rule inpkg1to a rule inpkg2.Existing Tests
Ran the following to ensure no regressions:
go test ./edit/...Related Issue
Checklist
go test ./...)