-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
ARROW-10838: [Rust] [CI] Add arrow build targeting wasm32 #8864
Conversation
override: true | ||
components: rustfmt | ||
target: wasm32-unknown-unknown | ||
- name: Build arrow crate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could also run some tests? Or maybe that is a follow on step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up, as some tests currently fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorgecarleitao The basic idea is great. 👍
I think some non-wasm32 code has snuck in since this PR was originally written
(note I was confused -- this is in support of #7767)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @jorgecarleitao
FWIW, I do not think we should merge this if there are no plans to merge #7767. |
I think we should merge this given that we don't know when the PR with tests will be ready. |
Are you building from within the The purpose of adding just the We have partial wasm32 support in I've just tested compiling with the wasm32 target in both Windows and Linux via WSL, and the If you currently try to run tests without the magic sauce that's in #7767, you get: $ cargo test --target wasm32-unknown-unknown -p arrow
arrow/rust/target/wasm32-unknown-unknown/debug/deps/arrow-2495d35fed1d3bb9.wasm: 1: Syntax error: end of file unexpected
error: test failed, to rerun pass '--lib' |
@nevi-me -- thank you. You are right -- I was not building with In that case, I agree that this would be a good PR to merge to make sure people don't accidentally introduce new code in arrow that breaks the wasm build . What do you think @jorgecarleitao ? |
I agree with @nevi-me , that this is useful to avoid regressions in being able to compile wasm32. Let's merge this then. |
This is a requirement fielded to merge apache#7767, and so I though about adding it to our CI. I am not sure whether this is exactly what we need to test apache#7767, and so I would like to request help from @paddyhoran and @rj-atw here. NOTE: this is built on top of apache#8821 . Only the last commit is relevant. Basically, this runs ``` rustup add target wasm32-unknown-unknown cd rust/arrow cargo build --target wasm32-unknown-unknown ``` Note that this does not actually run the tests yet. I think that that is apache#7767 's goal. Closes apache#8864 from jorgecarleitao/wasm32 Authored-by: Jorge C. Leitao <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>
This is a requirement fielded to merge #7767, and so I though about adding it to our CI.
I am not sure whether this is exactly what we need to test #7767, and so I would like to request help from @paddyhoran and @rj-atw here.
NOTE: this is built on top of #8821 . Only the last commit is relevant. Basically, this runs
Note that this does not actually run the tests yet. I think that that is #7767 's goal.