From 6febf8239081fb0e076ccd42134345ea7878c073 Mon Sep 17 00:00:00 2001 From: Vladimir Suvorov Date: Fri, 8 May 2026 21:24:14 +0400 Subject: [PATCH 1/2] Adding support for .bzl file operations Signed-off-by: Vladimir Suvorov Adding support for .bzl file operations Signed-off-by: Vladimir Suvorov Adding support for .bzl file operations Signed-off-by: Vladimir Suvorov Adding support for .bzl file operations Signed-off-by: Vladimir Suvorov Fixing comments Signed-off-by: Vladimir Suvorov Fixing comments Signed-off-by: Vladimir Suvorov Fix tests Signed-off-by: Vladimir Suvorov --- build/rule.go | 35 ++++++++++++--------- edit/buildozer.go | 54 ++++++++++++++++++++++++++++----- edit/buildozer_test.go | 61 +++++++++++++++++++++++++++++++++++++ edit/edit.go | 41 +++++++++++++++++++++---- edit/edit_test.go | 69 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 233 insertions(+), 27 deletions(-) diff --git a/build/rule.go b/build/rule.go index b6e10bc10..ead9d6c98 100644 --- a/build/rule.go +++ b/build/rule.go @@ -165,25 +165,32 @@ func (f *File) implicitRuleName() string { // begins with a literal, if the call expression does not conform to either of these forms, an // empty string will be returned func (r *Rule) Kind() string { + if r.Call.X == nil { + return "" + } + // The kind may be a simple identifier (e.g. `go_library`) or a dotted expression + // (e.g. `native.go_library`). Extract the dotted path conservatively. var names []string - expr := r.Call.X - for { - x, ok := expr.(*DotExpr) - if !ok { - break + var walk func(Expr) bool + walk = func(e Expr) bool { + switch e := e.(type) { + case *Ident: + names = append(names, e.Name) + return true + case *DotExpr: + // DotExpr represents `X.Name`. + if !walk(e.X) { + return false + } + names = append(names, e.Name) + return true + default: + return false } - names = append(names, x.Name) - expr = x.X } - x, ok := expr.(*Ident) - if !ok { + if !walk(r.Call.X) { return "" } - names = append(names, x.Name) - // Reverse the elements since the deepest expression contains the leading literal - for l, r := 0, len(names)-1; l < r; l, r = l+1, r-1 { - names[l], names[r] = names[r], names[l] - } return strings.Join(names, ".") } diff --git a/edit/buildozer.go b/edit/buildozer.go index d246bfa65..17eba237d 100644 --- a/edit/buildozer.go +++ b/edit/buildozer.go @@ -86,6 +86,20 @@ type CmdEnvironment struct { // The cmdXXX functions implement the various commands. func cmdAdd(opts *Options, env CmdEnvironment) (*build.File, error) { + // Variable targets are represented by a dummy Rule with a nil Call. + if env.Rule != nil && env.Rule.Call == nil { + varName := env.Rule.ImplicitName + varAssign, ok := env.Vars[varName] + if !ok { + return nil, fmt.Errorf("variable %s is not defined", varName) + } + for _, val := range env.Args[1:] { + strVal := getStringExpr(val, env.Pkg) + varAssign.RHS = AddValueToList(varAssign.RHS, env.Pkg, strVal, true) + } + + return env.File, nil + } attr := env.Args[0] for _, val := range env.Args[1:] { if IsIntList(attr) { @@ -419,6 +433,18 @@ func attrKeysForPattern(rule *build.Rule, pattern string) []string { } func cmdRemove(opts *Options, env CmdEnvironment) (*build.File, error) { + // Variable targets are represented by a dummy Rule with a nil Call. + if env.Rule != nil && env.Rule.Call == nil { + varName := env.Rule.ImplicitName + varAssign, ok := env.Vars[varName] + if !ok { + return nil, fmt.Errorf("variable %s is not defined", varName) + } + for _, val := range env.Args[1:] { + ListDelete(varAssign.RHS, val, env.Pkg) + } + return env.File, nil + } if len(env.Args) == 1 { // Remove the attribute if env.Args[0] == "*" { didDelete := false @@ -962,7 +988,7 @@ var readonlyCommands = map[string]bool{ "print_comment": true, } -func expandTargets(f *build.File, rule string) ([]*build.Rule, error) { +func expandTargets(f *build.File, rule string, vars map[string]*build.AssignExpr) ([]*build.Rule, error) { if r := FindRuleByName(f, rule); r != nil { return []*build.Rule{r}, nil } else if r := FindExportedFile(f, rule); r != nil { @@ -982,6 +1008,12 @@ func expandTargets(f *build.File, rule string) ([]*build.Rule, error) { } else { return f.Rules(kind), nil } + } else if vars != nil { + // When variable editing is enabled, allow targeting global variables directly, + // but only if the variable is actually defined in the file. + if _, ok := vars[rule]; ok { + return []*build.Rule{{ImplicitName: rule}}, nil + } } return nil, fmt.Errorf("rule '%s' not found", rule) } @@ -1132,11 +1164,19 @@ type rewriteResult struct { func getGlobalVariables(exprs []build.Expr) (vars map[string]*build.AssignExpr) { vars = make(map[string]*build.AssignExpr) for _, expr := range exprs { - if as, ok := expr.(*build.AssignExpr); ok { - if lhs, ok := as.LHS.(*build.Ident); ok { - vars[lhs.Name] = as + build.Walk(expr, func(x build.Expr, stk []build.Expr) { + //Skip variables defined inside functions + for _, frame := range stk { + if _, ok := frame.(*build.DefStmt); ok { + return + } } - } + if as, ok := x.(*build.AssignExpr); ok { + if lhs, ok := as.LHS.(*build.Ident); ok { + vars[lhs.Name] = as + } + } + }) } return vars } @@ -1238,7 +1278,7 @@ func rewrite(opts *Options, commandsForFile commandsForFile) *rewriteResult { absPkg = f.Pkg } - targets, err := expandTargets(f, rule) + targets, err := expandTargets(f, rule, vars) if err != nil { cerr := commandError(cft.commands, cft.target, err) errs = append(errs, cerr) @@ -1691,7 +1731,7 @@ func ExecuteCommandsOnInlineFile(fileContent []byte, commands []string) ([]byte, f.Type = build.TypeBuild } for _, cft := range commandsByTargetName { - rules, err := expandTargets(f, cft.target) + rules, err := expandTargets(f, cft.target, nil) if err != nil { return nil, err } diff --git a/edit/buildozer_test.go b/edit/buildozer_test.go index 5b1780f8d..fa6290358 100644 --- a/edit/buildozer_test.go +++ b/edit/buildozer_test.go @@ -859,6 +859,67 @@ func TestCmdDictOperations(t *testing.T) { } } +func TestCmdAddRemove_GlobalVariableInBzl(t *testing.T) { + input := `my_patches = ["a.patch"] + +def _fn(): + my_patches = ["function_local.patch"] +` + f, err := build.ParseBzl("defs.bzl", []byte(input)) + if err != nil { + t.Fatal(err) + } + vars := getGlobalVariables(f.Stmt) + if _, ok := vars["my_patches"]; !ok { + t.Fatalf("expected my_patches to be detected as a global variable") + } + // Ensure we don't treat function-local assignments as globals. + if _, ok := vars["_fn"]; ok { + t.Fatalf("unexpected: function name should not be treated as a variable") + } + + // Variable targets are represented by a dummy Rule with nil Call. + varTarget := &build.Rule{ImplicitName: "my_patches"} + + // Add b.patch to the variable list. + _, err = cmdAdd(NewOpts(), CmdEnvironment{ + File: f, + Rule: varTarget, + Vars: vars, + Pkg: "", + Args: []string{"patches", "b.patch"}, + }) + if err != nil { + t.Fatal(err) + } + + // Remove a.patch from the variable list. + _, err = cmdRemove(NewOpts(), CmdEnvironment{ + File: f, + Rule: varTarget, + Vars: vars, + Pkg: "", + Args: []string{"patches", "a.patch"}, + }) + if err != nil { + t.Fatal(err) + } + + got := strings.TrimSpace(string(build.Format(f))) + expected := `my_patches = ["b.patch"] + +def _fn(): + my_patches = ["function_local.patch"]` + wantF, err := build.ParseBzl("defs.bzl", []byte(expected)) + if err != nil { + t.Fatal(err) + } + want := strings.TrimSpace(string(build.Format(wantF))) + if got != want { + t.Errorf("global variable edit in .bzl:\n got: %s\nwant: %s", got, want) + } +} + func TestCmdSetSelect(t *testing.T) { for i, tc := range []struct { name string diff --git a/edit/edit.go b/edit/edit.go index 5bc91641f..46a2f14f5 100644 --- a/edit/edit.go +++ b/edit/edit.go @@ -38,6 +38,23 @@ var ( DeleteWithComments = true ) +func resolveBzlTarget(basePath, rule string) (bzlFile string, rulePart string, ok bool) { + if !strings.Contains(rule, ".bzl") { + return "", "", false + } + idx := strings.Index(rule, ".bzl") + bzlPart := rule[:idx+4] + rulePart = rule + if idx+4 < len(rule) && rule[idx+4] == ':' { + rulePart = rule[idx+5:] + } + bzlFile = filepath.Join(basePath, bzlPart) + if wspace.IsRegularFile(bzlFile) { + return bzlFile, rulePart, true + } + return "", "", false +} + // InterpretLabelForWorkspaceLocation returns the name of the BUILD file to // edit, the full package name, and the rule. It takes a workspace-rooted // directory to use. @@ -66,6 +83,11 @@ func InterpretLabelForWorkspaceLocation(root, target string) (buildFile, repo, p pkg = path.Dir(pkg) return } + if bzlFile, rulePart, ok := resolveBzlTarget(pkgPath, rule); ok { + buildFile = bzlFile + rule = rulePart + return + } for _, buildFileName := range BuildFileNames { buildFile = filepath.Join(pkgPath, buildFileName) if wspace.IsRegularFile(buildFile) { @@ -83,11 +105,18 @@ func InterpretLabelForWorkspaceLocation(root, target string) (buildFile, repo, p } found := false - for _, buildFileName := range BuildFileNames { - buildFile = filepath.Join(pkg, buildFileName) - if wspace.IsRegularFile(buildFile) { - found = true - break + if bzlFile, rulePart, ok := resolveBzlTarget(pkg, rule); ok { + buildFile = bzlFile + found = true + rule = rulePart + } + if !found { + for _, buildFileName := range BuildFileNames { + buildFile = filepath.Join(pkg, buildFileName) + if wspace.IsRegularFile(buildFile) { + found = true + break + } } } if !found { @@ -832,7 +861,7 @@ func sortedInsert(list []build.Expr, item build.Expr) []build.Expr { // sorted. For some attributes, it makes sense to try to do a sorted insert // (e.g. deps), even when buildifier will not sort it for conservative reasons. // For a few attributes, sorting will never make sense. -func attributeMustNotBeSorted(rule, attr string) bool { +func attributeMustNotBeSorted(_ string, attr string) bool { // TODO(bazel-team): Come up with a more complete list. return attr == "args" } diff --git a/edit/edit_test.go b/edit/edit_test.go index 762f42bf8..ff6c5882a 100644 --- a/edit/edit_test.go +++ b/edit/edit_test.go @@ -26,6 +26,7 @@ import ( "testing" "github.com/bazelbuild/buildtools/build" + "github.com/bazelbuild/buildtools/wspace" ) var parseLabelTests = []struct { @@ -830,6 +831,74 @@ func TestInterpretLabelForWorkspaceLocation(t *testing.T) { runTestInterpretLabelForWorkspaceLocation(t, "BUILD.bazel") } +func TestInterpretLabelForWorkspaceLocation_BzlFile(t *testing.T) { + // Create the test workspace under the current directory to avoid relying on + // system temp locations that may be unavailable in some sandboxes. + tmp, err := os.MkdirTemp(".", "edit_test_workspace_") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmp) + if err := os.MkdirAll(filepath.Join(tmp, "a"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(tmp, "WORKSPACE"), []byte("# test workspace\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(tmp, "a", "defs.bzl"), []byte(`http_archive(name = "r")`), 0644); err != nil { + t.Fatal(err) + } + if !wspace.IsRegularFile(filepath.Join(tmp, "a", "defs.bzl")) { + t.Fatalf("test setup failed: expected %q to be a regular file", filepath.Join(tmp, "a", "defs.bzl")) + } + + // Allow selecting a .bzl file using the "target" part of the label. + buildFile, _, pkg, rule := InterpretLabelForWorkspaceLocation(tmp, "//a:defs.bzl") + if buildFile != filepath.Join(tmp, "a", "defs.bzl") || pkg != "a" || rule != "defs.bzl" { + t.Fatalf("InterpretLabelForWorkspaceLocation(%q, %q) = %q, %q, %q; want %q, %q, %q", + tmp, "//a:defs.bzl", buildFile, pkg, rule, filepath.Join(tmp, "a", "defs.bzl"), "a", "defs.bzl") + } + + // Also allow addressing a "rule name" inside the .bzl label for buildozer-style edits. + buildFile, _, pkg, rule = InterpretLabelForWorkspaceLocation(tmp, "//a:defs.bzl:r") + if buildFile != filepath.Join(tmp, "a", "defs.bzl") || pkg != "a" || rule != "r" { + t.Fatalf("InterpretLabelForWorkspaceLocation(%q, %q) = %q, %q, %q; want %q, %q, %q", + tmp, "//a:defs.bzl:r", buildFile, pkg, rule, filepath.Join(tmp, "a", "defs.bzl"), "a", "r") + } +} + +func TestAddAndRemovePatchesInBzlFile(t *testing.T) { + input := `http_archive( + name = "r", + patches = ["a.patch"], +)` + bld, err := build.ParseBzl("defs.bzl", []byte(input)) + if err != nil { + t.Fatal(err) + } + rule := bld.RuleAt(1) + if rule == nil { + t.Fatalf("expected to find first rule in parsed .bzl file") + } + + AddValueToListAttribute(rule, "patches", "", &build.StringExpr{Value: "b.patch"}, nil) + ListAttributeDelete(rule, "patches", "a.patch", "") + + got := strings.TrimSpace(string(build.Format(bld))) + expected := `http_archive( + name = "r", + patches = ["b.patch"], +)` + wantBld, err := build.ParseBzl("defs.bzl", []byte(expected)) + if err != nil { + t.Fatal(err) + } + want := strings.TrimSpace(string(build.Format(wantBld))) + if got != want { + t.Errorf("patch list edit in .bzl:\n got: %s\nwant: %s", got, want) + } +} + func TestFindRuleByName(t *testing.T) { input := `load("foo.bzl", "bar") my_rule( From bb44e4ccdb199b8141278513676c9dae44cca21d Mon Sep 17 00:00:00 2001 From: Vladimir Suvorov Date: Fri, 8 May 2026 23:16:40 +0400 Subject: [PATCH 2/2] extention and fixes Signed-off-by: Vladimir Suvorov --- edit/buildozer.go | 85 ++++++++++++++++++++++++++++++++++++++++++ edit/buildozer_test.go | 52 ++++++++++++++++++++++++++ 2 files changed, 137 insertions(+) diff --git a/edit/buildozer.go b/edit/buildozer.go index 17eba237d..e837a44ad 100644 --- a/edit/buildozer.go +++ b/edit/buildozer.go @@ -225,6 +225,23 @@ func cmdPrintComment(opts *Options, env CmdEnvironment) (*build.File, error) { } func cmdDelete(opts *Options, env CmdEnvironment) (*build.File, error) { + // Variable targets are represented by a dummy Rule with a nil Call. + if env.Rule != nil && env.Rule.Call == nil { + varName := env.Rule.ImplicitName + varAssign, ok := env.Vars[varName] + if !ok { + return nil, fmt.Errorf("variable %s is not defined", varName) + } + var all []build.Expr + for _, stmt := range env.File.Stmt { + if stmt == varAssign { + continue + } + all = append(all, stmt) + } + env.File.Stmt = all + return env.File, nil + } return DeleteRule(env.File, env.Rule), nil } @@ -345,6 +362,47 @@ func cmdSubstituteLoad(opts *Options, env CmdEnvironment) (*build.File, error) { } func cmdPrint(opts *Options, env CmdEnvironment) (*build.File, error) { + // Variable targets are represented by a dummy Rule with a nil Call. + if env.Rule != nil && env.Rule.Call == nil { + varName := env.Rule.ImplicitName + varAssign, ok := env.Vars[varName] + if !ok { + return nil, fmt.Errorf("variable %s is not defined", varName) + } + format := env.Args + if len(format) == 0 { + format = []string{"value"} + } + fields := make([]*apipb.Output_Record_Field, len(format)) + for i, str := range format { + switch str { + case "name": + fields[i] = &apipb.Output_Record_Field{Value: &apipb.Output_Record_Field_Text{Text: varName}} + case "kind": + fields[i] = &apipb.Output_Record_Field{Value: &apipb.Output_Record_Field_Text{Text: "var"}} + case "label": + label := labels.Label{Package: env.Pkg, Target: varName} + fields[i] = &apipb.Output_Record_Field{Value: &apipb.Output_Record_Field_Text{Text: label.Format()}} + case "path": + fields[i] = &apipb.Output_Record_Field{Value: &apipb.Output_Record_Field_Text{Text: env.File.Path}} + case "startline": + start, _ := varAssign.Span() + fields[i] = &apipb.Output_Record_Field{Value: &apipb.Output_Record_Field_Number{Number: int32(start.Line)}} + case "endline": + _, end := varAssign.Span() + fields[i] = &apipb.Output_Record_Field{Value: &apipb.Output_Record_Field_Number{Number: int32(end.Line)}} + case "rule": + fields[i] = &apipb.Output_Record_Field{Value: &apipb.Output_Record_Field_Text{Text: build.FormatString(varAssign)}} + case "value": + fields[i] = &apipb.Output_Record_Field{Value: &apipb.Output_Record_Field_Text{Text: build.FormatString(varAssign.RHS)}} + default: + fields[i] = &apipb.Output_Record_Field{Value: &apipb.Output_Record_Field_Error{Error: apipb.Output_Record_Field_MISSING}} + } + } + env.output.Fields = fields + return nil, nil + } + format := env.Args if len(format) == 0 { format = []string{"name", "kind"} @@ -586,6 +644,16 @@ func cmdSubstitute(opts *Options, env CmdEnvironment) (*build.File, error) { func cmdSet(opts *Options, env CmdEnvironment) (*build.File, error) { attr := env.Args[0] args := env.Args[1:] + // Variable targets are represented by a dummy Rule with a nil Call. + if env.Rule != nil && env.Rule.Call == nil { + varName := env.Rule.ImplicitName + varAssign, ok := env.Vars[varName] + if !ok { + return nil, fmt.Errorf("variable %s is not defined", varName) + } + varAssign.RHS = getAttrValueExpr(attr, args, env) + return env.File, nil + } if attr == "kind" { env.Rule.SetKind(args[0]) } else { @@ -1338,6 +1406,7 @@ func executeCommandsInFile( ) (*build.File, error) { changed := false for _, cmd := range cft.commands { + cmdName := cmd.tokens[0] cmdInfo := AllCommands[cmd.tokens[0]] // Depending on whether a transformation is rule-specific or not, it should be applied to // every rule that satisfies the filter or just once to the file. @@ -1346,6 +1415,22 @@ func executeCommandsInFile( cmdTargets = []*build.Rule{nil} } for _, r := range cmdTargets { + // Variable targets are represented by a dummy Rule with a nil Call. + // Most rule-specific commands assume a non-nil Call, so guard centrally to avoid panics. + if r != nil && r.Call == nil { + switch cmdName { + case "add", "remove", "set", "print", "delete": + // Supported variable-target commands. + default: + err := fmt.Errorf("command %q does not support variable targets", cmdName) + cerr := commandError([]command{cmd}, cft.target, err) + if opts.KeepGoing { + *errs = append(*errs, cerr) + continue + } + return nil, cerr + } + } record := &apipb.Output_Record{} newf, err := cmdInfo.Fn(opts, CmdEnvironment{f, r, vars, absPkg, cmd.tokens[1:], record}) if len(record.Fields) != 0 { diff --git a/edit/buildozer_test.go b/edit/buildozer_test.go index fa6290358..99890d7dd 100644 --- a/edit/buildozer_test.go +++ b/edit/buildozer_test.go @@ -24,6 +24,7 @@ import ( "strings" "testing" + apipb "github.com/bazelbuild/buildtools/api_proto" "github.com/bazelbuild/buildtools/build" "github.com/google/go-cmp/cmp" ) @@ -920,6 +921,57 @@ def _fn(): } } +func TestExecuteCommandsInFile_RejectsUnsupportedVariableTargetCommands(t *testing.T) { + f, err := build.ParseBzl("defs.bzl", []byte(`v = ["a"]`)) + if err != nil { + t.Fatal(err) + } + vars := getGlobalVariables(f.Stmt) + varTarget := &build.Rule{ImplicitName: "v"} // Call == nil => variable target + + cft := commandsForTarget{ + target: "//pkg:v", + commands: []command{ + {tokens: []string{"rename", "a", "b"}}, + }, + } + records := []*apipb.Output_Record{} + errs := []error{} + _, execErr := executeCommandsInFile(NewOpts(), f, cft, []*build.Rule{varTarget}, &records, vars, "", &errs) + if execErr == nil { + t.Fatalf("expected error when running unsupported command on variable target") + } + if !strings.Contains(execErr.Error(), "does not support variable targets") { + t.Fatalf("unexpected error: %v", execErr) + } +} + +func TestExecuteCommandsInFile_AllowsSetOnVariableTarget(t *testing.T) { + f, err := build.ParseBzl("defs.bzl", []byte(`v = ["a"]`)) + if err != nil { + t.Fatal(err) + } + vars := getGlobalVariables(f.Stmt) + varTarget := &build.Rule{ImplicitName: "v"} // Call == nil => variable target + + cft := commandsForTarget{ + target: "//pkg:v", + commands: []command{ + {tokens: []string{"set", "deps", "b", "c"}}, + }, + } + records := []*apipb.Output_Record{} + errs := []error{} + _, execErr := executeCommandsInFile(NewOpts(), f, cft, []*build.Rule{varTarget}, &records, vars, "", &errs) + if execErr != nil { + t.Fatalf("unexpected error: %v", execErr) + } + got := strings.TrimSpace(string(build.Format(f))) + if got != `v = ["b", "c"]` { + t.Fatalf("got:\n%s\nwant:\n%s", got, `v = ["b", "c"]`) + } +} + func TestCmdSetSelect(t *testing.T) { for i, tc := range []struct { name string