feat(gazelle): Add "include_dep" Python comment annotation #1863

dougthor42 · 2024-04-23T19:25:14Z

Add a new Python comment annotation for Gazelle: include_dep. This
annotation accepts a comma-separated string of values. Values should
be targets names, but no validation is done.

The annotation can be added multiple times, and all values are combined
and de-duplicated.

For python_generation_mode = "package", the include_dep annotations
found across all files included in the generated target.

The parser.annotations struct is updated to include a new includeDep
field, and parser.parse is updated to return the annotations struct. All
target builders then add the resolved dependencies.

Fixes #1862.

Example:

# gazelle:include_dep //foo:bar,:hello_world,//:abc
# gazelle:include_dep //:def,//foo:bar

will cause gazelle to generate:

deps = [
    ":hello_world",
    "//:abc",
    "//:def",
    "//foo:bar",
]

dougthor42 · 2024-04-23T19:25:36Z

Still TODO:

Udpate CHANGELOG
Update documentation

dougthor42 · 2024-04-23T19:33:38Z

gazelle/python/parser.go

+	for i, v := range s.Values() {
+		dedupe[i] = fmt.Sprint(v)
+	}
+	return dedupe


Coming from python, this function feels icky to me. I'd really like to be able to just do

allAnnotations.includeDep = list(set(allAnnotations.includeDep))

above but I couldn't figure out how to do so. The issue is related to types: the treeset.Values() is type []interface{}, treeset.NewWith's values arg is interface{}, but includeDep is []string. From what I could find, there's no easy way to "cast" []string to []interface{} and back.

If you have any suggestions on how to clean things up, I'd be happy to hear them. Otherwise no reply is needed.

I've been meaning to pick up https://github.com/f0rmiga/datanalgo and add the missing data structures using generics. I can try to resurrect that, which would be ideal to use with the Gazelle extension.

dougthor42 · 2024-04-23T19:35:33Z

gazelle/python/parser.go

 	}

-	return modules, mainModules, nil
+	allAnnotations.includeDep = removeDupesFromStringTreeSetSlice(allAnnotations.includeDep)


Do we even want to remove duplicates here? We could just rely on the target builders to handle that.

dougthor42 · 2024-04-23T19:37:01Z

gazelle/python/parser.go

+		for k, v := range annotations.ignore {
+			allAnnotations.ignore[k] = v
+		}


annotations.ignore is not used in generate.go so we could leave this out if wanted. Thoughts? Leaving it out would mean that the returned allAnnotations is not representative of everything that was parsed.

dougthor42 · 2024-04-23T19:37:51Z

gazelle/python/parser.go

@@ -247,12 +272,15 @@ type annotation struct {
 type annotations struct {
 	// The parsed modules to be ignored by Gazelle.
 	ignore map[string]struct{}
+	// Labels that Gazelle should include as deps of the generated target.
+	includeDep []string


Should this be plural? Same elsewhere.

I originally went singular to be consistent with annotations.ignore.

I'd make it plural because it is a list. I don't see a need to make ignore plural, though.

Updated 👍

dougthor42 · 2024-04-23T21:25:21Z

I think this is ready for review.

Note that this might be a bit of a footgun. Users can add non-python targets to deps and then things will fail during bazel build with <target> does not have mandatory providers: 'PyInfo' or 'CcInfo' or 'PyInfo'. The docs I've added try to address this.

If that's deemed too big of risk, I'd be happy to discuss alternative solutions.

aignas · 2024-04-30T14:47:00Z

Don't have time to look into this right now, but after reading your last comment, I am wondering if the right way to avoid the footgun would be to use the import path as an argument to the gazelle comment, but that is almost as a noop import statement using just python? Which puts us back to square 1.

I'll think about this a little more.

gazelle/README.md

f0rmiga · 2024-05-04T18:48:50Z

gazelle/README.md

+This annotation accepts a comma-separated string of values. Values _must_
+be Python targets, but _no validation is done_. If a value is not a Python
+target, building will result in an error saying:
+
+```
+<target> does not have mandatory providers: 'PyInfo' or 'CcInfo' or 'PyInfo'.
+```
+
+Adding non-Python targets to the generated target is a feature request being
+tracked in [Issue #1865](https://github.com/bazelbuild/rules_python/issues/1865).


This is all fine, we can't save the world.

f0rmiga · 2024-05-04T18:53:32Z

gazelle/python/testdata/annotation_include_dep/test.yaml

@@ -0,0 +1,17 @@
+# Copyright 2023 The Bazel Authors. All rights reserved.


Could you please update all the new license headers you are adding?

Suggested change

# Copyright 2023 The Bazel Authors. All rights reserved.

# Copyright 2024 The Bazel Authors. All rights reserved.

f0rmiga · 2024-05-04T18:55:11Z

Don't have time to look into this right now, but after reading your last comment, I am wondering if the right way to avoid the footgun would be to use the import path as an argument to the gazelle comment, but that is almost as a noop import statement using just python? Which puts us back to square 1.

I'll think about this a little more.

We are overthinking here. If the user gets to this point, the entire implementation will look wrong (the user code).

Co-authored-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

…dep-gh1862

dougthor42

I'll think about this a little more.

We are overthinking here. If the user gets to this point, the entire implementation will look wrong (the user code).

👍 we won't try to prevent the user from adding a non-python dependency.

dougthor42 · 2024-05-08T21:21:05Z

gazelle/python/parser.go

@@ -247,12 +272,15 @@ type annotation struct {
 type annotations struct {
 	// The parsed modules to be ignored by Gazelle.
 	ignore map[string]struct{}
+	// Labels that Gazelle should include as deps of the generated target.
+	includeDep []string


Updated 👍

dougthor42 · 2024-05-08T21:23:11Z

gazelle/python/testdata/annotation_include_dep/test.yaml

@@ -0,0 +1,17 @@
+# Copyright 2023 The Bazel Authors. All rights reserved.


f0rmiga · 2024-05-09T01:37:41Z

Thank you, @dougthor42!

The annotation is `include_dep`, not `include_deps`. ```console $ # Before this PR: $ rg -F "include_deps" CHANGELOG.md 76:* (gazelle) Add a new annotation `include_deps`. Also add documentation for $ $ # After this PR, there are no more references to "include_deps" $ rg -F "include_deps" $ ```

dougthor42 added 13 commits April 21, 2024 15:01

Move test

3539d51

Add invalid_annotation_include_dep test

ce9ee9d

Add annotation_include_dep test

f2e42ad

Add includeDep to the annotations struct but we don't use it yet.

fbc02f3

Have parser.parse return a 4-tuple that includes the parsed annotations.

0fe5750

Add target.addResolvedDependencies

eccf7bd

Actually use the returned annotations

92adfc1

Correctly return the annotations

cdf2a49

git things working

975011b

Finish up the deduping; format stuff

4cbfecd

rename variables

5ad8471

factor out a function

ae8e200

update test: check deduping

b5f3bf9

Apply forgotten stash in testdata README

6f0b7eb

dougthor42 commented Apr 23, 2024

View reviewed changes

dougthor42 added 6 commits April 23, 2024 13:02

Update docs

4d15f7d

Update CHANGELOG

a00b5da

fix title in docs

8832835

Merge branch 'main' into annotation-include-dep-gh1862

8da48cb

Fix failing test

5a5a563

Update README.md

ac6151e

dougthor42 marked this pull request as ready for review April 23, 2024 21:25

dougthor42 requested review from f0rmiga and rickeylev as code owners April 23, 2024 21:25

Update readme with link to 'data' feature request

98c61fb

f0rmiga reviewed May 4, 2024

View reviewed changes

dougthor42 and others added 5 commits May 8, 2024 14:17

Update gazelle/README.md

8c68bdb

Co-authored-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Update gazelle/README.md

cb650d7

Co-authored-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

s/includeDep/includeDeps/g

43f82a8

Correct copyright year

ba2ed39

Merge remote-tracking branch 'upstream/main' into annotation-include-…

090c178

…dep-gh1862

dougthor42 commented May 8, 2024

View reviewed changes

format

b1bfd1a

f0rmiga approved these changes May 9, 2024

View reviewed changes

f0rmiga added this pull request to the merge queue May 9, 2024

Merged via the queue into bazelbuild:main with commit a1d3c0d May 9, 2024
4 checks passed

dougthor42 deleted the annotation-include-dep-gh1862 branch May 9, 2024 03:07

dougthor42 added a commit to dougthor42/rules_python that referenced this pull request May 9, 2024

Fix typo in changelog text from bazelbuild#1863

1f2e512

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gazelle): Add "include_dep" Python comment annotation #1863

feat(gazelle): Add "include_dep" Python comment annotation #1863

dougthor42 commented Apr 23, 2024

dougthor42 commented Apr 23, 2024 •

edited

dougthor42 Apr 23, 2024

f0rmiga May 4, 2024

dougthor42 Apr 23, 2024

dougthor42 Apr 23, 2024

dougthor42 Apr 23, 2024

f0rmiga May 4, 2024

dougthor42 May 8, 2024

dougthor42 commented Apr 23, 2024

aignas commented Apr 30, 2024

f0rmiga May 4, 2024

f0rmiga May 4, 2024

dougthor42 May 8, 2024

f0rmiga commented May 4, 2024 •

edited

dougthor42 left a comment

dougthor42 May 8, 2024

dougthor42 May 8, 2024

f0rmiga commented May 9, 2024

		@@ -0,0 +1,17 @@
		# Copyright 2023 The Bazel Authors. All rights reserved.

	# Copyright 2023 The Bazel Authors. All rights reserved.
	# Copyright 2024 The Bazel Authors. All rights reserved.

feat(gazelle): Add "include_dep" Python comment annotation #1863

feat(gazelle): Add "include_dep" Python comment annotation #1863

Conversation

dougthor42 commented Apr 23, 2024

dougthor42 commented Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougthor42 commented Apr 23, 2024

aignas commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0rmiga commented May 4, 2024 • edited

dougthor42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0rmiga commented May 9, 2024

dougthor42 commented Apr 23, 2024 •

edited

f0rmiga commented May 4, 2024 •

edited