feat(gazelle): create py_binary targets for `if name == "main"`#1566

aignas · 2023-11-17T01:14:38Z

I personally think that this new feature needs a feature toggle. There should be a gazelle directive that enables the behaviour.
What is more, sometimes developers add if __name__ in order to test the script locally but do not intend to create a binary target for others to consume. I wonder if in those cases we should be able to say in the python file:
if __name__ == "__main__": # gazelle: ignore main()

A feature toggle is a great call. Let me look into making that work.
As for the ignore, is there any reason you wouldn't want that to get moved to a py_binary? Then you could bazel run it and test it locally. (If the script is already in a test target, then this change won't move it - it only moves scripts from py_library -> py_binary, not from py_test, because you can already bazel run a test.) I guess I'm just curious when you might want an ignore / might want to keep a script out of a py_binary target.

Let's keep the ignore part out of scope for now. It probably does not need to be supported in the initial version.

aignas · 2023-11-17T01:10:03Z

Right now we are reading the python files once to get the imports and here it would be for once more to check if there is an if __name__ == "main" which may not scale well in super large repos because we have now twice the number of files to process.

Let me see how the read to get the import works. Maybe I can figure out a way to not read it twice.

aignas · 2023-11-17T01:11:41Z

Reading from the end of the file line by line in reverse and matching the whole line (stripped) could be faster? If you want to still use regexp, you could at least compile the regexp upfront and store it as a var at the top of the file.

Yeah, let me look into making this faster.

aignas · 2023-11-17T01:14:38Z

I personally think that this new feature needs a feature toggle. There should be a gazelle directive that enables the behaviour.
What is more, sometimes developers add if __name__ in order to test the script locally but do not intend to create a binary target for others to consume. I wonder if in those cases we should be able to say in the python file:
if __name__ == "__main__": # gazelle: ignore main()

A feature toggle is a great call. Let me look into making that work.
As for the ignore, is there any reason you wouldn't want that to get moved to a py_binary? Then you could bazel run it and test it locally. (If the script is already in a test target, then this change won't move it - it only moves scripts from py_library -> py_binary, not from py_test, because you can already bazel run a test.) I guess I'm just curious when you might want an ignore / might want to keep a script out of a py_binary target.

Let's keep the ignore part out of scope for now. It probably does not need to be supported in the initial version.

aignas · 2023-11-17T01:10:03Z

Right now we are reading the python files once to get the imports and here it would be for once more to check if there is an if __name__ == "main" which may not scale well in super large repos because we have now twice the number of files to process.

Let me see how the read to get the import works. Maybe I can figure out a way to not read it twice.

aignas · 2023-11-17T01:11:41Z

Reading from the end of the file line by line in reverse and matching the whole line (stripped) could be faster? If you want to still use regexp, you could at least compile the regexp upfront and store it as a var at the top of the file.

Yeah, let me look into making this faster.

aignas · 2023-11-17T01:14:38Z

I personally think that this new feature needs a feature toggle. There should be a gazelle directive that enables the behaviour.
What is more, sometimes developers add if __name__ in order to test the script locally but do not intend to create a binary target for others to consume. I wonder if in those cases we should be able to say in the python file:
if __name__ == "__main__": # gazelle: ignore main()

A feature toggle is a great call. Let me look into making that work.
As for the ignore, is there any reason you wouldn't want that to get moved to a py_binary? Then you could bazel run it and test it locally. (If the script is already in a test target, then this change won't move it - it only moves scripts from py_library -> py_binary, not from py_test, because you can already bazel run a test.) I guess I'm just curious when you might want an ignore / might want to keep a script out of a py_binary target.

Let's keep the ignore part out of scope for now. It probably does not need to be supported in the initial version.

aignas · 2023-11-17T01:10:03Z

Right now we are reading the python files once to get the imports and here it would be for once more to check if there is an if __name__ == "main" which may not scale well in super large repos because we have now twice the number of files to process.

Let me see how the read to get the import works. Maybe I can figure out a way to not read it twice.

aignas · 2023-11-17T01:11:41Z

Reading from the end of the file line by line in reverse and matching the whole line (stripped) could be faster? If you want to still use regexp, you could at least compile the regexp upfront and store it as a var at the top of the file.

Yeah, let me look into making this faster.

aignas · 2023-11-17T01:14:38Z

I personally think that this new feature needs a feature toggle. There should be a gazelle directive that enables the behaviour.
What is more, sometimes developers add if __name__ in order to test the script locally but do not intend to create a binary target for others to consume. I wonder if in those cases we should be able to say in the python file:
if __name__ == "__main__": # gazelle: ignore main()

A feature toggle is a great call. Let me look into making that work.
As for the ignore, is there any reason you wouldn't want that to get moved to a py_binary? Then you could bazel run it and test it locally. (If the script is already in a test target, then this change won't move it - it only moves scripts from py_library -> py_binary, not from py_test, because you can already bazel run a test.) I guess I'm just curious when you might want an ignore / might want to keep a script out of a py_binary target.

Let's keep the ignore part out of scope for now. It probably does not need to be supported in the initial version.

aignas · 2023-11-17T01:10:03Z

Right now we are reading the python files once to get the imports and here it would be for once more to check if there is an if __name__ == "main" which may not scale well in super large repos because we have now twice the number of files to process.

Let me see how the read to get the import works. Maybe I can figure out a way to not read it twice.

aignas · 2023-11-17T01:11:41Z

Reading from the end of the file line by line in reverse and matching the whole line (stripped) could be faster? If you want to still use regexp, you could at least compile the regexp upfront and store it as a var at the top of the file.

Yeah, let me look into making this faster.

aignas · 2023-11-17T01:14:38Z

I personally think that this new feature needs a feature toggle. There should be a gazelle directive that enables the behaviour.
What is more, sometimes developers add if __name__ in order to test the script locally but do not intend to create a binary target for others to consume. I wonder if in those cases we should be able to say in the python file:
if __name__ == "__main__": # gazelle: ignore main()

A feature toggle is a great call. Let me look into making that work.
As for the ignore, is there any reason you wouldn't want that to get moved to a py_binary? Then you could bazel run it and test it locally. (If the script is already in a test target, then this change won't move it - it only moves scripts from py_library -> py_binary, not from py_test, because you can already bazel run a test.) I guess I'm just curious when you might want an ignore / might want to keep a script out of a py_binary target.

Let's keep the ignore part out of scope for now. It probably does not need to be supported in the initial version.

aignas · 2023-11-17T01:10:03Z

Right now we are reading the python files once to get the imports and here it would be for once more to check if there is an if __name__ == "main" which may not scale well in super large repos because we have now twice the number of files to process.

Let me see how the read to get the import works. Maybe I can figure out a way to not read it twice.

aignas · 2023-11-17T01:11:41Z

Reading from the end of the file line by line in reverse and matching the whole line (stripped) could be faster? If you want to still use regexp, you could at least compile the regexp upfront and store it as a var at the top of the file.

Yeah, let me look into making this faster.

aignas · 2023-11-17T01:14:38Z

I personally think that this new feature needs a feature toggle. There should be a gazelle directive that enables the behaviour.
What is more, sometimes developers add if __name__ in order to test the script locally but do not intend to create a binary target for others to consume. I wonder if in those cases we should be able to say in the python file:
if __name__ == "__main__": # gazelle: ignore main()

A feature toggle is a great call. Let me look into making that work.
As for the ignore, is there any reason you wouldn't want that to get moved to a py_binary? Then you could bazel run it and test it locally. (If the script is already in a test target, then this change won't move it - it only moves scripts from py_library -> py_binary, not from py_test, because you can already bazel run a test.) I guess I'm just curious when you might want an ignore / might want to keep a script out of a py_binary target.

Let's keep the ignore part out of scope for now. It probably does not need to be supported in the initial version.

aignas · 2023-11-17T01:10:03Z

Right now we are reading the python files once to get the imports and here it would be for once more to check if there is an if __name__ == "main" which may not scale well in super large repos because we have now twice the number of files to process.

Let me see how the read to get the import works. Maybe I can figure out a way to not read it twice.

aignas · 2023-11-17T01:11:41Z

Reading from the end of the file line by line in reverse and matching the whole line (stripped) could be faster? If you want to still use regexp, you could at least compile the regexp upfront and store it as a var at the top of the file.

Yeah, let me look into making this faster.

aignas · 2023-11-17T01:14:38Z

I personally think that this new feature needs a feature toggle. There should be a gazelle directive that enables the behaviour.
What is more, sometimes developers add if __name__ in order to test the script locally but do not intend to create a binary target for others to consume. I wonder if in those cases we should be able to say in the python file:
if __name__ == "__main__": # gazelle: ignore main()

A feature toggle is a great call. Let me look into making that work.
As for the ignore, is there any reason you wouldn't want that to get moved to a py_binary? Then you could bazel run it and test it locally. (If the script is already in a test target, then this change won't move it - it only moves scripts from py_library -> py_binary, not from py_test, because you can already bazel run a test.) I guess I'm just curious when you might want an ignore / might want to keep a script out of a py_binary target.

Let's keep the ignore part out of scope for now. It probably does not need to be supported in the initial version.

aignas · 2023-11-17T01:10:03Z

Right now we are reading the python files once to get the imports and here it would be for once more to check if there is an if __name__ == "main" which may not scale well in super large repos because we have now twice the number of files to process.

Let me see how the read to get the import works. Maybe I can figure out a way to not read it twice.

aignas · 2023-11-17T01:11:41Z

Reading from the end of the file line by line in reverse and matching the whole line (stripped) could be faster? If you want to still use regexp, you could at least compile the regexp upfront and store it as a var at the top of the file.

Yeah, let me look into making this faster.

aignas · 2023-11-17T01:14:38Z

I personally think that this new feature needs a feature toggle. There should be a gazelle directive that enables the behaviour.
What is more, sometimes developers add if __name__ in order to test the script locally but do not intend to create a binary target for others to consume. I wonder if in those cases we should be able to say in the python file:
if __name__ == "__main__": # gazelle: ignore main()

A feature toggle is a great call. Let me look into making that work.
As for the ignore, is there any reason you wouldn't want that to get moved to a py_binary? Then you could bazel run it and test it locally. (If the script is already in a test target, then this change won't move it - it only moves scripts from py_library -> py_binary, not from py_test, because you can already bazel run a test.) I guess I'm just curious when you might want an ignore / might want to keep a script out of a py_binary target.

Let's keep the ignore part out of scope for now. It probably does not need to be supported in the initial version.

aignas · 2023-11-17T01:10:03Z

Right now we are reading the python files once to get the imports and here it would be for once more to check if there is an if __name__ == "main" which may not scale well in super large repos because we have now twice the number of files to process.

Let me see how the read to get the import works. Maybe I can figure out a way to not read it twice.

aignas · 2023-11-17T01:11:41Z

Reading from the end of the file line by line in reverse and matching the whole line (stripped) could be faster? If you want to still use regexp, you could at least compile the regexp upfront and store it as a var at the top of the file.

Yeah, let me look into making this faster.

-Original file line number
+Diff line change
@@ Expand Up / @@ -69,6 +69,9 @@ A brief description of the categories of changes: @@
  `__test__.py` exists in the same package. Previously in these cases there
  would only be one test target made.
+* (gazelle) If a non-test Python file contains `if __name__ == "__main__":`,
+ then a `py_binary` target is made for it instead of a `py_library` target.
 Breaking changes:
 * (pip) `pip_install` repository rule in this release has been disabled and
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -20,6 +20,7 @@ import ( @@
 	"log"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"github.com/bazelbuild/bazel-gazelle/config"
@@ Expand DownExpand Up @@
 	packageName := filepath.Base(args.Dir)
+	pyBinaryFilenames := treeset.NewWith(godsutils.StringComparator)
 	pyLibraryFilenames := treeset.NewWith(godsutils.StringComparator)
 	pyTestFilenames := treeset.NewWith(godsutils.StringComparator)
 	pyFileNames := treeset.NewWith(godsutils.StringComparator)
-	// hasPyBinary controls whether a py_binary target should be generated for
-	// this package or not.
-	hasPyBinary := false
 	// hasPyTestEntryPointFile and hasPyTestEntryPointTarget control whether a py_test target should
 	// be generated for this package or not.
 	hasPyTestEntryPointFile := false
@@ Expand All @@
  ext := filepath.Ext(f)
  if ext == ".py" {
  pyFileNames.Add(f)
- if !hasPyBinary && f == pyBinaryEntrypointFilename {
- hasPyBinary = true
- } else if !hasPyTestEntryPointFile && f == pyTestEntrypointFilename {
+ if !hasPyTestEntryPointFile && f == pyTestEntrypointFilename {
  hasPyTestEntryPointFile = true
  } else if f == conftestFilename {
  hasConftestFile = true
  } else if strings.HasSuffix(f, "_test.py") || strings.HasPrefix(f, "test_") {
  pyTestFilenames.Add(f)
+ } else if f == pyBinaryEntrypointFilename || hasNameEqualsMain(filepath.Join(args.Config.RepoRoot, args.Rel, f)) {
+ pyBinaryFilenames.Add(f)
  } else {
  pyLibraryFilenames.Add(f)
  }
@@ Expand DownExpand Up @@
  appendPyLibrary(pyLibraryFilenames, cfg.RenderLibraryName(packageName))
 	}
-	if hasPyBinary {
- deps, err := parser.parseSingle(pyBinaryEntrypointFilename)
+	pyBinaryFilenames.Each(func(index int, filename interface{}) {
+ entrypointFilename := filename.(string)
+ deps, err := parser.parseSingle(entrypointFilename)
  if err != nil {
  log.Fatalf("ERROR: %v\n", err)
  }
- pyBinaryTargetName := cfg.RenderBinaryName(packageName)
+ var pyBinaryTargetName string
+ if entrypointFilename == pyBinaryEntrypointFilename {
+ pyBinaryTargetName = cfg.RenderBinaryName(packageName)
+ } else {
+ pyBinaryTargetName = strings.TrimSuffix(filepath.Base(filename.(string)), ".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
@@ Expand All @@
  }
  pyBinaryTarget := newTargetBuilder(pyBinaryKind, pyBinaryTargetName, pythonProjectRoot, args.Rel, pyFileNames).
- setMain(pyBinaryEntrypointFilename).
  addVisibility(visibility).
- addSrc(pyBinaryEntrypointFilename).
+ addSrc(entrypointFilename).
  addModuleDependencies(deps).
  generateImportsAttribute()
+ if entrypointFilename == pyBinaryEntrypointFilename {
+ pyBinaryTarget.setMain(pyBinaryEntrypointFilename)
+ }
  pyBinary := pyBinaryTarget.build()
  result.Gen = append(result.Gen, pyBinary)
  result.Imports = append(result.Imports, pyBinary.PrivateAttr(config.GazelleImportsKey))
-	}
+	})
 	var conftest *rule.Rule
 	if hasConftestFile {
@@ Expand DownExpand Up / @@ -463,6 +470,20 @@ func hasEntrypointFile(dir string) bool { @@
 	return false
 }
+// hasNameEqualsMain determines if the file contains 'if __name__ == "__main__"'.
+func hasNameEqualsMain(path string) bool {
+	searchString := `if __name__ == ['"]__main__['"]:`
+	bytesContents, err := os.ReadFile(path)
+	if err != nil {
+ return false
+	}
+	match, err := regexp.Match(searchString, bytesContents)
+	if err == nil {
+ return match
+	}
+	return false
+}
 // isEntrypointFile returns whether the given path is an entrypoint file. The
 // given path can be absolute or relative.
 func isEntrypointFile(path string) bool {
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,35 @@
+load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test")
+py_library(
+ name = "binary_targets",
+ srcs = [
+ "bar.py",
+ "baz.py",
+ ],
+ visibility = ["//:__subpackages__"],
+)
+py_binary(
+ name = "bar_binary",
+ srcs = ["bar_binary.py"],
+ visibility = ["//:__subpackages__"],
+ deps = [":binary_targets"],
+)
+py_binary(
+ name = "single_quote_main",
+ srcs = ["single_quote_main.py"],
+ visibility = ["//:__subpackages__"],
+ deps = [":bar_test"],
+)
+py_test(
+ name = "bar_test",
+ srcs = ["bar_test.py"],
+ deps = [":bar_binary"],
+)
+py_test(
+ name = "name_main_test",
+ srcs = ["name_main_test.py"],
+)

-Original file line number
+Diff line change
@@ -0,0 +1,4 @@
+# Binary targets
+This test case generates `py_binary` targets for files containing
+`if __name__ == "__main__"`.

Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		# This is a Bazel workspace for the Gazelle test data.

Uh oh!

Uh oh!

feat(gazelle): create py_binary targets for if __name__ == "__main__"#1566

Uh oh!

feat(gazelle): create py_binary targets for if __name__ == "__main__"#1566

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

adzenithNov 17, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat(gazelle): create py_binary targets for if __name__ == "__main__"#1566

Uh oh!

feat(gazelle): create py_binary targets for if __name__ == "__main__"#1566

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

adzenithNov 17, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat(gazelle): create py_binary targets for if __name__ == "__main__"#1566

Uh oh!

feat(gazelle): create py_binary targets for if __name__ == "__main__"#1566

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

adzenithNov 17, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feat(gazelle): create py_binary targets for `if name == "main"`#1566

feat(gazelle): create py_binary targets for `if name == "main"`#1566

adzenithNov 17, 2023•
edited
Loading

feat(gazelle): create py_binary targets for `if name == "main"`#1566

feat(gazelle): create py_binary targets for `if name == "main"`#1566

adzenithNov 17, 2023•
edited
Loading

feat(gazelle): create py_binary targets for `if name == "main"`#1566

feat(gazelle): create py_binary targets for `if name == "main"`#1566

adzenithNov 17, 2023•
edited
Loading

feat(gazelle): create py_binary targets for `if name == "main"`#1566

feat(gazelle): create py_binary targets for `if name == "main"`#1566

adzenithNov 17, 2023•
edited
Loading