Ruby: Merge extractor crates #12510
Ruby: Merge extractor crates #12510
Conversation
This just moves the files, so as not to confuse git when we make changes to them.
Provide more context if a file cannot be opened by the generator.
hmac
force-pushed
the
merge-ruby-extractor
branch
from
040dc66 to
999b12f
Compare
.github/workflows/ruby-build.yml
Outdated
| codeql pack create ql/lib --output target/packs | ||
| codeql pack create -j0 ql/src --output target/packs --compilation-cache "${{ steps.query-cache.outputs.cache-dir }}" | ||
| PACK_FOLDER=$(readlink -f target/packs/codeql/ruby-queries/*) | ||
| rm -rf query-packs |
There was a problem hiding this comment.
The reason for using the target folder is to avoid interference with any folder that may be present in the repository. We don't have a query-packs folder, so it is fine for now, but we create one in the future,
There was a problem hiding this comment.
Hmm, that's a good point. Do you have a better suggestion? The target directory no longer exists here, since it has moved to extractor/target. Could we use a subdirectory of /tmp instead?
There was a problem hiding this comment.
Using ${{ runner.temp }} is probably best. That is like /tmp but for Actions, and also works on Windows.
There was a problem hiding this comment.
Tackled in eefc486. I'm going to try to re-run the CI job without cache to test that part of the code.
| cargo build --release | ||
| cd .. | ||
|
|
||
| extractor\target\release\generator --dbscheme ql/lib/ruby.dbscheme --library ql/lib/codeql/ruby/ast/internal/TreeSitter.qll |
There was a problem hiding this comment.
What is the reason for replacing cargo run with an explicit path?
| @@ -13,14 +13,14 @@ else | |||
| exit 1 | |||
| fi | |||
|
|
|||
| "$CARGO" build --release | |||
| (cd extractor && "$CARGO" build --release) | |||
| extractor/target/release/generator --dbscheme ql/lib/ruby.dbscheme --library ql/lib/codeql/ruby/ast/internal/TreeSitter.qll | |||
There was a problem hiding this comment.
What is the reason for replacing cargo run with an explicit path? This used to run cross run ... in case $CARGO=cross, not sure if it matters, but I wondered why it was changed.
There was a problem hiding this comment.
I changed this because it stopped working after my changes. I was seeing "No such file or directory" errors in CI. My guess as to why is because cross runs commands in a Docker container, and so when we moved the extractor to be inside ./extractor, the QL files in the parent directory were no longer accessible from the container.
There was a problem hiding this comment.
Ok, makes sense. I don't really mind, but I was curious ;-)
This ensures we don't clobber any existing directories in the repo.
This is primarily to bust the actions cache, to test a change in the ruby-build workflow.
|
@aibaars this is ready for another look when you have a chance |
Co-authored-by: Arthur Baars <aibaars@github.com>
This is the first step in making the extractor into a reusable library.
We merge the four crates
extractor,generator,autobuilder,node-typesinto one crate:extractor.If you review commit-by-commit, it's easy to see that most files are not modified, just renamed.