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/buildozer/README.md b/buildozer/README.md index 82a93942b..4e05239c7 100644 --- a/buildozer/README.md +++ b/buildozer/README.md @@ -66,6 +66,26 @@ macros. input instead of from a local file in the package directory: `-:all_tests`. (It is presumably not useful to both use a `-` package name and use the `-f -` flag to read commands from the standard input.) + * In `.bzl` files, use a label to refer to a top-level global variable: + `//pkg:my_patches` (see [Global variables in .bzl files](#global-variables-in-bzl-files)). + +### Global variables in .bzl files + +Buildozer can edit top-level global variables in `.bzl` files. Target the +variable by name, for example `//pkg:my_patches`, and use `add`, `remove`, `set`, +`print`, or `delete`. Other commands return an error on variable targets. + +Only simple assignments of the form `name = value` at module scope are +supported. Variables assigned via tuple unpacking (e.g. `(A, B) = ...`) or `for` +loop variables are not detected. Assignments inside `def` bodies are ignored. + +Example: + +```bash +# In defs.bzl: my_patches = ["a.patch"] +buildozer 'add patches b.patch' //pkg:my_patches +buildozer 'remove patches a.patch' //pkg:my_patches +``` ### Options @@ -85,7 +105,7 @@ See `buildozer -help` for the full list. ### Edit commands Buildozer supports the following commands(`'command args'`): - + * `add [:] `: Adds value(s) to a list attribute of a rule. If a value is already present in the list, it is not added. `type` specifies the type of values being added. See [supported types](#supported-types) for @@ -253,7 +273,7 @@ buildozer 'new cc_binary new_bin before tests' //:__pkg__ # Copy an attribute from `protolib` to `py_protolib`. buildozer 'copy testonly protolib' //pkg:py_protolib -# Set two attributes in the same rule +# Set two attributes in the same rule buildozer 'set compile 1' 'set srcmap 1' //pkg:rule # Make a default explicit in all soy_js rules in a package diff --git a/edit/buildozer.go b/edit/buildozer.go index ac7173f12..a3c6dd9c4 100644 --- a/edit/buildozer.go +++ b/edit/buildozer.go @@ -115,6 +115,20 @@ func parseAttr(attrName string) (attr string, attrType AttrType, err error) { // 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 := getLabelStringExpr(val, env.Pkg) + varAssign.RHS = AddValueToList(varAssign.RHS, env.Pkg, strVal, true) + } + + return env.File, nil + } attr, attrType, err := parseAttr(env.Args[0]) if err != nil { return nil, err @@ -245,6 +259,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 } @@ -365,6 +396,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"} @@ -453,6 +525,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 @@ -597,6 +681,16 @@ func cmdSet(opts *Options, env CmdEnvironment) (*build.File, error) { return nil, err } 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, attrType, args, env) + return env.File, nil + } if attr == "kind" { env.Rule.SetKind(args[0]) } else { @@ -1020,7 +1114,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 { @@ -1040,6 +1134,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) } @@ -1190,11 +1290,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 } @@ -1296,7 +1404,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) @@ -1356,6 +1464,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. @@ -1364,6 +1473,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 { @@ -1749,7 +1874,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..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" ) @@ -859,6 +860,118 @@ 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 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 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(