diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bb076dc1..5923c5e61 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/examples/bzlmod_build_file_generation/.bazelversion b/examples/bzlmod_build_file_generation/.bazelversion index 09b254e90..19b860c18 100644 --- a/examples/bzlmod_build_file_generation/.bazelversion +++ b/examples/bzlmod_build_file_generation/.bazelversion @@ -1 +1 @@ -6.0.0 +6.4.0 diff --git a/examples/bzlmod_build_file_generation/BUILD.bazel b/examples/bzlmod_build_file_generation/BUILD.bazel index bca3b3681..33d01f411 100644 --- a/examples/bzlmod_build_file_generation/BUILD.bazel +++ b/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"], ) diff --git a/examples/bzlmod_build_file_generation/gazelle_python.yaml b/examples/bzlmod_build_file_generation/gazelle_python.yaml index 46a1c8b33..ef0146012 100644 --- a/examples/bzlmod_build_file_generation/gazelle_python.yaml +++ b/examples/bzlmod_build_file_generation/gazelle_python.yaml @@ -586,4 +586,3 @@ manifest: yamllint.rules.truthy: yamllint pip_repository: name: pip -integrity: cd25503dc6b3d9e1c5f46715ba2d0499ecc8b3d654ebcbf9f4e52f2074290e0a diff --git a/gazelle/MODULE.bazel b/gazelle/MODULE.bazel index 8c6ad19c7..940a26395 100644 --- a/gazelle/MODULE.bazel +++ b/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") diff --git a/gazelle/README.md b/gazelle/README.md index 0a5b1046b..c1221a601 100644 --- a/gazelle/README.md +++ b/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", ) ``` diff --git a/gazelle/manifest/BUILD.bazel b/gazelle/manifest/BUILD.bazel index fc7fa0963..33b5a4694 100644 --- a/gazelle/manifest/BUILD.bazel +++ b/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"], diff --git a/gazelle/manifest/copy_to_source.py b/gazelle/manifest/copy_to_source.py new file mode 100644 index 000000000..685419233 --- /dev/null +++ b/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 ") + + copy_to_source(Path(sys.argv[1]), Path(sys.argv[2])) diff --git a/gazelle/manifest/defs.bzl b/gazelle/manifest/defs.bzl index ef0f27546..ccabfd299 100644 --- a/gazelle/manifest/defs.bzl +++ b/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], diff --git a/gazelle/manifest/generate/generate.go b/gazelle/manifest/generate/generate.go index bdd0206cc..19ca08a2d 100644 --- a/gazelle/manifest/generate/generate.go +++ b/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 diff --git a/gazelle/manifest/manifest.go b/gazelle/manifest/manifest.go index fb146f943..26b0dfb39 100644 --- a/gazelle/manifest/manifest.go +++ b/gazelle/manifest/manifest.go @@ -31,7 +31,7 @@ type File struct { // Integrity is the hash of the requirements.txt file and the Manifest for // ensuring the integrity of the entire gazelle_python.yaml file. This // controls the testing to keep the gazelle_python.yaml file up-to-date. - Integrity string `yaml:"integrity"` + Integrity string `yaml:"integrity,omitempty"` } // NewFile creates a new File with a given Manifest. @@ -40,12 +40,21 @@ func NewFile(manifest *Manifest) *File { } // Encode encodes the manifest file to the given writer. -func (f *File) Encode(w io.Writer, manifestGeneratorHashFile, requirements io.Reader) error { +func (f *File) EncodeWithIntegrity(w io.Writer, manifestGeneratorHashFile, requirements io.Reader) error { integrityBytes, err := f.calculateIntegrity(manifestGeneratorHashFile, requirements) if err != nil { return fmt.Errorf("failed to encode manifest file: %w", err) } f.Integrity = fmt.Sprintf("%x", integrityBytes) + + return f.encode(w) +} + +func (f *File) EncodeWithoutIntegrity(w io.Writer) error { + return f.encode(w) +} + +func (f *File) encode(w io.Writer) error { encoder := yaml.NewEncoder(w) defer encoder.Close() if err := encoder.Encode(f); err != nil { diff --git a/gazelle/manifest/manifest_test.go b/gazelle/manifest/manifest_test.go index 43c4099aa..274973370 100644 --- a/gazelle/manifest/manifest_test.go +++ b/gazelle/manifest/manifest_test.go @@ -40,7 +40,7 @@ var modulesMapping = manifest.ModulesMapping{ const pipDepsRepositoryName = "test_repository_name" func TestFile(t *testing.T) { - t.Run("Encode", func(t *testing.T) { + t.Run("EncodeWithIntegrity", func(t *testing.T) { f := manifest.NewFile(&manifest.Manifest{ ModulesMapping: modulesMapping, PipDepsRepositoryName: pipDepsRepositoryName, @@ -53,7 +53,7 @@ func TestFile(t *testing.T) { t.FailNow() } defer requirements.Close() - if err := f.Encode(&b, manifestGeneratorHashFile, requirements); err != nil { + if err := f.EncodeWithIntegrity(&b, manifestGeneratorHashFile, requirements); err != nil { log.Println(err) t.FailNow() }