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): Update to latest version of go-tree-sitter #2413

Closed
wants to merge 3 commits into from

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Nov 15, 2024

The latest version of go-tree-sitter includes grammer changes for python 3.12. To use it, we need to patch the build files generated by the go_repository to support building with uplevel files references. This should fix #2396.

@maffoo maffoo changed the title feat: Update to latest version of go-tree-sitter feat(gazelle): Update to latest version of go-tree-sitter Nov 15, 2024
Copy link
Collaborator

@dougthor42 dougthor42 left a comment

Choose a reason for hiding this comment

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

Hmm... still seems to not work on our pyle repo. I'll have more time to investigate early next week.

Please add a test case for python 3.12 syntax to gazelle/python/testdata. Specifically, we want to make sure that imports, gazelle annotations, and if __name__ that come after py3.12-only syntax are still parsed. For example:

# my_baz.py
import foo
# gazelle:include_dep //:bar    

def baz[T]() -> T:  # 3.12-only syntax
    pass

# these still get parsed
import foobar
# gazelle:include_dep //:hello

if __name__ == "__main__":
    pass

Should generate (assuming per-file target):

py_binary(
    name = "my_baz",
    srcs = ["my_baz.py"],
    deps = [
        "//:bar",
        "//:foo",
        "//:foobar",
        "//:hello",
    ],
)

If you only get 2 deps or a py_library, then the parsing still failed 😢

CHANGELOG.md Outdated
@@ -52,7 +52,7 @@ Unreleased changes template.

{#v0-0-0-changed}
### Changed
* Nothing yet.
* (gazelle): Update to latest version of go-tree-sitter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please include that this adds support for python 3.12 grammar (and 3.13?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: please move to a "patches" directory

@hunshcn
Copy link
Contributor

hunshcn commented Nov 16, 2024

maybe we can publish go-tree-sitter with patches to bazel registry? like https://registry.bazel.build/modules/circl

github-merge-queue bot pushed a commit that referenced this pull request Nov 18, 2024
While investigating #2396 and why #2413 doesn't appear to be working for
us, I realized that one of the things I was making heavy use of was
additional parser logging that I had added. This adds some of that
logging. I also throw in some documentation because I found it helpful.

Users can (attempt to) get additional parse failure information by
setting the `GAZELLE_VERBOSE` environment variable to `1`. Eg:

```console
$ GAZELLE_VERBOSE=1 bazel run //:gazelle
```

Here are some example logs:

```console
$ GAZELLE_VERBOSE=1 bazel run //:gazelle
INFO: Invocation ID: a4e026d8-17df-426c-b1cc-d3980690dd53
...
INFO: Running command line: bazel-bin/gazelle
INFO: Streaming build results to: https://btx.cloud.google.com/invocations/a4e026d8-17df-426c-b1cc-d3980690dd53
gazelle: WARNING: failed to parse "hello/get_deps.py". The resulting BUILD target may be incorrect.
gazelle: Parse error at {Row:1 Column:0}:
def search_one_more_level[T]():
gazelle: The above was parsed as: (ERROR (identifier) (call function: (list (identifier)) arguments: (argument_list)))
gazelle: ERROR: failed to generate target "//hello:get_deps" of kind "py_library": a target of kind "pyle_py_binary" with the same name already exists. Use the '# gazelle:python_library_naming_convention' directive to change the naming convention.
$
$ bazel run //:gazelle
INFO: Invocation ID: 726c9fd6-f566-4c30-95ef-c4781ad155de
...
INFO: Running command line: bazel-bin/gazelle
INFO: Streaming build results to: https://btx.cloud.google.com/invocations/726c9fd6-f566-4c30-95ef-c4781ad155de
gazelle: WARNING: failed to parse "hello/get_deps.py". The resulting BUILD target may be incorrect.
gazelle: ERROR: failed to generate target "//hello:get_deps" of kind "py_library": a target of kind "pyle_py_binary" with the same name already exists. Use the '# gazelle:python_library_naming_convention' directive to change the naming convention.
```

---------

Co-authored-by: Richard Levasseur <richardlev@gmail.com>
Co-authored-by: Richard Levasseur <rlevasseur@google.com>
Include a patch to the build files generated by the go_repository to
support building with uplevel files references.
@maffoo
Copy link
Contributor Author

maffoo commented Nov 20, 2024

I think I got things working now. Had to remove go-tree-sitter from go.mod because this seems to override the custom go_respository definition and results in patches not being applied to go-tree-sitter before building. I'm not sure whether we still need go.mod for other dependencies or if they can be handled by the go_repository definitions in deps.bzl too; experts please weight in.

I also moved the patch file into a patches subdirectory and added a test case exercising the py3.12 syntax that fails on main but succeeds on this branch. I tried to create a minimal failing test case based on our internal code; interestingly, I found that the function docstring is key to getting this to fail on main, no idea why that is the case...

@maffoo maffoo requested a review from dougthor42 November 20, 2024 05:13
@dougthor42
Copy link
Collaborator

Just relaying info that @maffoo and I had at work:

This still doesn't work for pyle:

$ bazel run //:gazelle src/pyle/tools
INFO: Invocation ID: e06d2fbb-5642-4159-9abf-f1402367d298
INFO: Streaming build results to: https://btx.cloud.google.com/invocations/e06d2fbb-5642-4159-9abf-f1402367d298
WARNING: For repository 'bazel_skylib', the root module requires module version bazel_skylib@1.6.1, but got bazel_skylib@1.7.1 in the resolved dependency graph.
ERROR: no such package '@@[unknown repo 'com_github_smacker_go_tree_sitter' requested from @@rules_python_gazelle_plugin~]//python': The repository '@@[unknown repo 'com_github_smacker_go_tree_sitter' requested from @@rules_python_gazelle_plugin~]' could not be resolved: No repository visible as '@com_github_smacker_go_tree_sitter' from repository '@@rules_python_gazelle_plugin~'
ERROR: /usr/local/google/home/dthor/.cache/bazel/_bazel_dthor/dbe74c4144b5c9a438d84a119652bef9/external/rules_python_gazelle_plugin~/python/BUILD.bazel:6:11: no such package '@@[unknown repo 'com_github_smacker_go_tree_sitter' requested from @@rules_python_gazelle_plugin~]//python': The repository '@@[unknown repo 'com_github_smacker_go_tree_sitter' requested from @@rules_python_gazelle_plugin~]' could not be resolved: No repository visible as '@com_github_smacker_go_tree_sitter' from repository '@@rules_python_gazelle_plugin~' and referenced by '@@rules_python_gazelle_plugin~//python:python'
ERROR: Analysis of target '//:gazelle' failed; build aborted: Analysis failed
INFO: Elapsed time: 1.437s, Critical Path: 0.51s
INFO: 1 process: 1 internal.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

Here's something weird. If I run bazel sync in my rules_pyle/gazelle directory it applies patches to go-tree-sitter:

DEBUG: /usr/local/google/home/maffoo/.cache/bazel/_bazel_maffoo/051c217dd68266c8e1eaa3206da93fe4/external/gazelle~/internal/go_repository.bzl:345:10: bazel_gazelle/go_repository: applying patches. name=com_github_smacker_go_tree_sitter, patches=[Label("//:patches/0001-go-tree-sitter-build.patch")]format_text_overflowformat_text_wrap

but whenever I try to run some other build command I see patches=[], no patches are applied, and the build fails.

I think this is the fundamental problem. The go_repository definitions are only having an effect in the WORKSPACE. The MODULE.bazel uses a different mechanism with the go_deps extension and this doesn't know about the patches we want to apply. There's a way to specify patches for repositories brought in with go_deps but it only works in the root module so we can't use it (it's fine if running bazel in rules_python/gazelle, but when we pull this in as a dependency to pyle the patches get ignored again. The only way I can see to fix this is to update bazel-gazelle itself or add go-tree-sitter to the bazel central registry.


maybe we can publish go-tree-sitter with patches to bazel registry?

It's looking like we might have to do that. We can apply patches like @maffoo has done, but then those patches won't be used when some other project uses rules_python_gazelle_plugin because things like go_deps.module_override and will only work on the root module.

@hunshcn How do you recommend we publish to BCR? Fork go-tree-sitter, add MODULE.bazel and rules, and go from there? Or is there another option?

@hunshcn
Copy link
Contributor

hunshcn commented Nov 21, 2024

Just like bazelbuild/bazel-central-registry#674
Maybe @fmeum can provide some information about.

@dougthor42
Copy link
Collaborator

Ah thanks, that process looks pretty easy. I'll start it now.

@maffoo
Copy link
Contributor Author

maffoo commented Nov 22, 2024

I filed bazel-contrib/bazel-gazelle#1984 to propose allowing default patches on libraries included by go_deps and put up bazel-contrib/bazel-gazelle#1985 to actually implement this. We'll see what the bazel-gazelle maintainers say...

@ewianda
Copy link
Contributor

ewianda commented Dec 8, 2024

Will the BCR option not be limited to the bzlmod implementation?

I have a fork of the gazelle plugin and made this upgrade using http_archive approach, which is compatible with MODULE.bazel and WORKSPACE.

https://github.com/benchsci/rules_python_gazelle/blob/8b5c75b46bcf270bd7360c71daaa44e2c7a01fdd/MODULE.bazel#L14-L23

benchsci/rules_python_gazelle@8b5c75b#diff-7ad5c39e688ac3ccc3847bdf065963457ec66024308f03dcf1045cafa2c38108

I can contribute the change here.

@maffoo
Copy link
Contributor Author

maffoo commented Dec 11, 2024

Superseded by #2496.

@maffoo maffoo closed this Dec 11, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 14, 2024
… Parameter Syntax) by using dougthor42's fork of go-tree-sitter (#2496)

Replaces #2413.

Fixes #2396.

This updates the `go-tree-sitter` dependency to use my fork that
includes `BUILD.bazel` files. Specifically, the `BUILD.bazel` files in
the fork include references to top-level code like `array.h` which the
original Gazelle-generated files for `go-tree-sitter` were not able to
handle. I also include the test cases that @maffoo created in #2413 and
verified that they (a) fail before the fix and (b) pass after the fix.

The fork is: https://github.com/dougthor42/go-tree-sitter

The branch that includes all changes is:
https://github.com/dougthor42/go-tree-sitter/tree/for-rules-python-gazelle-plugin

A couple notes:
+ I have a PR open to get `go-tree-sitter` into BCR
[here](bazelbuild/bazel-central-registry#3366).
However:
1. I'm having trouble getting tests to pass and to get things running
locally to validate it
2. Using BCR would not fix things for people who still use WORKSPACE
(right?)
+ The fork is _mostly_ [autogenerated BUILD.bazel files from
gazelle](smacker/go-tree-sitter@cfa9bdf)
but also contains:
+ [manual updates so that build files reference the toplevel `array.h`
and other
files](smacker/go-tree-sitter@63f89cd)
+ [replace all `smacker` with `dougthor42` so that `go build`
works](smacker/go-tree-sitter@8a73cbd)
    + various other more minor things.
+ I was unable to get `go mod edit -replace` to work, so I've just
manually updated `go.mod` and whatnot everywhere. If someone with more
go knowledge has a suggestion I'm happy to hear it.

---------

Co-authored-by: Matthew Neeley <maffoo@google.com>
Co-authored-by: Ignas Anikevicius <240938+aignas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gazelle seems to not like PEP 695 – Type Parameter Syntax
4 participants