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

Trigger CI on PR, as well as push #797

Merged
merged 11 commits into from
Dec 2, 2024
Merged

Trigger CI on PR, as well as push #797

merged 11 commits into from
Dec 2, 2024

Conversation

Xophmeister
Copy link
Member

@Xophmeister Xophmeister commented Nov 28, 2024

Also updated benchmark to use new tree-sitter-ocaml API.

@Xophmeister Xophmeister requested a review from nbacquey November 28, 2024 20:40
@Xophmeister Xophmeister requested a review from yannham November 28, 2024 22:08
@Xophmeister Xophmeister marked this pull request as draft November 29, 2024 08:55
@Xophmeister
Copy link
Member Author

There are still a few broken things... I will fix and resubmit for review.

@Xophmeister
Copy link
Member Author

Xophmeister commented Nov 29, 2024

The problems appear to be with the WASM target, from the changes introduced in #794, if I'm following the log output correctly. It complains about a bunch of stuff in topiary-core/src/tree-sitter.rs:

  • tree_sitter_facade::QueryMatch is a trait for native code, but it's a struct for WASM. It needs to be imported for native, but in WASM we get a dead code warning.
  • tree_sitter_facade::Query::matches returns a streaming_iterator::StreamingIterator implementation in native code, hence needing the trait imported, but an ExactSizeIterator implementation in WASM. Another dead code warning.
  • tree_sitter_facade::QueryMatch::pattern_index returns a usize in native code and a u32 in WASM, so we get downstream type mismatch errors.
  • From<tree_sitter::Point> is not implemented for topiary_tree_sitter_facade::Point for WASM. (From<topiary_web_tree_sitter_sys::Point> is implemented instead, in WASM.)

@Xophmeister
Copy link
Member Author

  • From<tree_sitter::Point> is not implemented for topiary_tree_sitter_facade::Point for WASM. (From<topiary_web_tree_sitter_sys::Point> is implemented instead, in WASM.)

I'm not sure how to solve this one. If I add this to the WASM module in topiary-tree-sitter-facade/src/point.rs:

    impl From<tree_sitter::Point> for Point {
        fn from(point: tree_sitter::Point) -> Self {
            topiary_web_tree_sitter_sys::Point::new(point.row, point.column).into()
        }
    }

The error changes to:

error[E0433]: failed to resolve: use of undeclared crate or module `tree_sitter`
   --> topiary-tree-sitter-facade/src/point.rs:121:15
    |
121 |     impl From<tree_sitter::Point> for Point {
    |               ^^^^^^^^^^^ use of undeclared crate or module `tree_sitter`

This is true and by design...

@Xophmeister
Copy link
Member Author

...fixed that by avoiding the conversion call when targetting WASM.

@Xophmeister Xophmeister marked this pull request as ready for review December 2, 2024 12:32
topiary-core/src/tree_sitter.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Xophmeister Xophmeister merged commit 932cb22 into main Dec 2, 2024
9 checks passed
@Xophmeister Xophmeister deleted the chris/ci/trigger-on-pr branch December 2, 2024 16:56
Comment on lines +4 to +5
pull_request:
branches: main
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any reason not to run CI on pull requests which don't target master. Especially since there's this playground branch that I believe we still have, which is also in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants