refactor: switch to official go tree-sitter implementation#2952
refactor: switch to official go tree-sitter implementation#2952jbedard wants to merge 2 commits intobazel-contrib:mainfrom
Conversation
|
|
||
| go_deps = use_extension("@bazel_gazelle//:extensions.bzl", "go_deps") | ||
| go_deps.from_file(go_mod = "//:go.mod") | ||
| go_deps.module_override( |
There was a problem hiding this comment.
This is, sadly, a blocker because overrides cannot be used in non-root modules.
If this is included, then no one can add the dep via:
# MODULE.bazel
bazel_dep(name = "rules_python_gazelle_plugin")As it will fail with:
Using the "go_deps.{tag_class}" tag in a non-root Bazel module is forbidden
There was a problem hiding this comment.
You can fork that library as well if you want, or ask users to add these 3 lines themselves. I'm just opening this PR to suggest using the modern go-tree-sitter library...
There was a problem hiding this comment.
I'm 100% for using the modern go-tree-sitter library. I just think we are blocked from doing so until bazel-contrib/rules_go#4298 is released and we update our rules_go version.
Otherwise we get stuck in the "users must apply a patch themselves" or "we have to maintain a fork" situation that we're already in.
I think the path forward is:
- Wait for rules_go to release the fix.
- Update rules_go
- Remove the dougthor42/go-tree-sitter hack and go back to smacker/go-tree-sitter
- Migrate to the official tree-sitter repos.
There was a problem hiding this comment.
I don't think bazel-contrib/rules_go#4298 fixes every scenario. After upgrading to rules_go@HEAD I was able to remove my patch to smacker/go-tree-sitter, but when switching to tree-sitter/go-tree-sitter I had to add almost the same patch again 😢
| patch_strip = 1, | ||
| ) | ||
| go_deps.module_override( | ||
| path = "github.com/tree-sitter/tree-sitter-python", |
There was a problem hiding this comment.
One alternative to patching the tree-sitter-* libraries (with the bindings/go/bindings.go file) is generating a single BUILD ourselves like what I've done in the aspect-cli, see the http fetch+BUILD and associated go.
That's probably nicer then a patch now that I look at it again...?
There was a problem hiding this comment.
Using go modules works better when mixing go modules across multiple modules though, such as multiple gazelle languages from different modules. But that requires the patches :/
There was a problem hiding this comment.
Making the BUILD file ourselves is basically what the dougthor42/go-tree-sitter does.
Does manually setting http_archive.build_file_content work with both WORKSPACE and bzlmod? If so that might be a way forward. I agree it's cleaner than the patches.
There was a problem hiding this comment.
This is only for the tree-sitter-{lang}/bindings/go/bindings.go files, not the main go-tree-sitter go module that has the bigger problem.
|
|
||
| go_deps = use_extension("@bazel_gazelle//:extensions.bzl", "go_deps") | ||
| go_deps.from_file(go_mod = "//:go.mod") | ||
| go_deps.module_override( |
There was a problem hiding this comment.
I'm 100% for using the modern go-tree-sitter library. I just think we are blocked from doing so until bazel-contrib/rules_go#4298 is released and we update our rules_go version.
Otherwise we get stuck in the "users must apply a patch themselves" or "we have to maintain a fork" situation that we're already in.
I think the path forward is:
- Wait for rules_go to release the fix.
- Update rules_go
- Remove the dougthor42/go-tree-sitter hack and go back to smacker/go-tree-sitter
- Migrate to the official tree-sitter repos.
| patch_strip = 1, | ||
| ) | ||
| go_deps.module_override( | ||
| path = "github.com/tree-sitter/tree-sitter-python", |
There was a problem hiding this comment.
Making the BUILD file ourselves is basically what the dougthor42/go-tree-sitter does.
Does manually setting http_archive.build_file_content work with both WORKSPACE and bzlmod? If so that might be a way forward. I agree it's cleaner than the patches.
| version = "v0.23.1", | ||
| ) | ||
| go_repository( | ||
| name = "com_github_tree_sitter_tree_sitter_rust", |
There was a problem hiding this comment.
Why does this need all of the non-python languages?
There was a problem hiding this comment.
I don't think it needs any of them, but they are there, I think just for tests AFAICT.
I don't think we should care. That's how go.mod works, and deps.bzl is how (legacy) rules_go works.
|
FWIW I might be blocked on tree-sitter/go-tree-sitter#32 for now so your plan upgrading rules_go and dropping the fork is probably still the next best step. |
This would break other extensions. See bazel-contrib/rules_python#2952
* chore: add python This runs cgo, so we need to add a hermetic cc toolchain and avoid linking against libc * chore: unfork go-tree-sitter This would break other extensions. See bazel-contrib/rules_python#2952
|
Changing this to a draft until we have a larger discussion of this is worth it. I found tree-sitter/go-tree-sitter#32 was too noticeable of a perf hit when changing the js gazelle extension. |
|
ACK, SGTM. |
This would break other extensions. See bazel-contrib/rules_python#2952
* chore: add python This runs cgo, so we need to add a hermetic cc toolchain and avoid linking against libc * chore: unfork go-tree-sitter This would break other extensions. See bazel-contrib/rules_python#2952 * add py file
2 participants
The https://github.com/smacker/go-tree-sitter library seems dead and we should switch to the official https://github.com/tree-sitter/go-tree-sitter. I know there's already a lot of discussion around tree-sitter in
rules_python/gazelle, I thought I should bring this into the discussion as well. This PR depends on even more patches then what https://github.com/dougthor42/go-tree-sitter is patching, I'm not sure how you want to deal with that going forward (I've found the latest rules_go solves 50% of it).The official library has been properly maintained for some time now and has a lot of features that the smacker version is lacking. I was able to delete a decent amount of code in the js/ts gazelle extension when switching (such as all predicate filtering which is properly implemented in the official go-tree-sitter).