From 9e6fed26cf368fe9a1fb24bb24579f777ced09c0 Mon Sep 17 00:00:00 2001 From: jamy Date: Wed, 19 Jan 2022 21:34:33 +0000 Subject: [PATCH 1/8] tools/upgradedep: add support for Label based repositories.bzl --- go/tools/releaser/BUILD.bazel | 10 +- go/tools/releaser/repositories_test.bzl | 51 ++++++++++ go/tools/releaser/upgradedep.go | 54 +++++++++-- go/tools/releaser/upgradedep_test.go | 118 ++++++++++++++++++++++++ 4 files changed, 225 insertions(+), 8 deletions(-) create mode 100644 go/tools/releaser/repositories_test.bzl create mode 100644 go/tools/releaser/upgradedep_test.go diff --git a/go/tools/releaser/BUILD.bazel b/go/tools/releaser/BUILD.bazel index 28a344db72..449f80693b 100644 --- a/go/tools/releaser/BUILD.bazel +++ b/go/tools/releaser/BUILD.bazel @@ -1,4 +1,4 @@ -load("//go:def.bzl", "go_binary", "go_library") +load("//go:def.bzl", "go_binary", "go_library", "go_test") go_binary( name = "releaser", @@ -28,3 +28,11 @@ go_library( "@org_golang_x_sync//errgroup", ], ) + +go_test( + name = "releaser_test", + srcs = ["upgradedep_test.go"], + data = ["repositories_test.bzl"], + embed = [":releaser_lib"], + deps = ["@com_github_bazelbuild_buildtools//build:go_default_library"], +) diff --git a/go/tools/releaser/repositories_test.bzl b/go/tools/releaser/repositories_test.bzl new file mode 100644 index 0000000000..b76f9a4613 --- /dev/null +++ b/go/tools/releaser/repositories_test.bzl @@ -0,0 +1,51 @@ +# Copyright 2014 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Once nested repositories work, this file should cease to exist. + +def go_rules_dependencies(): + # releaser:upgrade-dep golang tools + repo_rule( + expected = [ + "//third_party:org_golang_x_tools-gazelle.patch", + "//third_party:org_golang_x_tools-gazelle.patch", + "//third_party:org_golang_x_tools-gazelle.patch", + "@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch", + "invalid patch function: NotLabel", + "invalid patch function: NotLabel", + "not all patches are string literals or Label()", + "Label expr should have 1 argument, found 2", + "Label expr does not contain a string literal", + ], + patches = [ + # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + Label("//third_party:org_golang_x_tools-gazelle.patch"), + # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + "@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch", + # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + "//third_party:org_golang_x_tools-gazelle.patch", + # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + Label("@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch"), + # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + NotLabel("//third_party:org_golang_x_tools-gazelle.patch"), + # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + NotLabel(True), + # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + True, + # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + Label("//third_party:org_golang_x_tools-gazelle.patch", True), + # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + Label(True), + ], + ) diff --git a/go/tools/releaser/upgradedep.go b/go/tools/releaser/upgradedep.go index 03548dbd4e..644ff06de6 100644 --- a/go/tools/releaser/upgradedep.go +++ b/go/tools/releaser/upgradedep.go @@ -433,19 +433,20 @@ func upgradeDepDecl(ctx context.Context, gh *githubClient, workDir, name string, return fmt.Errorf("\"patches\" attribute is not a list") } for patchIndex, patchLabelExpr := range patchesList.List { - patchLabel, ok := patchLabelExpr.(*bzl.StringExpr) - if !ok { - return fmt.Errorf("not all patches are string literals") + patchLabelValue, comments, err := parsePatchesItem(patchLabelExpr) + if err != nil { + return err } - if !strings.HasPrefix(patchLabel.Value, "@io_bazel_rules_go//third_party:") { - return fmt.Errorf("patch does not start with '@io_bazel_rules_go//third_party:': %s", patchLabel) + + if !strings.HasPrefix(patchLabelValue, "//third_party:") { + return fmt.Errorf("patch does not start with '//third_party:': %s", patchLabelValue) } - patchName := patchLabel.Value[len("@io_bazel_rules_go//third_party:"):] + patchName := patchLabelValue[len("//third_party:"):] patchPath := filepath.Join(rootDir, "third_party", patchName) prevDir := filepath.Join(workDir, name, string('a'+patchIndex)) patchDir := filepath.Join(workDir, name, string('a'+patchIndex+1)) var patchCmd []string - for _, c := range patchLabel.Comment().Before { + for _, c := range comments.Before { words := strings.Fields(strings.TrimPrefix(c.Token, "#")) if len(words) > 0 && words[0] == "releaser:patch-cmd" { patchCmd = words[1:] @@ -488,6 +489,45 @@ func upgradeDepDecl(ctx context.Context, gh *githubClient, workDir, name string, return nil } +func parsePatchesItem(patchLabelExpr bzl.Expr) (value string, comments *bzl.Comments, err error) { + // Parse as regular call + patchLabelCall, ok := patchLabelExpr.(*bzl.CallExpr) + + if ok { + // Verify the identifier, should be Label + ident, ok := patchLabelCall.X.(*bzl.Ident) + if !ok { + return "", nil, fmt.Errorf("invalid identifier while parsing patch label") + } + if ident.Name != "Label" { + return "", nil, fmt.Errorf("invalid patch function: %s", ident.Name) + } + + // Expect 1 String argument with the patch + if len(patchLabelCall.List) != 1 { + return "", nil, fmt.Errorf("Label expr should have 1 argument, found %d", len(patchLabelCall.List)) + } + + // Parse patch as a string + patchLabelStr, ok := patchLabelCall.List[0].(*bzl.StringExpr) + if !ok { + return "", nil, fmt.Errorf("Label expr does not contain a string literal") + } + return patchLabelStr.Value, patchLabelCall.Comment(), nil + } + + // non-label string literal + patchLabelStr, ok := patchLabelExpr.(*bzl.StringExpr) + if !ok { + return "", nil, fmt.Errorf("not all patches are string literals or Label()") + } + + if strings.HasPrefix(patchLabelStr.Value, "@io_bazel_rules_go") { + return patchLabelStr.Value[len("@io_bazel_rules_go"):], patchLabelStr.Comment(), nil + } + return patchLabelStr.Value, patchLabelStr.Comment(), nil +} + // parseUpgradeDepDirective parses a '# releaser:upgrade-dep org repo' directive // and returns the organization and repository name or an error if the directive // was not found or malformed. diff --git a/go/tools/releaser/upgradedep_test.go b/go/tools/releaser/upgradedep_test.go new file mode 100644 index 0000000000..d30b171e9c --- /dev/null +++ b/go/tools/releaser/upgradedep_test.go @@ -0,0 +1,118 @@ +package main + +import ( + "fmt" + "os" + "testing" + + bzl "github.com/bazelbuild/buildtools/build" +) + +const ( + repos = "repositories_test.bzl" + funcName = "go_rules_dependencies" +) + +func TestPatchItemParser(t *testing.T) { + rules, err := loadRepositoriesFile(repos) + if err != nil { + t.Fatal(err) + } + // Loop over the repo rules + for _, expr := range rules { + patches, expected, err := parseRepoRule(expr) + if err != nil { + t.Fatal(err) + } + // parse each of the patch items, check against expected result + for i, patchLabelExpr := range patches.List { + patchLabelStr, _, err := parsePatchesItem(patchLabelExpr) + if err != nil && err.Error() != expected[i] { + t.Fatalf("Patch index %d: expected %s but got error: %s", i, expected[i], err.Error()) + } + if err == nil && patchLabelStr != expected[i] { + t.Fatalf("Patch index %d: expected %s but got: %s", i, expected[i], patchLabelStr) + } + } + } +} + +// loads the repository file and parses out the body +func loadRepositoriesFile(filename string) (body []bzl.Expr, err error) { + data, err := os.ReadFile(filename) + if err != nil { + return nil, fmt.Errorf("could not open %s\n", filename) + } + + parsed, err := bzl.Parse(filename, data) + if err != nil { + return nil, fmt.Errorf("failed to parse: %s\n", err) + } + + // Parse out go_rules_dependencies + for _, expr := range parsed.Stmt { + def, ok := expr.(*bzl.DefStmt) + if !ok { + continue + } + if def.Name == funcName { + body = def.Body + break + } + } + + return +} + +// parses an individual repo rule +func parseRepoRule(expr bzl.Expr) (patches *bzl.ListExpr, expected []string, err error) { + // Check if repo rule is a function call + call, ok := expr.(*bzl.CallExpr) + if !ok { + return nil, nil, fmt.Errorf("repo_rule is not a CallExpr") + } + + expected = make([]string, 0, 5) + + // Loop over the KV pairs in the repo_rule to parse patches and expected results + for _, arg := range call.List { + kwarg, ok := arg.(*bzl.AssignExpr) + if !ok { + continue + } + + key, ok := kwarg.LHS.(*bzl.Ident) // required by parser + if !ok { + continue + } + // Fetch each of the expected items and store them + if key.Name == "expected" { + value, ok := kwarg.RHS.(*bzl.ListExpr) + if !ok { + return nil, nil, fmt.Errorf("expected value does not contain a List at line %d\n", kwarg.OpPos.Line) + } + + for _, val := range value.List { + str, ok := val.(*bzl.StringExpr) + + if !ok { + return nil, nil, fmt.Errorf("invalid expected results at line %d\n", kwarg.OpPos.Line) + } + + expected = append(expected, str.Value) + } + } + + // Parse the patches list + if key.Name == "patches" { + value, ok := kwarg.RHS.(*bzl.ListExpr) + if !ok { + return nil, nil, fmt.Errorf("patches do not contain a List at line %d\n", kwarg.OpPos.Line) + } + + patches = value + } + } + + return +} From 26e563f21bf94c4f0a59c6f523ec662ff1edfcce Mon Sep 17 00:00:00 2001 From: jamy Date: Wed, 19 Jan 2022 22:18:04 +0000 Subject: [PATCH 2/8] upgradedep: switch to ioutil, move test data --- go/tools/releaser/BUILD.bazel | 4 +++- .../repositories.bzl} | 0 go/tools/releaser/upgradedep_test.go | 10 +++++----- 3 files changed, 8 insertions(+), 6 deletions(-) rename go/tools/releaser/{repositories_test.bzl => testdata/repositories.bzl} (100%) diff --git a/go/tools/releaser/BUILD.bazel b/go/tools/releaser/BUILD.bazel index 449f80693b..0d7610a08a 100644 --- a/go/tools/releaser/BUILD.bazel +++ b/go/tools/releaser/BUILD.bazel @@ -32,7 +32,9 @@ go_library( go_test( name = "releaser_test", srcs = ["upgradedep_test.go"], - data = ["repositories_test.bzl"], + data = glob([ + "testdata/*", + ]), embed = [":releaser_lib"], deps = ["@com_github_bazelbuild_buildtools//build:go_default_library"], ) diff --git a/go/tools/releaser/repositories_test.bzl b/go/tools/releaser/testdata/repositories.bzl similarity index 100% rename from go/tools/releaser/repositories_test.bzl rename to go/tools/releaser/testdata/repositories.bzl diff --git a/go/tools/releaser/upgradedep_test.go b/go/tools/releaser/upgradedep_test.go index d30b171e9c..c110ef7aa7 100644 --- a/go/tools/releaser/upgradedep_test.go +++ b/go/tools/releaser/upgradedep_test.go @@ -2,19 +2,19 @@ package main import ( "fmt" - "os" + "io/ioutil" "testing" bzl "github.com/bazelbuild/buildtools/build" ) const ( - repos = "repositories_test.bzl" - funcName = "go_rules_dependencies" + reposFile = "testdata/repositories.bzl" + funcName = "go_rules_dependencies" ) func TestPatchItemParser(t *testing.T) { - rules, err := loadRepositoriesFile(repos) + rules, err := loadRepositoriesFile(reposFile) if err != nil { t.Fatal(err) } @@ -39,7 +39,7 @@ func TestPatchItemParser(t *testing.T) { // loads the repository file and parses out the body func loadRepositoriesFile(filename string) (body []bzl.Expr, err error) { - data, err := os.ReadFile(filename) + data, err := ioutil.ReadFile(filename) if err != nil { return nil, fmt.Errorf("could not open %s\n", filename) } From 1e6bd8bde82da928e4cbea2f7cc8d66db1872410 Mon Sep 17 00:00:00 2001 From: jamy Date: Wed, 19 Jan 2022 22:23:16 +0000 Subject: [PATCH 3/8] upgradedep: log actual failure reason --- go/tools/releaser/upgradedep_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/tools/releaser/upgradedep_test.go b/go/tools/releaser/upgradedep_test.go index c110ef7aa7..c0324ec0d9 100644 --- a/go/tools/releaser/upgradedep_test.go +++ b/go/tools/releaser/upgradedep_test.go @@ -41,7 +41,7 @@ func TestPatchItemParser(t *testing.T) { func loadRepositoriesFile(filename string) (body []bzl.Expr, err error) { data, err := ioutil.ReadFile(filename) if err != nil { - return nil, fmt.Errorf("could not open %s\n", filename) + return nil, fmt.Errorf("could not open %s: %s\n", filename, err) } parsed, err := bzl.Parse(filename, data) From 3b86af4649f5c1b7fac1ca24f92bae1e3d2cb94c Mon Sep 17 00:00:00 2001 From: jamy Date: Wed, 19 Jan 2022 22:48:46 +0000 Subject: [PATCH 4/8] upgradedep_test: use .Abs path --- go/tools/releaser/BUILD.bazel | 4 +--- go/tools/releaser/upgradedep_test.go | 9 +++++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/go/tools/releaser/BUILD.bazel b/go/tools/releaser/BUILD.bazel index 0d7610a08a..4a0f6dd861 100644 --- a/go/tools/releaser/BUILD.bazel +++ b/go/tools/releaser/BUILD.bazel @@ -32,9 +32,7 @@ go_library( go_test( name = "releaser_test", srcs = ["upgradedep_test.go"], - data = glob([ - "testdata/*", - ]), + data = glob(["testdata/**"]), embed = [":releaser_lib"], deps = ["@com_github_bazelbuild_buildtools//build:go_default_library"], ) diff --git a/go/tools/releaser/upgradedep_test.go b/go/tools/releaser/upgradedep_test.go index c0324ec0d9..27ba0ec685 100644 --- a/go/tools/releaser/upgradedep_test.go +++ b/go/tools/releaser/upgradedep_test.go @@ -3,6 +3,7 @@ package main import ( "fmt" "io/ioutil" + "path/filepath" "testing" bzl "github.com/bazelbuild/buildtools/build" @@ -39,9 +40,13 @@ func TestPatchItemParser(t *testing.T) { // loads the repository file and parses out the body func loadRepositoriesFile(filename string) (body []bzl.Expr, err error) { - data, err := ioutil.ReadFile(filename) + filepath, err := filepath.Abs(filename) if err != nil { - return nil, fmt.Errorf("could not open %s: %s\n", filename, err) + return nil, fmt.Errorf("loadRepositoriesFile: %s\n", err) + } + data, err := ioutil.ReadFile(filepath) + if err != nil { + return nil, fmt.Errorf("loadRepositoriesFile: %s\n", err) } parsed, err := bzl.Parse(filename, data) From fbc50587773590ec151c2f23b78f857ad5765d04 Mon Sep 17 00:00:00 2001 From: jamy Date: Wed, 19 Jan 2022 23:31:41 +0000 Subject: [PATCH 5/8] upgradedep_test: use bazel.Runfile --- go/tools/releaser/BUILD.bazel | 5 ++++- go/tools/releaser/upgradedep_test.go | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/go/tools/releaser/BUILD.bazel b/go/tools/releaser/BUILD.bazel index 4a0f6dd861..7a8e307c1c 100644 --- a/go/tools/releaser/BUILD.bazel +++ b/go/tools/releaser/BUILD.bazel @@ -34,5 +34,8 @@ go_test( srcs = ["upgradedep_test.go"], data = glob(["testdata/**"]), embed = [":releaser_lib"], - deps = ["@com_github_bazelbuild_buildtools//build:go_default_library"], + deps = [ + "//go/tools/bazel", + "@com_github_bazelbuild_buildtools//build:go_default_library", + ], ) diff --git a/go/tools/releaser/upgradedep_test.go b/go/tools/releaser/upgradedep_test.go index 27ba0ec685..d60901514b 100644 --- a/go/tools/releaser/upgradedep_test.go +++ b/go/tools/releaser/upgradedep_test.go @@ -3,14 +3,14 @@ package main import ( "fmt" "io/ioutil" - "path/filepath" "testing" bzl "github.com/bazelbuild/buildtools/build" + "github.com/bazelbuild/rules_go/go/tools/bazel" ) const ( - reposFile = "testdata/repositories.bzl" + reposFile = "go/tools/releaser/testdata/repositories.bzl" funcName = "go_rules_dependencies" ) @@ -40,7 +40,7 @@ func TestPatchItemParser(t *testing.T) { // loads the repository file and parses out the body func loadRepositoriesFile(filename string) (body []bzl.Expr, err error) { - filepath, err := filepath.Abs(filename) + filepath, err := bazel.Runfile(filename) if err != nil { return nil, fmt.Errorf("loadRepositoriesFile: %s\n", err) } From a323aff1fe81f65bbfc9628f7cc7a74a498783c3 Mon Sep 17 00:00:00 2001 From: jamy Date: Thu, 20 Jan 2022 17:54:52 +0000 Subject: [PATCH 6/8] upgradedep_test: change to simpler testing format --- go/tools/releaser/BUILD.bazel | 6 +- go/tools/releaser/testdata/repositories.bzl | 51 ------ go/tools/releaser/upgradedep_test.go | 172 ++++++++------------ 3 files changed, 70 insertions(+), 159 deletions(-) delete mode 100644 go/tools/releaser/testdata/repositories.bzl diff --git a/go/tools/releaser/BUILD.bazel b/go/tools/releaser/BUILD.bazel index 7a8e307c1c..e750c67bd5 100644 --- a/go/tools/releaser/BUILD.bazel +++ b/go/tools/releaser/BUILD.bazel @@ -32,10 +32,6 @@ go_library( go_test( name = "releaser_test", srcs = ["upgradedep_test.go"], - data = glob(["testdata/**"]), embed = [":releaser_lib"], - deps = [ - "//go/tools/bazel", - "@com_github_bazelbuild_buildtools//build:go_default_library", - ], + deps = ["@com_github_bazelbuild_buildtools//build:go_default_library"], ) diff --git a/go/tools/releaser/testdata/repositories.bzl b/go/tools/releaser/testdata/repositories.bzl deleted file mode 100644 index b76f9a4613..0000000000 --- a/go/tools/releaser/testdata/repositories.bzl +++ /dev/null @@ -1,51 +0,0 @@ -# Copyright 2014 The Bazel Authors. All rights reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -# Once nested repositories work, this file should cease to exist. - -def go_rules_dependencies(): - # releaser:upgrade-dep golang tools - repo_rule( - expected = [ - "//third_party:org_golang_x_tools-gazelle.patch", - "//third_party:org_golang_x_tools-gazelle.patch", - "//third_party:org_golang_x_tools-gazelle.patch", - "@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch", - "invalid patch function: NotLabel", - "invalid patch function: NotLabel", - "not all patches are string literals or Label()", - "Label expr should have 1 argument, found 2", - "Label expr does not contain a string literal", - ], - patches = [ - # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias - Label("//third_party:org_golang_x_tools-gazelle.patch"), - # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias - "@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch", - # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias - "//third_party:org_golang_x_tools-gazelle.patch", - # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias - Label("@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch"), - # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias - NotLabel("//third_party:org_golang_x_tools-gazelle.patch"), - # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias - NotLabel(True), - # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias - True, - # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias - Label("//third_party:org_golang_x_tools-gazelle.patch", True), - # releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias - Label(True), - ], - ) diff --git a/go/tools/releaser/upgradedep_test.go b/go/tools/releaser/upgradedep_test.go index d60901514b..b36aa58155 100644 --- a/go/tools/releaser/upgradedep_test.go +++ b/go/tools/releaser/upgradedep_test.go @@ -2,122 +2,88 @@ package main import ( "fmt" - "io/ioutil" "testing" bzl "github.com/bazelbuild/buildtools/build" - "github.com/bazelbuild/rules_go/go/tools/bazel" -) - -const ( - reposFile = "go/tools/releaser/testdata/repositories.bzl" - funcName = "go_rules_dependencies" ) func TestPatchItemParser(t *testing.T) { - rules, err := loadRepositoriesFile(reposFile) - if err != nil { - t.Fatal(err) - } - // Loop over the repo rules - for _, expr := range rules { - patches, expected, err := parseRepoRule(expr) - if err != nil { - t.Fatal(err) - } - // parse each of the patch items, check against expected result - for i, patchLabelExpr := range patches.List { - patchLabelStr, _, err := parsePatchesItem(patchLabelExpr) - if err != nil && err.Error() != expected[i] { - t.Fatalf("Patch index %d: expected %s but got error: %s", i, expected[i], err.Error()) - } - if err == nil && patchLabelStr != expected[i] { - t.Fatalf("Patch index %d: expected %s but got: %s", i, expected[i], patchLabelStr) - } - } - } -} - -// loads the repository file and parses out the body -func loadRepositoriesFile(filename string) (body []bzl.Expr, err error) { - filepath, err := bazel.Runfile(filename) - if err != nil { - return nil, fmt.Errorf("loadRepositoriesFile: %s\n", err) - } - data, err := ioutil.ReadFile(filepath) - if err != nil { - return nil, fmt.Errorf("loadRepositoriesFile: %s\n", err) - } - - parsed, err := bzl.Parse(filename, data) - if err != nil { - return nil, fmt.Errorf("failed to parse: %s\n", err) - } - - // Parse out go_rules_dependencies - for _, expr := range parsed.Stmt { - def, ok := expr.(*bzl.DefStmt) - if !ok { - continue - } - if def.Name == funcName { - body = def.Body - break - } - } - - return -} - -// parses an individual repo rule -func parseRepoRule(expr bzl.Expr) (patches *bzl.ListExpr, expected []string, err error) { - // Check if repo rule is a function call - call, ok := expr.(*bzl.CallExpr) - if !ok { - return nil, nil, fmt.Errorf("repo_rule is not a CallExpr") + tests := []struct { + expression []byte + result string + error string + }{ + { + expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + Label("//third_party:org_golang_x_tools-gazelle.patch")`), + result: "//third_party:org_golang_x_tools-gazelle.patch", + }, + { + expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + "@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch"`), + result: "//third_party:org_golang_x_tools-gazelle.patch", + }, + { + expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + "//third_party:org_golang_x_tools-gazelle.patch"`), + result: "//third_party:org_golang_x_tools-gazelle.patch", + }, + { + expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + Label("@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch")`), + result: "@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch", + }, + { + expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + NotLabel("//third_party:org_golang_x_tools-gazelle.patch")`), + result: "", + error: "invalid patch function: NotLabel", + }, + { + expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + NotLabel(True)`), + error: "invalid patch function: NotLabel", + }, + { + expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + True`), + error: "not all patches are string literals or Label()", + }, + { + expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + Label("//third_party:org_golang_x_tools-gazelle.patch", True)`), + error: "Label expr should have 1 argument, found 2", + }, + { + expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias + Label(True)`), + error: "Label expr does not contain a string literal", + }, } - expected = make([]string, 0, 5) - - // Loop over the KV pairs in the repo_rule to parse patches and expected results - for _, arg := range call.List { - kwarg, ok := arg.(*bzl.AssignExpr) - if !ok { - continue - } - - key, ok := kwarg.LHS.(*bzl.Ident) // required by parser - if !ok { - continue - } - // Fetch each of the expected items and store them - if key.Name == "expected" { - value, ok := kwarg.RHS.(*bzl.ListExpr) - if !ok { - return nil, nil, fmt.Errorf("expected value does not contain a List at line %d\n", kwarg.OpPos.Line) + for _, tt := range tests { + t.Run(fmt.Sprintf("%v", tt.expression), func(t *testing.T) { + patchExpr, err := bzl.Parse("repos.bzl", tt.expression) + if err != nil { + t.Fatalf(err.Error()) } - for _, val := range value.List { - str, ok := val.(*bzl.StringExpr) - - if !ok { - return nil, nil, fmt.Errorf("invalid expected results at line %d\n", kwarg.OpPos.Line) + patchLabelStr, _, err := parsePatchesItem(patchExpr.Stmt[0]) + if err != nil && err.Error() != tt.error { + if tt.error != "" { + t.Errorf("expected error '%s', but got error '%s' instead", tt.error, err.Error()) + } else { + t.Errorf("unexpected error while parsing expression: %s", err.Error()) } - - expected = append(expected, str.Value) } - } - // Parse the patches list - if key.Name == "patches" { - value, ok := kwarg.RHS.(*bzl.ListExpr) - if !ok { - return nil, nil, fmt.Errorf("patches do not contain a List at line %d\n", kwarg.OpPos.Line) + if err == nil && patchLabelStr != tt.result { + if tt.error != "" { + t.Errorf("expected error '%s', but got result '%s' instead", tt.error, patchLabelStr) + } else { + t.Errorf("expected result '%s', but got result '%s' instead", tt.result, patchLabelStr) + } } - - patches = value - } + }) } - - return } From 83cc25fe86bfb11a2b590c71d95daad98b05d97c Mon Sep 17 00:00:00 2001 From: jamy Date: Thu, 20 Jan 2022 20:42:51 +0000 Subject: [PATCH 7/8] upgradedep: nit comments from PR --- go/tools/releaser/upgradedep.go | 39 ++++++++++------------------ go/tools/releaser/upgradedep_test.go | 10 +++---- 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/go/tools/releaser/upgradedep.go b/go/tools/releaser/upgradedep.go index 644ff06de6..a0c2871b86 100644 --- a/go/tools/releaser/upgradedep.go +++ b/go/tools/releaser/upgradedep.go @@ -435,11 +435,11 @@ func upgradeDepDecl(ctx context.Context, gh *githubClient, workDir, name string, for patchIndex, patchLabelExpr := range patchesList.List { patchLabelValue, comments, err := parsePatchesItem(patchLabelExpr) if err != nil { - return err + return fmt.Errorf("parsing expr %#v : %w", patchLabelExpr, err) } if !strings.HasPrefix(patchLabelValue, "//third_party:") { - return fmt.Errorf("patch does not start with '//third_party:': %s", patchLabelValue) + return fmt.Errorf("patch does not start with '//third_party:': %q", patchLabelValue) } patchName := patchLabelValue[len("//third_party:"):] patchPath := filepath.Join(rootDir, "third_party", patchName) @@ -490,42 +490,31 @@ func upgradeDepDecl(ctx context.Context, gh *githubClient, workDir, name string, } func parsePatchesItem(patchLabelExpr bzl.Expr) (value string, comments *bzl.Comments, err error) { - // Parse as regular call - patchLabelCall, ok := patchLabelExpr.(*bzl.CallExpr) - - if ok { + switch patchLabel := patchLabelExpr.(type) { + case *bzl.CallExpr: // Verify the identifier, should be Label - ident, ok := patchLabelCall.X.(*bzl.Ident) - if !ok { + if ident, ok := patchLabel.X.(*bzl.Ident); !ok { return "", nil, fmt.Errorf("invalid identifier while parsing patch label") - } - if ident.Name != "Label" { - return "", nil, fmt.Errorf("invalid patch function: %s", ident.Name) + } else if ident.Name != "Label" { + return "", nil, fmt.Errorf("invalid patch function: %q", ident.Name) } // Expect 1 String argument with the patch - if len(patchLabelCall.List) != 1 { - return "", nil, fmt.Errorf("Label expr should have 1 argument, found %d", len(patchLabelCall.List)) + if len(patchLabel.List) != 1 { + return "", nil, fmt.Errorf("Label expr should have 1 argument, found %d", len(patchLabel.List)) } // Parse patch as a string - patchLabelStr, ok := patchLabelCall.List[0].(*bzl.StringExpr) + patchLabelStr, ok := patchLabel.List[0].(*bzl.StringExpr) if !ok { return "", nil, fmt.Errorf("Label expr does not contain a string literal") } - return patchLabelStr.Value, patchLabelCall.Comment(), nil - } - - // non-label string literal - patchLabelStr, ok := patchLabelExpr.(*bzl.StringExpr) - if !ok { + return patchLabelStr.Value, patchLabel.Comment(), nil + case *bzl.StringExpr: + return strings.TrimPrefix(patchLabel.Value, "@io_bazel_rules_go"), patchLabel.Comment(), nil + default: return "", nil, fmt.Errorf("not all patches are string literals or Label()") } - - if strings.HasPrefix(patchLabelStr.Value, "@io_bazel_rules_go") { - return patchLabelStr.Value[len("@io_bazel_rules_go"):], patchLabelStr.Comment(), nil - } - return patchLabelStr.Value, patchLabelStr.Comment(), nil } // parseUpgradeDepDirective parses a '# releaser:upgrade-dep org repo' directive diff --git a/go/tools/releaser/upgradedep_test.go b/go/tools/releaser/upgradedep_test.go index b36aa58155..f2351a22a6 100644 --- a/go/tools/releaser/upgradedep_test.go +++ b/go/tools/releaser/upgradedep_test.go @@ -37,12 +37,12 @@ func TestPatchItemParser(t *testing.T) { expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias NotLabel("//third_party:org_golang_x_tools-gazelle.patch")`), result: "", - error: "invalid patch function: NotLabel", + error: `invalid patch function: "NotLabel"`, }, { expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias NotLabel(True)`), - error: "invalid patch function: NotLabel", + error: `invalid patch function: "NotLabel"`, }, { expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias @@ -71,7 +71,7 @@ func TestPatchItemParser(t *testing.T) { patchLabelStr, _, err := parsePatchesItem(patchExpr.Stmt[0]) if err != nil && err.Error() != tt.error { if tt.error != "" { - t.Errorf("expected error '%s', but got error '%s' instead", tt.error, err.Error()) + t.Errorf("expected error %q, but got error %q instead", tt.error, err.Error()) } else { t.Errorf("unexpected error while parsing expression: %s", err.Error()) } @@ -79,9 +79,9 @@ func TestPatchItemParser(t *testing.T) { if err == nil && patchLabelStr != tt.result { if tt.error != "" { - t.Errorf("expected error '%s', but got result '%s' instead", tt.error, patchLabelStr) + t.Errorf("expected error %q, but got result %q instead", tt.error, patchLabelStr) } else { - t.Errorf("expected result '%s', but got result '%s' instead", tt.result, patchLabelStr) + t.Errorf("expected result %q, but got result %q instead", tt.result, patchLabelStr) } } }) From 777c96103042a51de0999dd2baa4dea49532f563 Mon Sep 17 00:00:00 2001 From: jamy Date: Thu, 20 Jan 2022 21:28:34 +0000 Subject: [PATCH 8/8] fixup! separate tests between success and fail --- go/tools/releaser/upgradedep_test.go | 47 ++++++++++++++++++---------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/go/tools/releaser/upgradedep_test.go b/go/tools/releaser/upgradedep_test.go index f2351a22a6..1371add1e0 100644 --- a/go/tools/releaser/upgradedep_test.go +++ b/go/tools/releaser/upgradedep_test.go @@ -7,11 +7,10 @@ import ( bzl "github.com/bazelbuild/buildtools/build" ) -func TestPatchItemParser(t *testing.T) { +func TestPatchItemParser_Success(t *testing.T) { tests := []struct { expression []byte result string - error string }{ { expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias @@ -33,11 +32,34 @@ func TestPatchItemParser(t *testing.T) { Label("@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch")`), result: "@io_bazel_rules_go//third_party:org_golang_x_tools-gazelle.patch", }, + } + + for _, tt := range tests { + t.Run(fmt.Sprintf("%v", tt.expression), func(t *testing.T) { + patchExpr, err := bzl.Parse("repos.bzl", tt.expression) + if err != nil { + t.Fatalf(err.Error()) + } + + patchLabelStr, _, err := parsePatchesItem(patchExpr.Stmt[0]) + if err != nil { + t.Errorf("unexpected error while parsing expression: %q", err.Error()) + } else if patchLabelStr != tt.result { + t.Errorf("expected result %q, but got result %q instead", tt.result, patchLabelStr) + } + }) + } +} + +func TestPatchItemParser_Error(t *testing.T) { + tests := []struct { + expression []byte + error string + }{ { expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias NotLabel("//third_party:org_golang_x_tools-gazelle.patch")`), - result: "", - error: `invalid patch function: "NotLabel"`, + error: `invalid patch function: "NotLabel"`, }, { expression: []byte(`# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/tools -go_naming_convention import_alias @@ -69,20 +91,11 @@ func TestPatchItemParser(t *testing.T) { } patchLabelStr, _, err := parsePatchesItem(patchExpr.Stmt[0]) - if err != nil && err.Error() != tt.error { - if tt.error != "" { - t.Errorf("expected error %q, but got error %q instead", tt.error, err.Error()) - } else { - t.Errorf("unexpected error while parsing expression: %s", err.Error()) - } - } - if err == nil && patchLabelStr != tt.result { - if tt.error != "" { - t.Errorf("expected error %q, but got result %q instead", tt.error, patchLabelStr) - } else { - t.Errorf("expected result %q, but got result %q instead", tt.result, patchLabelStr) - } + if err == nil { + t.Errorf("expected error %q, but got result %q instead", tt.error, patchLabelStr) + } else if err.Error() != tt.error { + t.Errorf("expected error %q, but got error %q instead", tt.error, err.Error()) } }) }