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

Migrate tree-sitter support to MODULE.bazel #4783

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Jan 9, 2025

The WORKSPACE file is deprecated; support is already off by default, and it'll be removed in the next major bazel release. Our main dependency is tree-sitter, and I'm trying to address that here.

We're currently using https://github.com/elliottt/rules_tree_sitter, but that hasn't been updated in a couple years, meaning it lacks MODULE.bazel support. In the registry, there's https://registry.bazel.build/modules/tree-sitter-bazel, but this is only the parser libraries of tree-sitter, not the generator. I'm using it for that much, at least.

For the generator, which transforms grammar.js to parser.c/h, I'm just requiring a non-hermetic invocation (i.e., people who want to work on it will need to install tree-sitter; see the README.md updates). I tried running it manually, but parser.c is about 600 KB; pre-commit rejects files that large and I don't think an exception makes sense to override for this (it'd probably also grow substantially if the grammar were updated to cover more syntax). In order to make the non-hermetic call not break "bazel build //..." for most developers, I'm marking most targets in the package as manual.

Note, I did look long and hard at using aspect_rules_js/rules_nodejs to invoke npm. This took a lot of time, and I have a commit that's mostly working, except I hit a point where it uses declare_symlink which we disallow for compatibility reasons (commit "Lots of work for figuring out rule_js uses declare_symlink" on the PR). As a consequence, I think we can't use the primary supported ways to have hermetic npm calls.

Also, treesitter -> tree_sitter because it's generally called tree-sitter, two words. We even had a treesitter/src/tree_sitter directory so it's a bit inconsistent.

As far as bugs here, the parser library breaks bazel queries, e.g. the error:

ERROR: Evaluation of query "somepath(//..., @llvm-project//third-party/unittest:gtest)" failed: preloading transitive closure failed: no such package '@@[unknown repo 'platforms' requested from @@tree-sitter-bazel+]//': The repository '@@[unknown repo 'platforms' requested from @@tree-sitter-bazel+]' could not be resolved: No repository visible as '@platforms' from repository '@@tree-sitter-bazel+'

I'm just excluding tree_sitter from queries where I can to work around the error.

@github-actions github-actions bot requested a review from zygoloid January 9, 2025 21:05
@github-actions github-actions bot added documentation An issue or proposed change to our documentation explorer Action items related to Carbon explorer code infrastructure utilities Utilities for working with Carbon code: syntax highlighting, editor plugins, etc. labels Jan 9, 2025
@jonmeow
Copy link
Contributor Author

jonmeow commented Jan 9, 2025

Note I split out the woff2 removal to #4781; this depends on it to remove the WORKSPACE file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation An issue or proposed change to our documentation explorer Action items related to Carbon explorer code infrastructure utilities Utilities for working with Carbon code: syntax highlighting, editor plugins, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant