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

Introduce a shared extractor library #12546

Merged
merged 12 commits into from
Mar 27, 2023
Merged

Conversation

hmac
Copy link
Contributor

@hmac hmac commented Mar 16, 2023

Move most of the common code between the Ruby and QL extractors into a library at shared/extractor.

@hmac hmac force-pushed the extractor-shared-library branch 2 times, most recently from efc8b34 to 3829454 Compare March 17, 2023 01:08
This makes it possible for different languages to share this extractor.
This mirrors the structure we have in the Ruby extractor, and will allow
us to share more code.
Ensure that builds via cargo-cross, which are executed in a docker
container, can see the shared library.
This include support for mounting external path dependencies as volumes.
@hmac hmac force-pushed the extractor-shared-library branch 9 times, most recently from 0fc65dc to 24baf64 Compare March 24, 2023 02:02
@hmac hmac added the no-change-note-required This PR does not need a change note label Mar 24, 2023
@hmac hmac marked this pull request as ready for review March 24, 2023 08:26
@hmac hmac requested a review from a team as a code owner March 24, 2023 08:26
@hmac hmac requested a review from a team as a code owner March 24, 2023 08:26
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me. A couple of suggestions.

@@ -60,8 +60,10 @@ jobs:
path: |
ruby/extractor/target/release/autobuilder
ruby/extractor/target/release/autobuilder.exe
ruby/extractor/target/x86_64-unknown-linux-gnu/release/autobuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed? It looks like you are mv-ing those paths when building the extractor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot. I've removed these paths.

run: cd extractor && cross build --release
run: |
cd extractor
cross build --release
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use --target-dir instead of using mv afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that will work. The default target-dir is ./target. Inside this dir, cross creates the x86_64-unknown-linux-gnu directory.

/// assert_eq!(parse_codeql_threads("-5", 4), Some(1));
/// assert_eq!(parse_codeql_threads("nope", 4), None);
/// ```
pub fn parse_codeql_threads(threads_str: &str, num_cpus: usize) -> Option<usize> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn parse_codeql_threads(threads_str: &str, num_cpus: usize) -> Option<usize> {
fn parse_codeql_threads(threads_str: &str, num_cpus: usize) -> Option<usize> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid we need the pub for the doc tests to work

Copy link
Contributor

Choose a reason for hiding this comment

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

That's just annoying. I quite like the doc tests. Let's leave it.

@@ -14,13 +14,21 @@ else
fi

(cd extractor && "$CARGO" build --release)
extractor/target/release/generator --dbscheme ql/lib/ruby.dbscheme --library ql/lib/codeql/ruby/ast/internal/TreeSitter.qll

# If building via cross, the binaries will be in extractor/target/<triple>/release
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we try to use the --target-dir flag to make cross output the files to the same directory as cargo would?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea - I'll give that a try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this doesn't work - see my other comment for more info.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Should we rather put it in shared/tree-sitter-extractor, to make it clear that is for TreeSitter based extractors?

It is now called `tree-sitter-extractor`, to make it clearer that it
builds on tree-sitter grammars.
@aibaars aibaars merged commit 7e7cd54 into github:main Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note QL-for-QL Ruby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants