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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

jorgecarleitao
Copy link
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

github-actions bot commented Dec 7, 2020

override: true
components: rustfmt
target: wasm32-unknown-unknown
- name: Build arrow crate
Copy link
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
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-actions github-actions bot added the needs-rebase A PR that needs to be rebased by the author label Dec 8, 2020
@github-actions github-actions bot removed the needs-rebase A PR that needs to be rebased by the author label Dec 8, 2020
Copy link
Contributor

@alamb alamb left a 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)

Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@paddyhoran paddyhoran left a comment

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
Member Author

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
Contributor

nevi-me commented Dec 16, 2020

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

@alamb
Copy link
Contributor

alamb commented Dec 16, 2020

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/jackfan.us.kg-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/jackfan.us.kg-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/jackfan.us.kg-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
Contributor

nevi-me commented Dec 16, 2020

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
Contributor

alamb commented Dec 16, 2020

@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
Member Author

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
@alamb alamb closed this in 1a7b692 Dec 18, 2020
@jorgecarleitao jorgecarleitao deleted the wasm32 branch March 6, 2021 20:24
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
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]>
Sign up for free to 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