Skip to content

ARROW-10838: [Rust] [CI] Add arrow build targeting wasm32 - #8864

Closed
jorgecarleitao wants to merge 1 commit into
apache:masterfrom
jorgecarleitao:wasm32
Closed

ARROW-10838: [Rust] [CI] Add arrow build targeting wasm32#8864
jorgecarleitao wants to merge 1 commit into
apache:masterfrom
jorgecarleitao:wasm32

Conversation

@jorgecarleitao

Copy link
Copy Markdown
Member

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

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 #7767 's goal.

@github-actions

Copy link
Copy Markdown

Comment thread.github/workflows/rust.yml Outdated

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor

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

@github-actionsgithub-actionsBot added the needs-rebase A PR that needs to be rebased by the author label Dec 8, 2020
@github-actionsgithub-actionsBot removed the needs-rebase A PR that needs to be rebased by the author label Dec 8, 2020

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

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)

@nevi-menevi-me left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@paddyhoranpaddyhoran left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @jorgecarleitao

@jorgecarleitao

Copy link
Copy Markdown
MemberAuthor

FWIW, I do not think we should merge this if there are no plans to merge #7767.

@jorgecarleitao
jorgecarleitao marked this pull request as draft December 16, 2020 14:00
@nevi-me

Copy link
Copy Markdown
Contributor

I think we should merge this given that we don't know when the PR with tests will be ready.

@alamb

Copy link
Copy Markdown
Contributor

I don't fully understand what this check is doing

I see it did something which claims to have run cargo build --target wasm32-unknown-unknown:
Screen Shot 2020-12-16 at 4 34 47 PM

But it takes a suspiciously short amount of time (8s) and when I do (seemingly) the same thing locally I get a bunch of compilation errors

 Compiling encode_unicode v0.3.6
error[E0433]: failed to resolve: could not find `unix` in `os`
--> /Users/alamb/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-1.0.5/src/lin.rs:41:18
|
41 | use std::os::unix::ffi::OsStringExt;
| ^^^^ could not find `unix` in `os`
error[E0432]: unresolved import `unix`
--> /Users/alamb/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-1.0.5/src/lin.rs:6:5
|
6 | use unix;
| ^^^^ no `unix` in the root
Compiling hex v0.4.2
Compiling tower-service v0.3.0
Compiling alloc-no-stdlib v2.0.1
Compiling adler32 v1.0.4
error[E0599]: no function or associated item named `from_vec` found for struct `OsString` in the current scope
--> /Users/alamb/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-1.0.5/src/lin.rs:48:34
|
48 | Some(PathBuf::from(OsString::from_vec(out)))
| ^^^^^^^^ function or associated item not found in `OsString`
|
= help: items from traits can only be used if the trait is in scope
= note: the following trait is implemented but not in scope; perhaps add a `use` for it:
`use std::sys_common::os_str_bytes::OsStringExt;`
error: aborting due to 3 previous errors
Some errors have detailed explanations: E0432, E0433, E0599.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `dirs`
To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed

I probably don't have something installed right

@nevi-me

Copy link
Copy Markdown
Contributor

Are you building from within the arrow directory, or using -p arrow?

The purpose of adding just the cargo build (or even cargo check) to CI is to prevent making changes (normally feature-gates in SIMD) that break wasm32-unknown-unknown targets.

We have partial wasm32 support in arrow, in the sense that the crate can be built, but not all tests pass, and it's likely that std::fs-related code + some other stuff don't work.
So we'd want to at least test that we don't break support, see ARROW-9088.

I've just tested compiling with the wasm32 target in both Windows and Linux via WSL, and the arrow crate builds. I'm assuming that the 8 seconds taken is due to the build artefacts being cached from some previous build, but @jorgecarleitao would be authoritative in answering that.

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'

@alamb

Copy link
Copy Markdown
Contributor

@nevi-me -- thank you. You are right -- I was not building with -p arrow (so presumably the build errors were from some other crate).

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 ?

@jorgecarleitao

Copy link
Copy Markdown
MemberAuthor

I agree with @nevi-me , that this is useful to avoid regressions in being able to compile wasm32. Let's merge this then.

@jorgecarleitao
jorgecarleitao marked this pull request as ready for review December 17, 2020 05:10
@alambalamb closed this in 1a7b692Dec 18, 2020
@jorgecarleitao
jorgecarleitao deleted the wasm32 branch March 6, 2021 20:24
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorgecarleitao@nevi-me@alamb@paddyhoran