feat(gazelle): Remove integrity field from Gazelle manifest (#1666)

As per the discussion in #1465 and in this PR, this PR does the
following:

- Make the `requirements` parameter to the `gazelle_python_manifest`
macro optional.
- If `requirements` is not provided, no integrity field is added to the
manifest, and `diff_test` is used to see if the manifest is stale
instead of the existing `go_test` that just checks the integrity.
- Migrates `go_binary` to `genrule` to generate the manifest file that
can be used with `diff_test`. (There's still a `go_binary` under the
hood, but the manifest macro itself uses a genrule now rather than a
wrapper `go_binary`.)
- It does not migrate the manifest generator from Go to Python;
hopefully that can be deferred to another PR.

A custom `copy_to_source.sh` script is included, which is used to update
the manifest in the source tree.

Compatibility discussion:
- The update and test target names have not changed; they are still
`//:gazelle_python_manifest.update` and
`//:gazelle_python_manifest.test`.

Author: adzenith

CHANGELOG.md

@@ -164,6 +164,10 @@ A brief description of the categories of changes:
   target for each file with `if __name__ == "__main__"` instead of just one
   `py_binary` for the whole module.
 
+* (gazelle) the Gazelle manifest integrity field is now optional. If the
+  `requirements` argument to `gazelle_python_manifest` is unset, no integrity
+  field will be generated.
+
 ### Fixed
 
 * (gazelle) The gazelle plugin helper was not working with Python toolchains 3.11

examples/bzlmod_build_file_generation/.bazelversion

@@ -1 +1 @@
-6.0.0
+6.4.0

examples/bzlmod_build_file_generation/BUILD.bazel

@@ -29,6 +29,8 @@ modules_mapping(
     exclude_patterns = [
         "^_|(\\._)+",  # This is the default.
         "(\\.tests)+",  # Add a custom one to get rid of the psutil tests.
+        "^colorama",  # Get rid of colorama on Windows.
+        "^lazy_object_proxy\\.cext$",  # Get rid of this on Linux because it isn't included on Windows.
     ],
     wheels = all_whl_requirements,
 )
@@ -47,10 +49,6 @@ gazelle_python_manifest(
     name = "gazelle_python_manifest",
     modules_mapping = ":modules_map",
     pip_repository_name = "pip",
-    requirements = [
-        "//:requirements_lock.txt",
-        "//:requirements_windows.txt",
-    ],
     tags = ["exclusive"],
 )
 

examples/bzlmod_build_file_generation/gazelle_python.yaml

@@ -586,4 +586,3 @@ manifest:
     yamllint.rules.truthy: yamllint
   pip_repository:
     name: pip
-integrity: cd25503dc6b3d9e1c5f46715ba2d0499ecc8b3d654ebcbf9f4e52f2074290e0a

gazelle/MODULE.bazel

@@ -4,6 +4,7 @@ module(
     compatibility_level = 1,
 )
 
+bazel_dep(name = "bazel_skylib", version = "1.5.0")
 bazel_dep(name = "rules_python", version = "0.18.0")
 bazel_dep(name = "rules_go", version = "0.41.0", repo_name = "io_bazel_rules_go")
 bazel_dep(name = "gazelle", version = "0.33.0", repo_name = "bazel_gazelle")

gazelle/README.md

@@ -114,6 +114,10 @@ gazelle_python_manifest(
     pip_repository_name = "pip",
     # This should point to wherever we declare our python dependencies
     # (the same as what we passed to the modules_mapping rule in WORKSPACE)
+    # This argument is optional. If provided, the `.test` target is very
+    # fast because it just has to check an integrity field. If not provided,
+    # the integrity field is not added to the manifest which can help avoid
+    # merge conflicts in large repos.
     requirements = "//:requirements_lock.txt",
 )
 ```

gazelle/manifest/BUILD.bazel

@@ -1,5 +1,10 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
+exports_files([
+    # This gets wrapped up into a py_binary with args inside of the gazelle_python_manifest macro.
+    "copy_to_source.py",
+])
+
 go_library(
     name = "manifest",
     srcs = ["manifest.go"],

gazelle/manifest/copy_to_source.py

@@ -0,0 +1,36 @@
+"""Copy a generated file to the source tree.
+
+Run like:
+    copy_to_source path/to/generated_file path/to/source_file_to_overwrite
+"""
+
+import os
+import shutil
+import stat
+import sys
+from pathlib import Path
+
+
+def copy_to_source(generated_relative_path: Path, target_relative_path: Path) -> None:
+    """Copy the generated file to the target file path.
+
+    Expands the relative paths by looking at Bazel env vars to figure out which absolute paths to use.
+    """
+    # This script normally gets executed from the runfiles dir, so find the absolute path to the generated file based on that.
+    generated_absolute_path = Path.cwd() / generated_relative_path
+
+    # Similarly, the target is relative to the source directory.
+    target_absolute_path = os.getenv("BUILD_WORKSPACE_DIRECTORY") / target_relative_path
+
+    print(f"Copying {generated_absolute_path} to {target_absolute_path}")
+    target_absolute_path.parent.mkdir(parents=True, exist_ok=True)
+    shutil.copy(generated_absolute_path, target_absolute_path)
+
+    target_absolute_path.chmod(0O664)
+
+
+if __name__ == "__main__":
+    if len(sys.argv) != 3:
+        sys.exit("Usage: copy_to_source <generated_file> <target_file>")
+
+    copy_to_source(Path(sys.argv[1]), Path(sys.argv[2]))

gazelle/manifest/defs.bzl

@@ -16,12 +16,14 @@
 for updating and testing the Gazelle manifest file.
 """
 
-load("@io_bazel_rules_go//go:def.bzl", "GoSource", "go_binary", "go_test")
+load("@bazel_skylib//rules:diff_test.bzl", "diff_test")
+load("@io_bazel_rules_go//go:def.bzl", "GoSource", "go_test")
+load("@rules_python//python:defs.bzl", "py_binary")
 
 def gazelle_python_manifest(
         name,
-        requirements,
         modules_mapping,
+        requirements = [],
         pip_repository_name = "",
         pip_deps_repository_name = "",
         manifest = ":gazelle_python.yaml",
@@ -30,15 +32,18 @@ def gazelle_python_manifest(
 
     Args:
         name: the name used as a base for the targets.
+        modules_mapping: the target for the generated modules_mapping.json file.
         requirements: the target for the requirements.txt file or a list of
             requirements files that will be concatenated before passing on to
-            the manifest generator.
+            the manifest generator. If unset, no integrity field is added to the
+            manifest, meaning testing it is just as expensive as generating it,
+            but modifying it is much less likely to result in a merge conflict.
         pip_repository_name: the name of the pip_install or pip_repository target.
         pip_deps_repository_name: deprecated - the old pip_install target name.
-        modules_mapping: the target for the generated modules_mapping.json file.
-        manifest: the target for the Gazelle manifest file.
-        **kwargs: other bazel attributes passed to the target target generated by
-            this macro.
+        manifest: the Gazelle manifest file.
+            defaults to the same value as manifest.
+        **kwargs: other bazel attributes passed to the generate and test targets
+            generated by this macro.
     """
     if pip_deps_repository_name != "":
         # buildifier: disable=print
@@ -52,12 +57,17 @@ def gazelle_python_manifest(
         # This is a temporary check while pip_deps_repository_name exists as deprecated.
         fail("pip_repository_name must be set in //{}:{}".format(native.package_name(), name))
 
+    test_target = "{}.test".format(name)
     update_target = "{}.update".format(name)
     update_target_label = "//{}:{}".format(native.package_name(), update_target)
 
+    manifest_genrule = name + ".genrule"
+    generated_manifest = name + ".generated_manifest"
+    manifest_generator = Label("//manifest/generate:generate")
     manifest_generator_hash = Label("//manifest/generate:generate_lib_sources_hash")
 
-    if type(requirements) == "list":
+    if requirements and type(requirements) == "list":
+        # This runs if requirements is a list or is unset (default value is empty list)
         native.genrule(
             name = name + "_requirements_gen",
             srcs = sorted(requirements),
@@ -68,56 +78,71 @@ def gazelle_python_manifest(
         requirements = name + "_requirements_gen"
 
     update_args = [
-        "--manifest-generator-hash",
-        "$(rootpath {})".format(manifest_generator_hash),
-        "--requirements",
-        "$(rootpath {})".format(requirements),
-        "--pip-repository-name",
-        pip_repository_name,
-        "--modules-mapping",
-        "$(rootpath {})".format(modules_mapping),
-        "--output",
-        "$(rootpath {})".format(manifest),
-        "--update-target",
-        update_target_label,
+        "--manifest-generator-hash=$(execpath {})".format(manifest_generator_hash),
+        "--requirements=$(rootpath {})".format(requirements) if requirements else "--requirements=",
+        "--pip-repository-name={}".format(pip_repository_name),
+        "--modules-mapping=$(execpath {})".format(modules_mapping),
+        "--output=$(execpath {})".format(generated_manifest),
+        "--update-target={}".format(update_target_label),
     ]
 
-    go_binary(
-        name = update_target,
-        embed = [Label("//manifest/generate:generate_lib")],
-        data = [
-            manifest,
+    native.genrule(
+        name = manifest_genrule,
+        outs = [generated_manifest],
+        cmd = "$(execpath {}) {}".format(manifest_generator, " ".join(update_args)),
+        tools = [manifest_generator],
+        srcs = [
             modules_mapping,
-            requirements,
             manifest_generator_hash,
-        ],
-        args = update_args,
-        visibility = ["//visibility:private"],
-        tags = ["manual"],
+        ] + ([requirements] if requirements else []),
     )
 
-    attrs = {
-        "env": {
-            "_TEST_MANIFEST": "$(rootpath {})".format(manifest),
-            "_TEST_MANIFEST_GENERATOR_HASH": "$(rootpath {})".format(manifest_generator_hash),
-            "_TEST_REQUIREMENTS": "$(rootpath {})".format(requirements),
-        },
-        "size": "small",
-    }
-    go_test(
-        name = "{}.test".format(name),
-        srcs = [Label("//manifest/test:test.go")],
+    py_binary(
+        name = update_target,
+        srcs = [Label("//manifest:copy_to_source.py")],
+        main = Label("//manifest:copy_to_source.py"),
+        args = [
+            "$(rootpath {})".format(generated_manifest),
+            "$(rootpath {})".format(manifest),
+        ],
         data = [
+            generated_manifest,
             manifest,
-            requirements,
-            manifest_generator_hash,
         ],
-        rundir = ".",
-        deps = [Label("//manifest")],
-        # kwargs could contain test-specific attributes like size or timeout
-        **dict(attrs, **kwargs)
+        **kwargs
     )
 
+    if requirements:
+        attrs = {
+            "env": {
+                "_TEST_MANIFEST": "$(rootpath {})".format(manifest),
+                "_TEST_MANIFEST_GENERATOR_HASH": "$(rootpath {})".format(manifest_generator_hash),
+                "_TEST_REQUIREMENTS": "$(rootpath {})".format(requirements),
+            },
+            "size": "small",
+        }
+        go_test(
+            name = test_target,
+            srcs = [Label("//manifest/test:test.go")],
+            data = [
+                manifest,
+                requirements,
+                manifest_generator_hash,
+            ],
+            rundir = ".",
+            deps = [Label("//manifest")],
+            # kwargs could contain test-specific attributes like size or timeout
+            **dict(attrs, **kwargs)
+        )
+    else:
+        diff_test(
+            name = test_target,
+            file1 = generated_manifest,
+            file2 = manifest,
+            failure_message = "Gazelle manifest is out of date. Run 'bazel run {}' to update it.".format(native.package_relative_label(update_target)),
+            **kwargs
+        )
+
     native.filegroup(
         name = name,
         srcs = [manifest],

gazelle/manifest/generate/generate.go

@@ -31,12 +31,6 @@ import (
 	"github.com/bazelbuild/rules_python/gazelle/manifest"
 )
 
-func init() {
-	if os.Getenv("BUILD_WORKSPACE_DIRECTORY") == "" {
-		log.Fatalln("ERROR: this program must run under Bazel")
-	}
-}
-
 func main() {
 	var (
 		manifestGeneratorHashPath string
@@ -79,10 +73,6 @@ func main() {
 		"The Bazel target to update the YAML manifest file.")
 	flag.Parse()
 
-	if requirementsPath == "" {
-		log.Fatalln("ERROR: --requirements must be set")
-	}
-
 	if modulesMappingPath == "" {
 		log.Fatalln("ERROR: --modules-mapping must be set")
 	}
@@ -102,12 +92,12 @@ func main() {
 
 	header := generateHeader(updateTarget)
 	repository := manifest.PipRepository{
-		Name:                    pipRepositoryName,
+		Name: pipRepositoryName,
 	}
 
 	manifestFile := manifest.NewFile(&manifest.Manifest{
 		ModulesMapping: modulesMapping,
-		PipRepository: &repository,
+		PipRepository:  &repository,
 	})
 	if err := writeOutput(
 		outputPath,
@@ -155,12 +145,7 @@ func writeOutput(
 	manifestGeneratorHashPath string,
 	requirementsPath string,
 ) error {
-	stat, err := os.Stat(outputPath)
-	if err != nil {
-		return fmt.Errorf("failed to write output: %w", err)
-	}
-
-	outputFile, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_TRUNC, stat.Mode())
+	outputFile, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0644)
 	if err != nil {
 		return fmt.Errorf("failed to write output: %w", err)
 	}
@@ -170,20 +155,26 @@ func writeOutput(
 		return fmt.Errorf("failed to write output: %w", err)
 	}
 
-	manifestGeneratorHash, err := os.Open(manifestGeneratorHashPath)
-	if err != nil {
-		return fmt.Errorf("failed to write output: %w", err)
-	}
-	defer manifestGeneratorHash.Close()
-
-	requirements, err := os.Open(requirementsPath)
-	if err != nil {
-		return fmt.Errorf("failed to write output: %w", err)
-	}
-	defer requirements.Close()
-
-	if err := manifestFile.Encode(outputFile, manifestGeneratorHash, requirements); err != nil {
-		return fmt.Errorf("failed to write output: %w", err)
+	if requirementsPath != "" {
+		manifestGeneratorHash, err := os.Open(manifestGeneratorHashPath)
+		if err != nil {
+			return fmt.Errorf("failed to write output: %w", err)
+		}
+		defer manifestGeneratorHash.Close()
+
+		requirements, err := os.Open(requirementsPath)
+		if err != nil {
+			return fmt.Errorf("failed to write output: %w", err)
+		}
+		defer requirements.Close()
+
+		if err := manifestFile.EncodeWithIntegrity(outputFile, manifestGeneratorHash, requirements); err != nil {
+			return fmt.Errorf("failed to write output: %w", err)
+		}
+	} else {
+		if err := manifestFile.EncodeWithoutIntegrity(outputFile); err != nil {
+			return fmt.Errorf("failed to write output: %w", err)
+		}
 	}
 
 	return nil