8000 feat(gazelle): Remove integrity field from Gazelle manifest (#1666) · emfdavid/rules_python@677fb53 · GitHub
[go: up one dir, main page]
Skip to content

Commit 677fb53

Browse files
adzenithadzenith
authored
feat(gazelle): Remove integrity field from Gazelle manifest (bazel-contrib#1666)
As per the discussion in bazel-contrib#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`.
1 parent 74d576f commit 677fb53

File tree

12 files changed

+161
-89
lines changed

12 files changed

+161
-89
lines changed

‎CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,10 @@ A brief description of the categories of changes:
164164
target for each file with `if __name__ == "__main__"` instead of just one
165165
`py_binary` for the whole module.
166166

167+
* (gazelle) the Gazelle manifest integrity field is now optional. If the
168+
`requirements` argument to `gazelle_python_manifest` is unset, no integrity
169+
field will be generated.
170+
167171
### Fixed
168172

169173
* (gazelle) The gazelle plugin helper was not working with Python toolchains 3.11

‎examples/bzlmod_build_file_generation/.bazelversion

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
6.0.0
1+
6.4.0

‎examples/bzlmod_build_file_generation/BUILD.bazel

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ modules_mapping(
2929
exclude_patterns = [
3030
"^_|(\\._)+", # This is the default.
3131
"(\\.tests)+", # Add a custom one to get rid of the psutil tests.
32+
"^colorama", # Get rid of colorama on Windows.
33+
"^lazy_object_proxy\\.cext$", # Get rid of this on Linux because it isn't included on Windows.
3234
],
3335
wheels = all_whl_requirements,
3436
)
@@ -47,10 +49,6 @@ gazelle_python_manifest(
4749
name = "gazelle_python_manifest",
4850
modules_mapping = ":modules_map",
4951
pip_repository_name = "pip",
50-
requirements = [
51-
"//:requirements_lock.txt",
52-
"//:requirements_windows.txt",
53-
],
5452
tags = ["exclusive"],
5553
)
5654

‎examples/bzlmod_build_file_generation/gazelle_python.yaml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -586,4 +586,3 @@ manifest:
586586
yamllint.rules.truthy: yamllint
587587
pip_repository:
588588
name: pip
589-
integrity: cd25503dc6b3d9e1c5f46715ba2d0499ecc8b3d654ebcbf9f4e52f2074290e0a

‎gazelle/MODULE.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ module(
44
compatibility_level = 1,
55
)
66

7+
bazel_dep(name = "bazel_skylib", version = "1.5.0")
78
bazel_dep(name = "rules_python", version = "0.18.0")
89
bazel_dep(name = "rules_go", version = "0.41.0", repo_name = "io_bazel_rules_go")
910
bazel_dep(name = "gazelle", version = "0.33.0", repo_name = "bazel_gazelle")

‎gazelle/README.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,10 @@ gazelle_python_manifest(
114114
pip_repository_name = "pip",
115115
# This should point to wherever we declare our python dependencies
116116
# (the same as what we passed to the modules_mapping rule in WORKSPACE)
117+
# This argument is optional. If provided, the `.test` target is very
118+
# fast because it just has to check an integrity field. If not provided,
119+
# the integrity field is not added to the manifest which can help avoid
120+
# merge conflicts in large repos.
117121
requirements = "//:requirements_lock.txt",
118122
)
119123
```

‎gazelle/manifest/BUILD.bazel

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
22

3+
exports_files([
4+
# This gets wrapped up into a py_binary with args inside of the gazelle_python_manifest macro.
5+
"copy_to_source.py",
6+
])
7+
38
go_library(
49
name = "manifest",
510
srcs = ["manifest.go"],

‎gazelle/manifest/copy_to_source.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
"""Copy a generated file to the source tree.
2+
3+
Run like:
4+
copy_to_source path/to/generated_file path/to/source_file_to_overwrite
5+
"""
6+
7+
import os
8+
import shutil
9+
import stat
10+
import sys
11+
from pathlib import Path
12+
13+
14+
def copy_to_source(generated_relative_path: Path, target_relative_path: Path) -> None:
15+
"""Copy the generated file to the target file path.
16+
17+
Expands the relative paths by looking at Bazel env vars to figure out which absolute paths to use.
18+
"""
19+
# This script normally gets executed from the runfiles dir, so find the absolute path to the generated file based on that.
20+
generated_absolute_path = Path.cwd() / generated_relative_path
21+
22+
# Similarly, the target is relative to the source directory.
23+
target_absolute_path = os.getenv("BUILD_WORKSPACE_DIRECTORY") / target_relative_path
24+
25+
print(f"Copying {generated_absolute_path} to {target_absolute_path}")
26+
target_absolute_path.parent.mkdir(parents=True, exist_ok=True)
27+
shutil.copy(generated_absolute_path, target_absolute_path)
28+
29+
target_absolute_path.chmod(0O664)
30+
31+
32+
if __name__ == "__main__":
33+
if len(sys.argv) != 3:
34+
sys.exit("Usage: copy_to_source <generated_file> <target_file>")
35+
36+
copy_to_source(Path(sys.argv[1]), Path(sys.argv[2]))

‎gazelle/manifest/defs.bzl

Lines changed: 72 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,14 @@
1616
for updating and testing the Gazelle manifest file.
1717
"""
1818

19-
load("@io_bazel_rules_go//go:def.bzl", "GoSource", "go_binary", "go_test")
19+
load("@bazel_skylib//rules:diff_test.bzl", "diff_test")
20+
load("@io_bazel_rules_go//go:def.bzl", "GoSource", "go_test")
21+
load("@rules_python//python:defs.bzl", "py_binary")
2022

2123
def gazelle_python_manifest(
2224
name,
23-
requirements,
2425
modules_mapping,
26+
requirements = [],
2527
pip_repository_name = "",
2628
pip_deps_repository_name = "",
2729
manifest = ":gazelle_python.yaml",
@@ -30,15 +32,18 @@ def gazelle_python_manifest(
3032
3133
Args:
3234
name: the name used as a base for the targets.
35+
modules_mapping: the target for the generated modules_mapping.json file.
3336
requirements: the target for the requirements.txt file or a list of
3437
requirements files that will be concatenated before passing on to
35-
the manifest generator.
38+
the manifest generator. If unset, no integrity field is added to the
39+
manifest, meaning testing it is just as expensive as generating it,
40+
but modifying it is much less likely to result in a merge conflict.
3641
pip_repository_name: the name of the pip_install or pip_repository target.
3742
pip_deps_repository_name: deprecated - the old pip_install target name.
38-
modules_mapping: the target for the generated modules_mapping.json file.
39-
manifest: the target for the Gazelle manifest file.
40-
**kwargs: other bazel attributes passed to the target target generated by
41-
this macro.
43+
manifest: the Gazelle manifest file.
44+
defaults to the same value as manifest.
45+
**kwargs: other bazel attributes passed to the generate and test targets
46+
generated by this macro.
4247
"""
4348
if pip_deps_repository_name != "":
4449
# buildifier: disable=print
@@ -52,12 +57,17 @@ def gazelle_python_manifest(
5257
# This is a temporary check while pip_deps_repository_name exists as deprecated.
5358
fail("pip_repository_name must be set in //{}:{}".format(native.package_name(), name))
5459

60+
test_target = "{}.test".format(name)
5561
update_target = "{}.update".format(name)
5662
update_target_label = "//{}:{}".format(native.package_name(), update_target)
5763

64+
manifest_genrule = name + ".genrule"
65+
generated_manifest = name + ".generated_manifest"
66+
manifest_generator = Label("//manifest/generate:generate")
5867
manifest_generator_hash = Label("//manifest/generate:generate_lib_sources_hash")
5968

60-
if type(requirements) == "list":
69+
if requirements and type(requirements) == "list":
70+
# This runs if requirements is a list or is unset (default value is empty list)
6171
native.genrule(
6272
name = name + "_requirements_gen",
6373
srcs = sorted(requirements),
@@ -68,56 +78,71 @@ def gazelle_python_manifest(
6878
requirements = name + "_requirements_gen"
6979

7080
update_args = [
71-
"--manifest-generator-hash",
72-
"$(rootpath {})".format(manifest_generator_hash),
73-
"--requirements",
74-
"$(rootpath {})".format(requirements),
75-
"--pip-repository-name",
76-
pip_repository_name,
77-
"--modules-mapping",
78-
"$(rootpath {})".format(modules_mapping),
79-
"--output",
80-
"$(rootpath {})".format(manifest),
81-
"--update-target",
82-
update_target_label,
81+
"--manifest-generator-hash=$(execpath {})".format(manifest_generator_hash),
82+
"--requirements=$(rootpath {})".format(requirements) if requirements else "--requirements=",
83+
"--pip-repository-name={}".format(pip_repository_name),
84+
"--modules-mapping=$(execpath {})".format(modules_mapping),
85+
"--output=$(execpath {})".format(generated_manifest),
86+
"--update-target={}".format(update_target_label),
8387
]
8488

85-
go_binary(
86-
name = update_target,
87-
embed = [Label("//manifest/generate:generate_lib")],
88-
data = [
89-
manifest,
89+
native.genrule(
90+
name = manifest_genrule,
91+
outs = [generated_manifest],
92+
cmd = "$(execpath {}) {}".format(manifest_generator, " ".join(update_args)),
93+
tools = [manifest_generator],
94+
srcs = [
9095
modules_mapping,
91-
requirements,
9296
manifest_generator_hash,
93-
],
94-
args = update_args,
95-
visibility = ["//visibility:private"],
96-
tags = ["manual"],
97+
] + ([requirements] if requirements else []),
9798
)
9899

99-
attrs = {
100-
"env": {
101-
"_TEST_MANIFEST": "$(rootpath {})".format(manifest),
102-
"_TEST_MANIFEST_GENERATOR_HASH": "$(rootpath {})".format(manifest_generator_hash),
103-
"_TEST_REQUIREMENTS": "$(rootpath {})".format(requirements),
104-
},
105-
"size": "small",
106-
}
107-
go_test(
108-
name = "{}.test".format(name),
109-
srcs = [Label("//manifest/test:test.go")],
100+
py_binary(
101+
name = update_target,
102+
srcs = [Label("//manifest:copy_to_source.py")],
103+
main = Label("//manifest:copy_to_source.py"),
104+
args = [
105+
"$(rootpath {})".format(generated_manifest),
106+
"$(rootpath {})".format(manifest),
107+
],
110108
data = [
109+
generated_manifest,
111110
manifest,
112-
requirements,
113-
manifest_generator_hash,
114111
],
115-
rundir = ".",
116-
deps = [Label("//manifest")],
117-
# kwargs could contain test-specific attributes like size or timeout
118-
**dict(attrs, **kwargs)
112+
**kwargs
119113
)
120114

115+
if requirements:
116+
attrs = {
117+
"env": {
118+
"_TEST_MANIFEST": "$(rootpath {})".format(manifest),
119+
"_TEST_MANIFEST_GENERATOR_HASH": "$(rootpath {})".format(manifest_generator_hash),
120+
"_TEST_REQUIREMENTS": "$(rootpath {})".format(requirements),
121+
},
122+
"size": "small",
123+
}
124+
go_test(
125+
name = test_target,
126+
srcs = [Label("//manifest/test:test.go")],
127+
data = [
128+
manifest,
129+
requirements,
130+
manifest_generator_hash,
131+
],
132+
rundir = ".",
133+
deps = [Label("//manifest")],
134+
# kwargs could contain test-specific attributes like size or timeout
135+
**dict(attrs, **kwargs)
136+
)
137+
else:
138+
diff_test(
139+
name = test_target,
140+
file1 = generated_manifest,
141+
file2 = manifest,
142+
failure_message = "Gazelle manifest is out of date. Run 'bazel run {}' to update it.".format(native.package_relative_label(update_target)),
143+
**kwargs
144+
)
145+
121146
native.filegroup(
122147
name = name,
123148
srcs = [manifest],

‎gazelle/manifest/generate/generate.go

Lines changed: 23 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,6 @@ import (
3131
"github.com/bazelbuild/rules_python/gazelle/manifest"
3232
)
3333

34-
func init() {
35-
if os.Getenv("BUILD_WORKSPACE_DIRECTORY") == "" {
36-
log.Fatalln("ERROR: this program must run under Bazel")
37-
}
38-
}
39-
4034
func main() {
4135
var (
4236
manifestGeneratorHashPath string
@@ -79,10 +73,6 @@ func main() {
7973
"The Bazel target to update the YAML manifest file.")
8074
flag.Parse()
8175

82-
if requirementsPath == "" {
83-
log.Fatalln("ERROR: --requirements must be set")
84-
}
85-
8676
if modulesMappingPath == "" {
8777
log.Fatalln("ERROR: --modules-mapping must be set")
8878
}
@@ -102,12 +92,12 @@ func main() {
10292

10393
header := generateHeader(updateTarget)
10494
repository := manifest.PipRepository{
105-
Name: pipRepositoryName,
95+
Name: pipRepositoryName,
10696
}
10797

10898
manifestFile := manifest.NewFile(&manifest.Manifest{
10999
ModulesMapping: modulesMapping,
110-
PipRepository: &repository,
100+
PipRepository: &repository,
111101
})
112102
if err := writeOutput(
113103
outputPath,
@@ -155,12 +145,7 @@ func writeOutput(
155145
manifestGeneratorHashPath string,
156146
requirementsPath string,
157147
) error {
158-
stat, err := os.Stat(outputPath)
159-
if err != nil {
160-
return fmt.Errorf("failed to write output: %w", err)
161-
}
162-
163-
outputFile, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_TRUNC, stat.Mode())
148+
outputFile, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0644)
164149
if err != nil {
165150
return fmt.Errorf("failed to write output: %w", err)
166151
}
@@ -170,20 +155,26 @@ func writeOutput(
170155
return fmt.Errorf("failed to write output: %w", err)
171156
}
172157

173-
manifestGeneratorHash, err := os.Open(manifestGeneratorHashPath)
174-
if err != nil {
175-
return fmt.Errorf("failed to write output: %w", err)
176-
}
177-
defer manifestGeneratorHash.Close()
178-
179-
requirements, err := os.Open(requirementsPath)
180-
if err != nil {
181-
return fmt.Errorf("failed to write output: %w", err)
182-
}
183-
defer requirements.Close()
184-
185-
if err := manifestFile.Encode(outputFile, manifestGeneratorHash, requirements); err != nil {
186-
return fmt.Errorf("failed to write output: %w", err)
158+
if requirementsPath != "" {
159+
manifestGeneratorHash, err := os.Open(manifestGeneratorHashPath)
160+
if err != nil {
161+
return fmt.Errorf("failed to write output: %w", err)
162+
}
163+
defer manifestGeneratorHash.Close()
164+
165+
requirements, err := os.Open(requirementsPath)
166+
if err != nil {
167+
return fmt.Errorf("failed to write output: %w", err)
168+
}
169+
defer requirements.Close()
170+
171+
if err := manifestFile.EncodeWithIntegrity(outputFile, manifestGeneratorHash, requirements); err != nil {
172+
return fmt.Errorf("failed to write output: %w", err)
173+
}
174+
} else {
175+
if err := manifestFile.EncodeWithoutIntegrity(outputFile); err != nil {
176+
return fmt.Errorf("failed to write output: %w", err)
177+
}
187178
}
188179

189180
return nil

0 commit comments

Comments
 (0)
0