Move tree-sitter-extrator project to ruby/extractor/codeql-extractor.
#15595
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…or`. I investigated porting our ruby extractor build to bazel, and unfortunately, bazels `rules_rust` only support local dependencies inside a workspace, c.f. bazelbuild/rules_rust#1525. Therefore, to be able to build the ruby extractor with bazel, I had to move the `tree-sitter-extractor` project out of `shared/`, where it morally belongs, into the ruby extractor workspace. Alternatives considered: - Provide a symlink from inside the workspace into `shared/`. This is possible, but due to the poor symlink support on Windows, it'd make working with the entire codeql repo on Windows more difficult. Our internal developers all (have to) have developer mode enabled, so that we can use symlinks in Bazel, but all external contributers on Windows would presumably run into trouble while cloning the repo. Imo, that's not acceptable. - Fake an environment that looks like it's a workspace using advanced bazel magic. That's possible, but would make the build much harder to understand. In particular, bazel doesn't (easily) allow writing to the source tree, so this is not an easy/quick hack. It's important to me that the build system can be understood by non-bazel experts, so this isn't acceptable either. - Fix the upstream issue. I don't have a good enough understanding of the issue or the code to do that. - Drop the `Cargo.toml` for the ruby extractor, and model all dependencies only on the bazel side. This'd break IDE support, and the bazel<->cargo integration is actually quite good (besides its limitations), so I don't want to do that. Out of all these alternatives, moving the shared code into the ruby workspace seemed the least bad option. Note that this works, because we want to build the ruby language pack with bazel, but not the ql-for-ql one - for now, I don't see an advantage of porting that to bazel anyways, so it's okay that it's blocked from being ported to bazel right now. I'm happy to discuss this change and alternatives further. If we agree that this is the way forward, then this needs to be merged together with a simple corresponding change in the internal repo.
|
Another alternative is presumably to publish |
|
What about other users of the shared extractor? |
aibaars
requested changes
Feb 13, 2024
|
@hmac Ah yes, publishing it, or moving it to another public repo are other options. @aibaars There are no other uses of the shared code so far, python doesn't use it, and we have (as of now) no other tree-sitter extractors. |
|
After a long slack conversation with @aibaars , he came up with the idea of specifying the dependency on the shared extractor as I'll investigate this further, and if it doesn't work out, look at the crates.io and separate repo options instead of moving the extractor. |
For future reference, QL-for-QL also uses it: https://github.com/github/codeql/blob/main/ql/extractor/Cargo.toml#L20 |
criemen
added a commit
that referenced
this pull request
Feb 19, 2024
This PR introduces a bazel and `rules_rust`-based build system for the ruby extractor and language pack. This replacese the existing, `cargo` and `cross`-based build system. For local development, nothing changes, and the existing `cargo`-based build still keeps working as-is. We no longer need to use `cross` to compile our Linux binaries, as we now can link against our hermetic C++ toolchain, which ships with an old enough glibc, so that we don't run into symbol version issues when deploying the binaries to older systems. Besides the one change in dependency (explained in detail in `Cargo.toml` and in #15595), nothing ought to change in how we build the extractor.
criemen
added a commit
that referenced
this pull request
Feb 20, 2024
This PR introduces a bazel and `rules_rust`-based build system for the ruby extractor and language pack. This replacese the existing, `cargo` and `cross`-based build system. For local development, nothing changes, and the existing `cargo`-based build still keeps working as-is. We no longer need to use `cross` to compile our Linux binaries, as we now can link against our hermetic C++ toolchain, which ships with an old enough glibc, so that we don't run into symbol version issues when deploying the binaries to older systems. Besides the one change in dependency (explained in detail in `Cargo.toml` and in #15595), nothing ought to change in how we build the extractor.
criemen
added a commit
that referenced
this pull request
Feb 20, 2024
This PR introduces a bazel and `rules_rust`-based build system for the ruby extractor and language pack. This replacese the existing, `cargo` and `cross`-based build system. For local development, nothing changes, and the existing `cargo`-based build still keeps working as-is. We no longer need to use `cross` to compile our Linux binaries, as we now can link against our hermetic C++ toolchain, which ships with an old enough glibc, so that we don't run into symbol version issues when deploying the binaries to older systems. Besides the one change in dependency (explained in detail in `Cargo.toml` and in #15595), nothing ought to change in how we build the extractor.
criemen
added a commit
that referenced
this pull request
Feb 20, 2024
This PR introduces a bazel and `rules_rust`-based build system for the ruby extractor and language pack. This replacese the existing, `cargo` and `cross`-based build system. For local development, nothing changes, and the existing `cargo`-based build still keeps working as-is. We no longer need to use `cross` to compile our Linux binaries, as we now can link against our hermetic C++ toolchain, which ships with an old enough glibc, so that we don't run into symbol version issues when deploying the binaries to older systems. Besides the one change in dependency (explained in detail in `Cargo.toml` and in #15595), nothing ought to change in how we build the extractor.
criemen
added a commit
that referenced
this pull request
Feb 23, 2024
This PR introduces a bazel and `rules_rust`-based build system for the ruby extractor and language pack. This replacese the existing, `cargo` and `cross`-based build system. For local development, nothing changes, and the existing `cargo`-based build still keeps working as-is. We no longer need to use `cross` to compile our Linux binaries, as we now can link against our hermetic C++ toolchain, which ships with an old enough glibc, so that we don't run into symbol version issues when deploying the binaries to older systems. Besides the one change in dependency (explained in detail in `Cargo.toml` and in #15595), nothing ought to change in how we build the extractor.
criemen
added a commit
that referenced
this pull request
Feb 26, 2024
This PR introduces a bazel and `rules_rust`-based build system for the ruby extractor and language pack. This replacese the existing, `cargo` and `cross`-based build system. For local development, nothing changes, and the existing `cargo`-based build still keeps working as-is. We no longer need to use `cross` to compile our Linux binaries, as we now can link against our hermetic C++ toolchain, which ships with an old enough glibc, so that we don't run into symbol version issues when deploying the binaries to older systems. Besides the one change in dependency (explained in detail in `Cargo.toml` and in #15595), nothing ought to change in how we build the extractor.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I investigated porting our ruby extractor build to bazel, and unfortunately, bazels
rules_rustonly support local dependencies inside a workspace, c.f. bazelbuild/rules_rust#1525.Therefore, to be able to build the ruby extractor with bazel, I had to move the
tree-sitter-extractorproject out ofshared/, where it morally belongs, into the ruby extractor workspace. Alternatives considered:shared/. This is possible, but due to the poor symlink support on Windows, it'd make working with the entire codeql repo on Windows more difficult. Our internal developers all (have to) have developer mode enabled, so that we can use symlinks in Bazel, but all external contributers on Windows would presumably run into trouble while cloning the repo. Imo, that's not acceptable.Cargo.tomlfor the ruby extractor, and model all dependencies only on the bazel side. This'd break IDE support, and the bazel<->cargo integration is actually quite good (besides its limitations), so I don't want to do that.Out of all these alternatives, moving the shared code into the ruby workspace seemed the least bad option.
Note that this works, because we want to build the ruby language pack with bazel, but not the ql-for-ql one - for now, I don't see an advantage of porting that to bazel anyways, so it's okay that it's blocked from being ported to bazel right now.
I'm happy to discuss this change and alternatives further. If we agree that this is the way forward, then this needs to be merged together with a simple corresponding change in the internal repo.