Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 27 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
3539d51
Move test
dougthor42 Apr 21, 2024
ce9ee9d
Add invalid_annotation_include_dep test
dougthor42 Apr 21, 2024
f2e42ad
Add annotation_include_dep test
dougthor42 Apr 21, 2024
fbc02f3
Add includeDep to the annotations struct but we don't use it yet.
dougthor42 Apr 21, 2024
0fe5750
Have parser.parse return a 4-tuple that includes the parsed annotations.
dougthor42 Apr 21, 2024
eccf7bd
Add target.addResolvedDependencies
dougthor42 Apr 21, 2024
92adfc1
Actually use the returned annotations
dougthor42 Apr 22, 2024
cdf2a49
Correctly return the annotations
dougthor42 Apr 22, 2024
975011b
git things working
dougthor42 Apr 23, 2024
4cbfecd
Finish up the deduping; format stuff
dougthor42 Apr 23, 2024
5ad8471
rename variables
dougthor42 Apr 23, 2024
ae8e200
factor out a function
dougthor42 Apr 23, 2024
b5f3bf9
update test: check deduping
dougthor42 Apr 23, 2024
6f0b7eb
Apply forgotten stash in testdata README
dougthor42 Apr 23, 2024
4d15f7d
Update docs
dougthor42 Apr 23, 2024
a00b5da
Update CHANGELOG
dougthor42 Apr 23, 2024
8832835
fix title in docs
dougthor42 Apr 23, 2024
8da48cb
Merge branch 'main' into annotation-include-dep-gh1862
dougthor42 Apr 23, 2024
5a5a563
Fix failing test
dougthor42 Apr 23, 2024
ac6151e
Update README.md
dougthor42 Apr 23, 2024
98c61fb
Update readme with link to 'data' feature request
dougthor42 Apr 23, 2024
8c68bdb
Update gazelle/README.md
dougthor42 May 8, 2024
cb650d7
Update gazelle/README.md
dougthor42 May 8, 2024
43f82a8
s/includeDep/includeDeps/g
dougthor42 May 8, 2024
ba2ed39
Correct copyright year
dougthor42 May 8, 2024
090c178
Merge remote-tracking branch 'upstream/main' into annotation-include-…
dougthor42 May 8, 2024
b1bfd1a
format
dougthor42 May 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ A brief description of the categories of changes:
the whl and sdist files will be written to the lock file. Controlling whether
the downloading of metadata is done in parallel can be done using
`parallel_download` attribute.
* (gazelle) Add a new annotation `include_deps`. Also add documentation for
annotations to `gazelle/README.md`.

[0.XX.0]: https://github.com/bazelbuild/rules_python/releases/tag/0.XX.0
[python_default_visibility]: gazelle/README.md#directive-python_default_visibility
Expand Down
102 changes: 102 additions & 0 deletions gazelle/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,108 @@ py_library(
[issue-1826]: https://github.com/bazelbuild/rules_python/issues/1826


### Annotations

*Annotations* refer to comments found _within Python files_ that configure how
Gazelle acts for that particular file.

Annotations have the form:

```python
# gazelle:annotation_name value
```

and can reside anywhere within a Python file where comments are valid. For example:

```python
import foo
# gazelle:annotation_name value

def bar(): # gazelle:annotation_name value
pass
```

The annotations are:

| **Annotation** | **Default value** |
|---------------------------------------------------------------|-------------------|
| [`# gazelle:ignore imports`](#annotation-ignore) | N/A |
| Tells Gazelle to ignore import statements. `imports` is a comma-separated list of imports to ignore. | |
| [`# gazelle:include_dep targets`](#annotation-include_dep) | N/A |
| Tells Gazelle to include a set of dependencies, even if they are not imported in a Python module. `targets` is a comma-separated list of target names to include as dependencies. | |


#### Annotation: `ignore`

This annotation accepts a comma-separated string of values. Values are names of Python
imports that Gazelle should _not_ include in target dependencies.

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

For `python_generation_mode = "package"`, the `ignore` annotations
found across all files included in the generated target are removed from `deps`.

Example:

```python
import numpy # a pypi package

# gazelle:ignore bar.baz.hello,foo
import bar.baz.hello
import foo

# Ignore this import because _reasons_
import baz # gazelle:ignore baz
```

will cause Gazelle to generate:

```starlark
deps = ["@pypi//numpy"],
```


#### Annotation: `include_dep`

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).
Comment on lines +492 to +501
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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 are included in `deps`.

Example:

```python
# gazelle:include_dep //foo:bar,:hello_world,//:abc
# gazelle:include_dep //:def,//foo:bar
import numpy # a pypi package
```

will cause Gazelle to generate:

```starlark
deps = [
":hello_world",
"//:abc",
"//:def",
"//foo:bar",
"@pypi//numpy",
]
```


### Libraries

Python source files are those ending in `.py` but not ending in `_test.py`.
Expand Down
13 changes: 9 additions & 4 deletions gazelle/python/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
collisionErrors := singlylinkedlist.New()

appendPyLibrary := func(srcs *treeset.Set, pyLibraryTargetName string) {
allDeps, mainModules, err := parser.parse(srcs)
allDeps, mainModules, annotations, err := parser.parse(srcs)
if err != nil {
log.Fatalf("ERROR: %v\n", err)
}
Expand Down Expand Up @@ -263,6 +263,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
addVisibility(visibility).
addSrc(filename).
addModuleDependencies(mainModules[filename]).
addResolvedDependencies(annotations.includeDep).
generateImportsAttribute().build()
result.Gen = append(result.Gen, pyBinary)
result.Imports = append(result.Imports, pyBinary.PrivateAttr(config.GazelleImportsKey))
Expand Down Expand Up @@ -290,6 +291,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
addVisibility(visibility).
addSrcs(srcs).
addModuleDependencies(allDeps).
addResolvedDependencies(annotations.includeDep).
generateImportsAttribute().
build()

Expand All @@ -314,7 +316,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
}

if hasPyBinaryEntryPointFile {
deps, _, err := parser.parseSingle(pyBinaryEntrypointFilename)
deps, _, annotations, err := parser.parseSingle(pyBinaryEntrypointFilename)
if err != nil {
log.Fatalf("ERROR: %v\n", err)
}
Expand All @@ -338,6 +340,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
addVisibility(visibility).
addSrc(pyBinaryEntrypointFilename).
addModuleDependencies(deps).
addResolvedDependencies(annotations.includeDep).
generateImportsAttribute()

pyBinary := pyBinaryTarget.build()
Expand All @@ -348,7 +351,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes

var conftest *rule.Rule
if hasConftestFile {
deps, _, err := parser.parseSingle(conftestFilename)
deps, _, annotations, err := parser.parseSingle(conftestFilename)
if err != nil {
log.Fatalf("ERROR: %v\n", err)
}
Expand All @@ -367,6 +370,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
conftestTarget := newTargetBuilder(pyLibraryKind, conftestTargetname, pythonProjectRoot, args.Rel, pyFileNames).
addSrc(conftestFilename).
addModuleDependencies(deps).
addResolvedDependencies(annotations.includeDep).
addVisibility(visibility).
setTestonly().
generateImportsAttribute()
Expand All @@ -379,7 +383,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes

var pyTestTargets []*targetBuilder
newPyTestTargetBuilder := func(srcs *treeset.Set, pyTestTargetName string) *targetBuilder {
deps, _, err := parser.parse(srcs)
deps, _, annotations, err := parser.parse(srcs)
if err != nil {
log.Fatalf("ERROR: %v\n", err)
}
Expand All @@ -397,6 +401,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
return newTargetBuilder(pyTestKind, pyTestTargetName, pythonProjectRoot, args.Rel, pyFileNames).
addSrcs(srcs).
addModuleDependencies(deps).
addResolvedDependencies(annotations.includeDep).
generateImportsAttribute()
}
if (hasPyTestEntryPointFile || hasPyTestEntryPointTarget || cfg.CoarseGrainedGeneration()) && !cfg.PerFileGeneration() {
Expand Down
58 changes: 49 additions & 9 deletions gazelle/python/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,15 @@ func newPython3Parser(

// parseSingle parses a single Python file and returns the extracted modules
// from the import statements as well as the parsed comments.
func (p *python3Parser) parseSingle(pyFilename string) (*treeset.Set, map[string]*treeset.Set, error) {
func (p *python3Parser) parseSingle(pyFilename string) (*treeset.Set, map[string]*treeset.Set, *annotations, error) {
pyFilenames := treeset.NewWith(godsutils.StringComparator)
pyFilenames.Add(pyFilename)
return p.parse(pyFilenames)
}

// parse parses multiple Python files and returns the extracted modules from
// the import statements as well as the parsed comments.
func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[string]*treeset.Set, error) {
func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[string]*treeset.Set, *annotations, error) {
parserMutex.Lock()
defer parserMutex.Unlock()

Expand All @@ -122,28 +122,30 @@ func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[strin
}
encoder := json.NewEncoder(parserStdin)
if err := encoder.Encode(&req); err != nil {
return nil, nil, fmt.Errorf("failed to parse: %w", err)
return nil, nil, nil, fmt.Errorf("failed to parse: %w", err)
}

reader := bufio.NewReader(parserStdout)
data, err := reader.ReadBytes(0)
if err != nil {
return nil, nil, fmt.Errorf("failed to parse: %w", err)
return nil, nil, nil, fmt.Errorf("failed to parse: %w", err)
}
data = data[:len(data)-1]
var allRes []parserResponse
if err := json.Unmarshal(data, &allRes); err != nil {
return nil, nil, fmt.Errorf("failed to parse: %w", err)
return nil, nil, nil, fmt.Errorf("failed to parse: %w", err)
}

mainModules := make(map[string]*treeset.Set, len(allRes))
allAnnotations := new(annotations)
allAnnotations.ignore = make(map[string]struct{})
for _, res := range allRes {
if res.HasMain {
mainModules[res.FileName] = treeset.NewWith(moduleComparator)
}
annotations, err := annotationsFromComments(res.Comments)
if err != nil {
return nil, nil, fmt.Errorf("failed to parse annotations: %w", err)
return nil, nil, nil, fmt.Errorf("failed to parse annotations: %w", err)
}

for _, m := range res.Modules {
Expand All @@ -164,9 +166,32 @@ func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[strin
mainModules[res.FileName].Add(m)
}
}

// Collect all annotations from each file into a single annotations struct.
for k, v := range annotations.ignore {
allAnnotations.ignore[k] = v
}
Comment on lines +171 to +173
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

allAnnotations.includeDep = append(allAnnotations.includeDep, annotations.includeDep...)
}

return modules, mainModules, nil
allAnnotations.includeDep = removeDupesFromStringTreeSetSlice(allAnnotations.includeDep)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return modules, mainModules, allAnnotations, nil
}

// removeDupesFromStringTreeSetSlice takes a []string, makes a set out of the
// elements, and then returns a new []string with all duplicates removed. Order
// is preserved.
func removeDupesFromStringTreeSetSlice(array []string) []string {
s := treeset.NewWith(godsutils.StringComparator)
for _, v := range array {
s.Add(v)
}
dedupe := make([]string, s.Size())
for i, v := range s.Values() {
dedupe[i] = fmt.Sprint(v)
}
return dedupe
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// parserResponse represents a response returned by the parser.py for a given
Expand Down Expand Up @@ -211,7 +236,8 @@ const (
// The Gazelle annotation prefix.
annotationPrefix string = "gazelle:"
// The ignore annotation kind. E.g. '# gazelle:ignore <module_name>'.
annotationKindIgnore annotationKind = "ignore"
annotationKindIgnore annotationKind = "ignore"
annotationKindIncludeDep annotationKind = "include_dep"
)

// comment represents a Python comment.
Expand Down Expand Up @@ -247,12 +273,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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be plural? Same elsewhere.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 👍

}

// annotationsFromComments returns all the annotations parsed out of the
// comments of a Python module.
func annotationsFromComments(comments []comment) (*annotations, error) {
ignore := make(map[string]struct{})
includeDep := []string{}
for _, comment := range comments {
annotation, err := comment.asAnnotation()
if err != nil {
Expand All @@ -269,10 +298,21 @@ func annotationsFromComments(comments []comment) (*annotations, error) {
ignore[m] = struct{}{}
}
}
if annotation.kind == annotationKindIncludeDep {
targets := strings.Split(annotation.value, ",")
for _, t := range targets {
if t == "" {
continue
}
t = strings.TrimSpace(t)
includeDep = append(includeDep, t)
}
}
}
}
return &annotations{
ignore: ignore,
ignore: ignore,
includeDep: includeDep,
}, nil
}

Expand Down
9 changes: 9 additions & 0 deletions gazelle/python/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,15 @@ func (t *targetBuilder) addResolvedDependency(dep string) *targetBuilder {
return t
}

// addResolvedDependencies adds multiple dependencies, that have already been
// resolved or generated, to the target.
func (t *targetBuilder) addResolvedDependencies(deps []string) *targetBuilder {
for _, dep := range deps {
t.addResolvedDependency(dep)
}
return t
}

// addVisibility adds visibility labels to the target.
func (t *targetBuilder) addVisibility(visibility []string) *targetBuilder {
for _, item := range visibility {
Expand Down
1 change: 1 addition & 0 deletions gazelle/python/testdata/annotation_include_dep/BUILD.in
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_generation_mode file