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

Make rand an optional dependency #674

Merged
merged 1 commit into from
Aug 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -327,12 +327,14 @@ jobs:
rustup override set ${{ matrix.rust }}
rustup component add rustfmt
rustup target add wasm32-unknown-unknown
rustup target add wasm32-wasi
- name: Build arrow crate
run: |
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd arrow
cargo build --features=js --target wasm32-unknown-unknown
cargo build --no-default-features --features=csv,ipc,simd --target wasm32-unknown-unknown
cargo build --no-default-features --features=csv,ipc,simd --target wasm32-wasi
Copy link
Contributor Author

@roee88 roee88 Aug 8, 2021

Choose a reason for hiding this comment

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

Specifically for wasm32-wasi there is no real need to exclude test dependencies so cargo build --features=simd --target wasm32-wasi or cargo build --target wasm32-wasi also work. However, I don't see a real reason to include it.


# test builds with various feature flags
default-build:
Expand Down
13 changes: 6 additions & 7 deletions arrow/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,7 @@ serde = { version = "1.0", features = ["rc"] }
serde_derive = "1.0"
serde_json = { version = "1.0", features = ["preserve_order"] }
indexmap = "1.6"
rand = { version = "0.8", default-features = false }
# getrandom is a dependency of rand, not (directly) of arrow
# need to specify `js` feature to build on wasm
getrandom = { version = "0.2", optional = true }
rand = { version = "0.8", optional = true }
num = "0.4"
csv_crate = { version = "1.1", optional = true, package="csv" }
regex = "1.3"
Expand All @@ -64,16 +61,18 @@ csv = ["csv_crate"]
ipc = ["flatbuffers"]
simd = ["packed_simd"]
prettyprint = ["prettytable-rs"]
js = ["getrandom/js"]
# The test utils feature enables code used in benchmarks and tests but
# not the core arrow code itself
test_utils = ["rand/std", "rand/std_rng"]
# not the core arrow code itself. Be aware that `rand` must be kept as
# an optional dependency for supporting compile to wasm32-unknown-unknown
# target without assuming an environment containing JavaScript.
test_utils = ["rand"]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

# this is only intended to be used in single-threaded programs: it verifies that
# all allocated memory is being released (no memory leaks).
# See README for details
memory-check = []

[dev-dependencies]
rand = "0.8"
criterion = "0.3"
flate2 = "1"
tempfile = "3"
Expand Down
6 changes: 4 additions & 2 deletions arrow/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,13 @@ println!("{:?}", array.value(1));

## Building for WASM

In order to compile Arrow for Web Assembly (the `wasm32-unknown-unknown` WASM target), you will likely need to turn off this crate's default features and use the `js` feature.
Arrow can compile to WebAssembly using the `wasm32-unknown-unknown` and `wasm32-wasi` targets.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI wasm32-unknown-emscripten is not supported because of chronotope/chrono#519

Copy link
Contributor

Choose a reason for hiding this comment

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

We can accommodate it when there's demand


In order to compile Arrow for `wasm32-unknown-unknown` you will need to disable default features, then include the desired features, but exclude test dependencies (the `test_utils` feature). For example, use this snippet in your `Cargo.toml`:

```toml
[dependencies]
arrow = { version = "5.0", default-features = false, features = ["js"] }
arrow = { version = "5.0", default-features = false, features = ["csv", "ipc", "simd"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't clearly articulate what's happening, compared to what's being changed. How about:

"In order to compile Arrow for wasm32-unknown-unknown you will need to disable default features, then include the desired features, but exclude test dependencies (the test_utils feature). For example, use this snippet in your Cargo.toml:"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated using your suggested text

```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

side note

When compiling to wasm32-wasi you can also enable the prettyprint feature if you patch prettytable-rs in your Cargo.toml:

[patch.crates-io]
prettytable-rs = { git = "https://github.com/phsym/prettytable-rs", branch = "master"}

This is mentioned in polars but I'm not sure if this fits the arrow README so I didn't include it. The arrow2 project recently changed to comfy-table because prettytable-rs is not maintained (see jorgecarleitao/arrow2#251). I don't think that comfy-table compiles to wasm32-wasi but that's minor for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also #656 from @PsiACE -- I am not sure if the same applies to comfy-table

## Examples
Expand Down