From 2c178ae5afadd7435bc50a8897dee8202716a336 Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Wed, 29 Nov 2023 12:12:11 -0800 Subject: [PATCH 01/12] Creating one py_binary per main module --- gazelle/python/BUILD.bazel | 12 ++++- gazelle/python/generate.go | 53 +++++++++++++++---- gazelle/python/parse.py | 20 ++++++- gazelle/python/parse_test.py | 29 ++++++++++ gazelle/python/parser.go | 23 +++++--- .../binary_without_entrypoint/BUILD.in | 4 ++ .../binary_without_entrypoint/BUILD.out | 21 ++++++++ .../binary_without_entrypoint/README.md | 4 ++ .../binary_without_entrypoint/WORKSPACE | 1 + .../binary_without_entrypoint/__init__.py | 15 ++++++ .../collided_main.py | 2 + .../binary_without_entrypoint/main.py | 2 + .../binary_without_entrypoint/test.yaml | 18 +++++++ gazelle/pythonconfig/BUILD.bazel | 2 +- 14 files changed, 186 insertions(+), 20 deletions(-) create mode 100644 gazelle/python/parse_test.py create mode 100644 gazelle/python/testdata/binary_without_entrypoint/BUILD.in create mode 100644 gazelle/python/testdata/binary_without_entrypoint/BUILD.out create mode 100644 gazelle/python/testdata/binary_without_entrypoint/README.md create mode 100644 gazelle/python/testdata/binary_without_entrypoint/WORKSPACE create mode 100644 gazelle/python/testdata/binary_without_entrypoint/__init__.py create mode 100644 gazelle/python/testdata/binary_without_entrypoint/collided_main.py create mode 100644 gazelle/python/testdata/binary_without_entrypoint/main.py create mode 100644 gazelle/python/testdata/binary_without_entrypoint/test.yaml diff --git a/gazelle/python/BUILD.bazel b/gazelle/python/BUILD.bazel index e993a14f22..d5b5270827 100644 --- a/gazelle/python/BUILD.bazel +++ b/gazelle/python/BUILD.bazel @@ -1,6 +1,6 @@ load("@bazel_gazelle//:def.bzl", "gazelle_binary") load("@io_bazel_rules_go//go:def.bzl", "go_library") -load("@rules_python//python:defs.bzl", "py_binary") +load("@rules_python//python:defs.bzl", "py_binary", "py_test") load(":gazelle_test.bzl", "gazelle_test") go_library( @@ -17,7 +17,7 @@ go_library( "std_modules.go", "target.go", ], - embedsrcs = [":helper.zip"], + embedsrcs = [":helper.zip"], # keep importpath = "github.com/bazelbuild/rules_python/gazelle/python", visibility = ["//visibility:public"], deps = [ @@ -48,6 +48,14 @@ py_binary( visibility = ["//visibility:public"], ) +py_test( + name = "parse_test", + srcs = [ + "parse.py", + "parse_test.py", + ], +) + filegroup( name = "helper.zip", srcs = [":helper"], diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 25fb194370..4046f8785c 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -89,9 +89,9 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes pyTestFilenames := treeset.NewWith(godsutils.StringComparator) pyFileNames := treeset.NewWith(godsutils.StringComparator) - // hasPyBinary controls whether a py_binary target should be generated for + // hasPyBinaryEntryPointFile controls whether a single py_binary target should be generated for // this package or not. - hasPyBinary := false + hasPyBinaryEntryPointFile := false // hasPyTestEntryPointFile and hasPyTestEntryPointTarget control whether a py_test target should // be generated for this package or not. @@ -106,8 +106,8 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes ext := filepath.Ext(f) if ext == ".py" { pyFileNames.Add(f) - if !hasPyBinary && f == pyBinaryEntrypointFilename { - hasPyBinary = true + if !hasPyBinaryEntryPointFile && f == pyBinaryEntrypointFilename { + hasPyBinaryEntryPointFile = true } else if !hasPyTestEntryPointFile && f == pyTestEntrypointFilename { hasPyTestEntryPointFile = true } else if f == conftestFilename { @@ -219,7 +219,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes collisionErrors := singlylinkedlist.New() appendPyLibrary := func(srcs *treeset.Set, pyLibraryTargetName string) { - deps, err := parser.parse(srcs) + deps, mainModules, err := parser.parse(srcs) if err != nil { log.Fatalf("ERROR: %v\n", err) } @@ -241,6 +241,41 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } } + if !hasPyBinaryEntryPointFile { + // Creating one py_binary target per main module when __main__.py doesn't exist. + for _, filename := range mainModules { + pyBinaryTargetName := strings.TrimSuffix(filepath.Base(filename), ".py") + // Check if a target with the same name we are generating already + // exists, and if it is of a different kind from the one we are + // generating. If so, we have to throw an error since Gazelle won't + // generate it correctly. + collide := false + if args.File != nil { + for _, t := range args.File.Rules { + if t.Name() == pyBinaryTargetName && t.Kind() != actualPyBinaryKind { + fqTarget := label.New("", args.Rel, pyLibraryTargetName) + log.Printf("failed to generate target %q of kind %q: "+ + "a target of kind %q with the same name already exists.", + fqTarget.String(), actualPyBinaryKind, t.Kind()) + collide = true + break + } + } + } + if collide { + continue + } + srcs.Remove(filename) + pyBinary := newTargetBuilder(pyBinaryKind, pyBinaryTargetName, pythonProjectRoot, args.Rel, pyFileNames). + addVisibility(visibility). + addSrc(filename). + addModuleDependencies(deps). + generateImportsAttribute().build() + result.Gen = append(result.Gen, pyBinary) + result.Imports = append(result.Imports, pyBinary.PrivateAttr(config.GazelleImportsKey)) + } + } + pyLibrary := newTargetBuilder(pyLibraryKind, pyLibraryTargetName, pythonProjectRoot, args.Rel, pyFileNames). addVisibility(visibility). addSrcs(srcs). @@ -270,8 +305,8 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes appendPyLibrary(pyLibraryFilenames, cfg.RenderLibraryName(packageName)) } - if hasPyBinary { - deps, err := parser.parseSingle(pyBinaryEntrypointFilename) + if hasPyBinaryEntryPointFile { + deps, _, err := parser.parseSingle(pyBinaryEntrypointFilename) if err != nil { log.Fatalf("ERROR: %v\n", err) } @@ -310,7 +345,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes var conftest *rule.Rule if hasConftestFile { - deps, err := parser.parseSingle(conftestFilename) + deps, _, err := parser.parseSingle(conftestFilename) if err != nil { log.Fatalf("ERROR: %v\n", err) } @@ -346,7 +381,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes var pyTestTargets []*targetBuilder newPyTestTargetBuilder := func(srcs *treeset.Set, pyTestTargetName string) *targetBuilder { - deps, err := parser.parse(srcs) + deps, _, err := parser.parse(srcs) if err != nil { log.Fatalf("ERROR: %v\n", err) } diff --git a/gazelle/python/parse.py b/gazelle/python/parse.py index 6c0ef69598..ddf82f4f18 100644 --- a/gazelle/python/parse.py +++ b/gazelle/python/parse.py @@ -22,7 +22,7 @@ import os import sys from io import BytesIO -from tokenize import COMMENT, tokenize +from tokenize import COMMENT, NAME, OP, STRING, tokenize def parse_import_statements(content, filepath): @@ -59,6 +59,19 @@ def parse_comments(content): return comments +def parse_main(content): + for line in content.splitlines(): + tokens = list(tokenize(BytesIO(line.encode("utf-8")).readline)) + if len(tokens) < 5: + continue + if tokens[1].type == NAME and tokens[1].string == "if" and \ + tokens[2].type == NAME and tokens[2].string == "__name__" and \ + tokens[3].type == OP and tokens[3].string == "==" and \ + tokens[4].type == STRING and tokens[4].string.strip("\"'") == '__main__': + return True + return False + + def parse(repo_root, rel_package_path, filename): rel_filepath = os.path.join(rel_package_path, filename) abs_filepath = os.path.join(repo_root, rel_filepath) @@ -70,11 +83,16 @@ def parse(repo_root, rel_package_path, filename): parse_import_statements, content, rel_filepath ) comments_future = executor.submit(parse_comments, content) + main_future = executor.submit(parse_main, content) modules = modules_future.result() comments = comments_future.result() + has_main = main_future.result() + output = { + "filename": filename, "modules": modules, "comments": comments, + "has_main": has_main, } return output diff --git a/gazelle/python/parse_test.py b/gazelle/python/parse_test.py new file mode 100644 index 0000000000..0af2b36bc8 --- /dev/null +++ b/gazelle/python/parse_test.py @@ -0,0 +1,29 @@ +import unittest +import parse + +class TestParse(unittest.TestCase): + def test_not_has_main(self): + content = "a = 1\nb = 2" + self.assertFalse(parse.parse_main(content)) + + def test_has_main_in_function(self): + content = """ +def foo(): + if __name__ == "__main__": + a = 3 +""" + self.assertFalse(parse.parse_main(content)) + + def test_has_main(self): + content = """ +def main(): + pass + +if __name__ == "__main__": + main() +""" + self.assertTrue(parse.parse_main(content)) + + +if __name__ == "__main__": + unittest.main() diff --git a/gazelle/python/parser.go b/gazelle/python/parser.go index 89310267c3..d22850b6a9 100644 --- a/gazelle/python/parser.go +++ b/gazelle/python/parser.go @@ -101,7 +101,7 @@ func newPython3Parser( // parseSingle parses a single Python file and returns the extracted modules // from the import statements as well as the parsed comments. -func (p *python3Parser) parseSingle(pyFilename string) (*treeset.Set, error) { +func (p *python3Parser) parseSingle(pyFilename string) (*treeset.Set, []string, error) { pyFilenames := treeset.NewWith(godsutils.StringComparator) pyFilenames.Add(pyFilename) return p.parse(pyFilenames) @@ -109,7 +109,7 @@ func (p *python3Parser) parseSingle(pyFilename string) (*treeset.Set, error) { // parse parses multiple Python files and returns the extracted modules from // the import statements as well as the parsed comments. -func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, error) { +func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, []string, error) { parserMutex.Lock() defer parserMutex.Unlock() @@ -122,24 +122,28 @@ func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, error) { } encoder := json.NewEncoder(parserStdin) if err := encoder.Encode(&req); err != nil { - return nil, fmt.Errorf("failed to parse: %w", err) + return nil, nil, fmt.Errorf("failed to parse: %w", err) } reader := bufio.NewReader(parserStdout) data, err := reader.ReadBytes(0) if err != nil { - return nil, fmt.Errorf("failed to parse: %w", err) + return nil, nil, fmt.Errorf("failed to parse: %w", err) } data = data[:len(data)-1] var allRes []parserResponse if err := json.Unmarshal(data, &allRes); err != nil { - return nil, fmt.Errorf("failed to parse: %w", err) + return nil, nil, fmt.Errorf("failed to parse: %w", err) } + var mainModules []string for _, res := range allRes { + if res.HasMain { + mainModules = append(mainModules, res.FileName) + } annotations, err := annotationsFromComments(res.Comments) if err != nil { - return nil, fmt.Errorf("failed to parse annotations: %w", err) + return nil, nil, fmt.Errorf("failed to parse annotations: %w", err) } for _, m := range res.Modules { @@ -159,17 +163,22 @@ func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, error) { } } - return modules, nil + return modules, mainModules, nil } // parserResponse represents a response returned by the parser.py for a given // parsed Python module. type parserResponse struct { + // FileName of the parsed module + FileName string // The modules depended by the parsed module. Modules []module `json:"modules"` // The comments contained in the parsed module. This contains the // annotations as they are comments in the Python module. Comments []comment `json:"comments"` + // HasMain indicates whether the Python module has `if __name == "__main__"` + // at the top level + HasMain bool `json:"has_main"` } // module represents a fully-qualified, dot-separated, Python module as seen on diff --git a/gazelle/python/testdata/binary_without_entrypoint/BUILD.in b/gazelle/python/testdata/binary_without_entrypoint/BUILD.in new file mode 100644 index 0000000000..3ccb32e598 --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/BUILD.in @@ -0,0 +1,4 @@ +filegroup( + name = "collided_main", + srcs = ["collided_main.py"], +) \ No newline at end of file diff --git a/gazelle/python/testdata/binary_without_entrypoint/BUILD.out b/gazelle/python/testdata/binary_without_entrypoint/BUILD.out new file mode 100644 index 0000000000..5fccc2bbf3 --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/BUILD.out @@ -0,0 +1,21 @@ +load("@rules_python//python:defs.bzl", "py_binary", "py_library") + +filegroup( + name = "collided_main", + srcs = ["collided_main.py"], +) + +py_binary( + name = "main", + srcs = ["main.py"], + visibility = ["//:__subpackages__"], +) + +py_library( + name = "binary_without_entrypoint", + srcs = [ + "__init__.py", + "collided_main.py", + ], + visibility = ["//:__subpackages__"], +) \ No newline at end of file diff --git a/gazelle/python/testdata/binary_without_entrypoint/README.md b/gazelle/python/testdata/binary_without_entrypoint/README.md new file mode 100644 index 0000000000..e91250d0ac --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/README.md @@ -0,0 +1,4 @@ +# Binary without entrypoint + +This test case asserts that when there is no __main__.py, a py_binary is generated per main module, unless a main +module main collides with existing target name. diff --git a/gazelle/python/testdata/binary_without_entrypoint/WORKSPACE b/gazelle/python/testdata/binary_without_entrypoint/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/binary_without_entrypoint/__init__.py b/gazelle/python/testdata/binary_without_entrypoint/__init__.py new file mode 100644 index 0000000000..730755995d --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/__init__.py @@ -0,0 +1,15 @@ +# Copyright 2023 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. + +# For test purposes only. diff --git a/gazelle/python/testdata/binary_without_entrypoint/collided_main.py b/gazelle/python/testdata/binary_without_entrypoint/collided_main.py new file mode 100644 index 0000000000..3668fcc445 --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/collided_main.py @@ -0,0 +1,2 @@ +if __name__ == "__main__": + run() \ No newline at end of file diff --git a/gazelle/python/testdata/binary_without_entrypoint/main.py b/gazelle/python/testdata/binary_without_entrypoint/main.py new file mode 100644 index 0000000000..3668fcc445 --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/main.py @@ -0,0 +1,2 @@ +if __name__ == "__main__": + run() \ No newline at end of file diff --git a/gazelle/python/testdata/binary_without_entrypoint/test.yaml b/gazelle/python/testdata/binary_without_entrypoint/test.yaml new file mode 100644 index 0000000000..fbc5a34fcc --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/test.yaml @@ -0,0 +1,18 @@ +# Copyright 2023 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. + +--- +expect: + stderr: | + gazelle: failed to generate target "//:binary_without_entrypoint" of kind "py_binary": a target of kind "filegroup" with the same name already exists. diff --git a/gazelle/pythonconfig/BUILD.bazel b/gazelle/pythonconfig/BUILD.bazel index d0f1690d94..d80902e7ce 100644 --- a/gazelle/pythonconfig/BUILD.bazel +++ b/gazelle/pythonconfig/BUILD.bazel @@ -18,7 +18,7 @@ go_library( go_test( name = "pythonconfig_test", srcs = ["pythonconfig_test.go"], - deps = [":pythonconfig"], + embed = [":pythonconfig"], ) filegroup( From 401125fda906f75e2f3ee928097f35637f492177 Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Wed, 29 Nov 2023 12:21:19 -0800 Subject: [PATCH 02/12] revert accidental changes --- gazelle/pythonconfig/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gazelle/pythonconfig/BUILD.bazel b/gazelle/pythonconfig/BUILD.bazel index d80902e7ce..d0f1690d94 100644 --- a/gazelle/pythonconfig/BUILD.bazel +++ b/gazelle/pythonconfig/BUILD.bazel @@ -18,7 +18,7 @@ go_library( go_test( name = "pythonconfig_test", srcs = ["pythonconfig_test.go"], - embed = [":pythonconfig"], + deps = [":pythonconfig"], ) filegroup( From c436ec851086802e8ac46973177184a911e45147 Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Wed, 29 Nov 2023 13:25:31 -0800 Subject: [PATCH 03/12] handle empty lines --- gazelle/python/parse.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gazelle/python/parse.py b/gazelle/python/parse.py index ddf82f4f18..7486f8adf3 100644 --- a/gazelle/python/parse.py +++ b/gazelle/python/parse.py @@ -61,6 +61,8 @@ def parse_comments(content): def parse_main(content): for line in content.splitlines(): + if not line.strip(): + continue tokens = list(tokenize(BytesIO(line.encode("utf-8")).readline)) if len(tokens) < 5: continue From 1784a9957e9f8a32cc01fb8097429205fd565ec1 Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Wed, 29 Nov 2023 15:18:54 -0800 Subject: [PATCH 04/12] reimplementing parse_main to fix a failing test --- gazelle/python/parse.py | 27 ++++++++++++++++++--------- gazelle/python/parse_test.py | 16 +++++++++++++--- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/gazelle/python/parse.py b/gazelle/python/parse.py index 7486f8adf3..daa6d2b47c 100644 --- a/gazelle/python/parse.py +++ b/gazelle/python/parse.py @@ -60,17 +60,26 @@ def parse_comments(content): def parse_main(content): - for line in content.splitlines(): - if not line.strip(): - continue - tokens = list(tokenize(BytesIO(line.encode("utf-8")).readline)) - if len(tokens) < 5: + g = tokenize(BytesIO(content.encode("utf-8")).readline) + for token_type, token_val, start, _, _ in g: + if token_type != NAME or token_val != "if" or start[1] != 0: continue - if tokens[1].type == NAME and tokens[1].string == "if" and \ - tokens[2].type == NAME and tokens[2].string == "__name__" and \ - tokens[3].type == OP and tokens[3].string == "==" and \ - tokens[4].type == STRING and tokens[4].string.strip("\"'") == '__main__': + try: + token_type, token_val, start, _, _ = next(g) + if token_type != NAME or token_val != "__name__": + continue + token_type, token_val, start, _, _ = next(g) + if token_type != OP or token_val != "==": + continue + token_type, token_val, start, _, _ = next(g) + if token_type != STRING or token_val.strip("\"'") != '__main__': + continue + token_type, token_val, start, _, _ = next(g) + if token_type != OP or token_val != ":": + continue return True + except StopIteration: + break return False diff --git a/gazelle/python/parse_test.py b/gazelle/python/parse_test.py index 0af2b36bc8..3ebded44b3 100644 --- a/gazelle/python/parse_test.py +++ b/gazelle/python/parse_test.py @@ -16,11 +16,21 @@ def foo(): def test_has_main(self): content = """ -def main(): - pass +import unittest + +from lib import main + + +class ExampleTest(unittest.TestCase): + def test_main(self): + self.assertEqual( + "", + main([["A", 1], ["B", 2]]), + ) + if __name__ == "__main__": - main() + unittest.main() """ self.assertTrue(parse.parse_main(content)) From 050efeb988c8377189870f5233b874ce8c49e70d Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Wed, 29 Nov 2023 15:49:41 -0800 Subject: [PATCH 05/12] fixed error message --- gazelle/python/generate.go | 10 +++------- .../testdata/binary_without_entrypoint/test.yaml | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 4046f8785c..4b8c5355b2 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -243,28 +243,24 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes if !hasPyBinaryEntryPointFile { // Creating one py_binary target per main module when __main__.py doesn't exist. + mainModulesLoop: for _, filename := range mainModules { pyBinaryTargetName := strings.TrimSuffix(filepath.Base(filename), ".py") // Check if a target with the same name we are generating already // exists, and if it is of a different kind from the one we are // generating. If so, we have to throw an error since Gazelle won't // generate it correctly. - collide := false if args.File != nil { for _, t := range args.File.Rules { if t.Name() == pyBinaryTargetName && t.Kind() != actualPyBinaryKind { - fqTarget := label.New("", args.Rel, pyLibraryTargetName) + fqTarget := label.New("", args.Rel, pyBinaryTargetName) log.Printf("failed to generate target %q of kind %q: "+ "a target of kind %q with the same name already exists.", fqTarget.String(), actualPyBinaryKind, t.Kind()) - collide = true - break + continue mainModulesLoop } } } - if collide { - continue - } srcs.Remove(filename) pyBinary := newTargetBuilder(pyBinaryKind, pyBinaryTargetName, pythonProjectRoot, args.Rel, pyFileNames). addVisibility(visibility). diff --git a/gazelle/python/testdata/binary_without_entrypoint/test.yaml b/gazelle/python/testdata/binary_without_entrypoint/test.yaml index fbc5a34fcc..2e3a645ebd 100644 --- a/gazelle/python/testdata/binary_without_entrypoint/test.yaml +++ b/gazelle/python/testdata/binary_without_entrypoint/test.yaml @@ -15,4 +15,4 @@ --- expect: stderr: | - gazelle: failed to generate target "//:binary_without_entrypoint" of kind "py_binary": a target of kind "filegroup" with the same name already exists. + gazelle: failed to generate target "//:collided_main" of kind "py_binary": a target of kind "filegroup" with the same name already exists. From d1b0593ee3e20767fb752d82f959c1c9840fd025 Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Sun, 3 Dec 2023 11:17:21 -0800 Subject: [PATCH 06/12] Adding test cases for multiple py_binary and tests files --- .../testdata/binary_without_entrypoint/BUILD.out | 13 ++++++++++++- .../testdata/binary_without_entrypoint/main2.py | 2 ++ .../testdata/binary_without_entrypoint/main_test.py | 7 +++++++ 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 gazelle/python/testdata/binary_without_entrypoint/main2.py create mode 100644 gazelle/python/testdata/binary_without_entrypoint/main_test.py diff --git a/gazelle/python/testdata/binary_without_entrypoint/BUILD.out b/gazelle/python/testdata/binary_without_entrypoint/BUILD.out index 5fccc2bbf3..458c4b667d 100644 --- a/gazelle/python/testdata/binary_without_entrypoint/BUILD.out +++ b/gazelle/python/testdata/binary_without_entrypoint/BUILD.out @@ -1,4 +1,4 @@ -load("@rules_python//python:defs.bzl", "py_binary", "py_library") +load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test") filegroup( name = "collided_main", @@ -11,6 +11,12 @@ py_binary( visibility = ["//:__subpackages__"], ) +py_binary( + name = "main2", + srcs = ["main2.py"], + visibility = ["//:__subpackages__"], +) + py_library( name = "binary_without_entrypoint", srcs = [ @@ -18,4 +24,9 @@ py_library( "collided_main.py", ], visibility = ["//:__subpackages__"], +) + +py_test( + name = "main_test", + srcs = ["main_test.py"], ) \ No newline at end of file diff --git a/gazelle/python/testdata/binary_without_entrypoint/main2.py b/gazelle/python/testdata/binary_without_entrypoint/main2.py new file mode 100644 index 0000000000..84e642a029 --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/main2.py @@ -0,0 +1,2 @@ +if __name__ == "__main__": + run() diff --git a/gazelle/python/testdata/binary_without_entrypoint/main_test.py b/gazelle/python/testdata/binary_without_entrypoint/main_test.py new file mode 100644 index 0000000000..505a766319 --- /dev/null +++ b/gazelle/python/testdata/binary_without_entrypoint/main_test.py @@ -0,0 +1,7 @@ +import unittest + +class TestMain(unittest.unittest): + pass + +if __name__ == "__main__": + unittest.main() From 2423b0c610fd59a0d6fb2a8b9b4c057539e7b2be Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Sun, 3 Dec 2023 11:52:06 -0800 Subject: [PATCH 07/12] sorting py_binary --- gazelle/python/__main__.py | 2 +- gazelle/python/generate.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/gazelle/python/__main__.py b/gazelle/python/__main__.py index 18bc1ca37f..9974c66d13 100644 --- a/gazelle/python/__main__.py +++ b/gazelle/python/__main__.py @@ -23,7 +23,7 @@ if __name__ == "__main__": if len(sys.argv) < 2: - sys.exit("Please provide subcommand, either print or std_modules") + sys.exit("Please provide subcommand, either parse or std_modules") if sys.argv[1] == "parse": sys.exit(parse.main(sys.stdin, sys.stdout)) elif sys.argv[1] == "std_modules": diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 4b8c5355b2..663d5813ba 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -20,6 +20,7 @@ import ( "log" "os" "path/filepath" + "sort" "strings" "github.com/bazelbuild/bazel-gazelle/config" @@ -242,6 +243,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } if !hasPyBinaryEntryPointFile { + sort.Strings(mainModules) // Creating one py_binary target per main module when __main__.py doesn't exist. mainModulesLoop: for _, filename := range mainModules { From bb82dde8abd6afcdb4d605379719bb511c281f04 Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Sun, 3 Dec 2023 16:29:49 -0800 Subject: [PATCH 08/12] fix test failures --- gazelle/python/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/gazelle/python/BUILD.bazel b/gazelle/python/BUILD.bazel index 040b88ea7e..fd051ebda6 100644 --- a/gazelle/python/BUILD.bazel +++ b/gazelle/python/BUILD.bazel @@ -64,6 +64,7 @@ py_test( "parse.py", "parse_test.py", ], + imports = ["."], ) filegroup( From 38c10a971215e3762507f342f872036a45784f3a Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Sun, 3 Dec 2023 17:11:35 -0800 Subject: [PATCH 09/12] increase timeout for requirements_3_10_test --- .bazelversion | 2 +- examples/bzlmod/.bazelversion | 2 +- examples/bzlmod/BUILD.bazel | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.bazelversion b/.bazelversion index 6abaeb2f90..91e4a9f262 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -6.2.0 +6.3.2 diff --git a/examples/bzlmod/.bazelversion b/examples/bzlmod/.bazelversion index 09b254e90c..91e4a9f262 100644 --- a/examples/bzlmod/.bazelversion +++ b/examples/bzlmod/.bazelversion @@ -1 +1 @@ -6.0.0 +6.3.2 diff --git a/examples/bzlmod/BUILD.bazel b/examples/bzlmod/BUILD.bazel index 6a4fdb8c4f..bee7d1fd18 100644 --- a/examples/bzlmod/BUILD.bazel +++ b/examples/bzlmod/BUILD.bazel @@ -28,6 +28,7 @@ compile_pip_requirements_3_10( src = "requirements.in", requirements_txt = "requirements_lock_3_10.txt", requirements_windows = "requirements_windows_3_10.txt", + timeout = "moderate", ) # The rules below are language specific rules defined in From bbd0389426a9213a3f03e8c203a6848cf1009028 Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Sun, 3 Dec 2023 20:39:47 -0800 Subject: [PATCH 10/12] formatting build file --- examples/bzlmod/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/bzlmod/BUILD.bazel b/examples/bzlmod/BUILD.bazel index bee7d1fd18..bb16f98a6f 100644 --- a/examples/bzlmod/BUILD.bazel +++ b/examples/bzlmod/BUILD.bazel @@ -25,10 +25,10 @@ compile_pip_requirements_3_9( # with pip-compile. compile_pip_requirements_3_10( name = "requirements_3_10", + timeout = "moderate", src = "requirements.in", requirements_txt = "requirements_lock_3_10.txt", requirements_windows = "requirements_windows_3_10.txt", - timeout = "moderate", ) # The rules below are language specific rules defined in From de9ce9bb2b086f3538bd0c179881042bae55df5b Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Sun, 3 Dec 2023 20:44:27 -0800 Subject: [PATCH 11/12] revert bazel version change --- .bazelversion | 2 +- examples/bzlmod/.bazelversion | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.bazelversion b/.bazelversion index 91e4a9f262..6abaeb2f90 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -6.3.2 +6.2.0 diff --git a/examples/bzlmod/.bazelversion b/examples/bzlmod/.bazelversion index 91e4a9f262..09b254e90c 100644 --- a/examples/bzlmod/.bazelversion +++ b/examples/bzlmod/.bazelversion @@ -1 +1 @@ -6.3.2 +6.0.0 From 94e632736272a8f0835271c9dcd2d2a628d2d1ef Mon Sep 17 00:00:00 2001 From: Zhongpeng Lin Date: Fri, 8 Dec 2023 18:01:10 -0800 Subject: [PATCH 12/12] extracting ensureNoCollision --- gazelle/python/generate.go | 98 ++++++++----------- .../binary_without_entrypoint/BUILD.in | 4 +- .../binary_without_entrypoint/BUILD.out | 4 +- .../binary_without_entrypoint/test.yaml | 2 +- 4 files changed, 47 insertions(+), 61 deletions(-) diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 663d5813ba..f812f17a63 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -229,39 +229,24 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes // exists, and if it is of a different kind from the one we are // generating. If so, we have to throw an error since Gazelle won't // generate it correctly. - if args.File != nil { - for _, t := range args.File.Rules { - if t.Name() == pyLibraryTargetName && t.Kind() != actualPyLibraryKind { - fqTarget := label.New("", args.Rel, pyLibraryTargetName) - err := fmt.Errorf("failed to generate target %q of kind %q: "+ - "a target of kind %q with the same name already exists. "+ - "Use the '# gazelle:%s' directive to change the naming convention.", - fqTarget.String(), actualPyLibraryKind, t.Kind(), pythonconfig.LibraryNamingConvention) - collisionErrors.Add(err) - } - } + if err := ensureNoCollision(args.File, pyLibraryTargetName, actualPyLibraryKind); err != nil { + fqTarget := label.New("", args.Rel, pyLibraryTargetName) + err := fmt.Errorf("failed to generate target %q of kind %q: %w. "+ + "Use the '# gazelle:%s' directive to change the naming convention.", + fqTarget.String(), actualPyLibraryKind, err, pythonconfig.LibraryNamingConvention) + collisionErrors.Add(err) } if !hasPyBinaryEntryPointFile { sort.Strings(mainModules) // Creating one py_binary target per main module when __main__.py doesn't exist. - mainModulesLoop: for _, filename := range mainModules { pyBinaryTargetName := strings.TrimSuffix(filepath.Base(filename), ".py") - // Check if a target with the same name we are generating already - // exists, and if it is of a different kind from the one we are - // generating. If so, we have to throw an error since Gazelle won't - // generate it correctly. - if args.File != nil { - for _, t := range args.File.Rules { - if t.Name() == pyBinaryTargetName && t.Kind() != actualPyBinaryKind { - fqTarget := label.New("", args.Rel, pyBinaryTargetName) - log.Printf("failed to generate target %q of kind %q: "+ - "a target of kind %q with the same name already exists.", - fqTarget.String(), actualPyBinaryKind, t.Kind()) - continue mainModulesLoop - } - } + if err := ensureNoCollision(args.File, pyBinaryTargetName, actualPyBinaryKind); err != nil { + fqTarget := label.New("", args.Rel, pyBinaryTargetName) + log.Printf("failed to generate target %q of kind %q: %v", + fqTarget.String(), actualPyBinaryKind, err) + continue } srcs.Remove(filename) pyBinary := newTargetBuilder(pyBinaryKind, pyBinaryTargetName, pythonProjectRoot, args.Rel, pyFileNames). @@ -315,17 +300,12 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes // exists, and if it is of a different kind from the one we are // generating. If so, we have to throw an error since Gazelle won't // generate it correctly. - if args.File != nil { - for _, t := range args.File.Rules { - if t.Name() == pyBinaryTargetName && t.Kind() != actualPyBinaryKind { - fqTarget := label.New("", args.Rel, pyBinaryTargetName) - err := fmt.Errorf("failed to generate target %q of kind %q: "+ - "a target of kind %q with the same name already exists. "+ - "Use the '# gazelle:%s' directive to change the naming convention.", - fqTarget.String(), actualPyBinaryKind, t.Kind(), pythonconfig.BinaryNamingConvention) - collisionErrors.Add(err) - } - } + if err := ensureNoCollision(args.File, pyBinaryTargetName, actualPyBinaryKind); err != nil { + fqTarget := label.New("", args.Rel, pyBinaryTargetName) + err := fmt.Errorf("failed to generate target %q of kind %q: %w. "+ + "Use the '# gazelle:%s' directive to change the naming convention.", + fqTarget.String(), actualPyBinaryKind, err, pythonconfig.BinaryNamingConvention) + collisionErrors.Add(err) } pyBinaryTarget := newTargetBuilder(pyBinaryKind, pyBinaryTargetName, pythonProjectRoot, args.Rel, pyFileNames). @@ -352,16 +332,11 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes // exists, and if it is of a different kind from the one we are // generating. If so, we have to throw an error since Gazelle won't // generate it correctly. - if args.File != nil { - for _, t := range args.File.Rules { - if t.Name() == conftestTargetname && t.Kind() != actualPyLibraryKind { - fqTarget := label.New("", args.Rel, conftestTargetname) - err := fmt.Errorf("failed to generate target %q of kind %q: "+ - "a target of kind %q with the same name already exists.", - fqTarget.String(), actualPyLibraryKind, t.Kind()) - collisionErrors.Add(err) - } - } + if err := ensureNoCollision(args.File, conftestTargetname, actualPyLibraryKind); err != nil { + fqTarget := label.New("", args.Rel, conftestTargetname) + err := fmt.Errorf("failed to generate target %q of kind %q: %w. ", + fqTarget.String(), actualPyLibraryKind, err) + collisionErrors.Add(err) } conftestTarget := newTargetBuilder(pyLibraryKind, conftestTargetname, pythonProjectRoot, args.Rel, pyFileNames). @@ -387,17 +362,12 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes // exists, and if it is of a different kind from the one we are // generating. If so, we have to throw an error since Gazelle won't // generate it correctly. - if args.File != nil { - for _, t := range args.File.Rules { - if t.Name() == pyTestTargetName && t.Kind() != actualPyTestKind { - fqTarget := label.New("", args.Rel, pyTestTargetName) - err := fmt.Errorf("failed to generate target %q of kind %q: "+ - "a target of kind %q with the same name already exists. "+ - "Use the '# gazelle:%s' directive to change the naming convention.", - fqTarget.String(), actualPyTestKind, t.Kind(), pythonconfig.TestNamingConvention) - collisionErrors.Add(err) - } - } + if err := ensureNoCollision(args.File, pyTestTargetName, actualPyTestKind); err != nil { + fqTarget := label.New("", args.Rel, pyTestTargetName) + err := fmt.Errorf("failed to generate target %q of kind %q: %w. "+ + "Use the '# gazelle:%s' directive to change the naming convention.", + fqTarget.String(), actualPyTestKind, err, pythonconfig.TestNamingConvention) + collisionErrors.Add(err) } return newTargetBuilder(pyTestKind, pyTestTargetName, pythonProjectRoot, args.Rel, pyFileNames). addSrcs(srcs). @@ -509,3 +479,15 @@ func isEntrypointFile(path string) bool { return false } } + +func ensureNoCollision(file *rule.File, targetName, kind string) error { + if file == nil { + return nil + } + for _, t := range file.Rules { + if t.Name() == targetName && t.Kind() != kind { + return fmt.Errorf("a target of kind %q with the same name already exists", t.Kind()) + } + } + return nil +} diff --git a/gazelle/python/testdata/binary_without_entrypoint/BUILD.in b/gazelle/python/testdata/binary_without_entrypoint/BUILD.in index 3ccb32e598..7aace6794f 100644 --- a/gazelle/python/testdata/binary_without_entrypoint/BUILD.in +++ b/gazelle/python/testdata/binary_without_entrypoint/BUILD.in @@ -1,4 +1,6 @@ +# gazelle:python_library_naming_convention py_default_library + filegroup( name = "collided_main", srcs = ["collided_main.py"], -) \ No newline at end of file +) diff --git a/gazelle/python/testdata/binary_without_entrypoint/BUILD.out b/gazelle/python/testdata/binary_without_entrypoint/BUILD.out index 458c4b667d..c88f2ff11e 100644 --- a/gazelle/python/testdata/binary_without_entrypoint/BUILD.out +++ b/gazelle/python/testdata/binary_without_entrypoint/BUILD.out @@ -1,5 +1,7 @@ load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test") +# gazelle:python_library_naming_convention py_default_library + filegroup( name = "collided_main", srcs = ["collided_main.py"], @@ -18,7 +20,7 @@ py_binary( ) py_library( - name = "binary_without_entrypoint", + name = "py_default_library", srcs = [ "__init__.py", "collided_main.py", diff --git a/gazelle/python/testdata/binary_without_entrypoint/test.yaml b/gazelle/python/testdata/binary_without_entrypoint/test.yaml index 2e3a645ebd..44e4ae8364 100644 --- a/gazelle/python/testdata/binary_without_entrypoint/test.yaml +++ b/gazelle/python/testdata/binary_without_entrypoint/test.yaml @@ -15,4 +15,4 @@ --- expect: stderr: | - gazelle: failed to generate target "//:collided_main" of kind "py_binary": a target of kind "filegroup" with the same name already exists. + gazelle: failed to generate target "//:collided_main" of kind "py_binary": a target of kind "filegroup" with the same name already exists