Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Reduced dependencies from confi-table and enabled wasm on io_print feature. #276

Merged
merged 2 commits into from
Aug 11, 2021
Merged

Reduced dependencies from confi-table and enabled wasm on io_print feature. #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
783e72f
Removed default features of comfy-table
jorgecarleitao Aug 11, 2021
39d5f60
Added io_print to wasm testing.
jorgecarleitao Aug 11, 2021
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
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
# no need
cargo build --no-default-features --features=merge_sort,io_ipc,io_csv,io_json,io_parquet --target wasm32-unknown-unknown
cargo build --no-default-features --features=merge_sort,io_ipc,io_csv,io_print,io_json,io_parquet --target wasm32-unknown-unknown
Copy link

@roee88 roee88 Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I doubt wasm32-unknown-unknown will work. Only wasm32-wasi works with stdio.
Edit: I stand corrected it seems to pass CI, no idea how but I'll check it later.

Copy link
Owner Author

Choose a reason for hiding this comment

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

it worked :/

Copy link

@roee88 roee88 Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It appears like the approach in rust is to throw unsupported operation errors. See https://github.com/rust-lang/rust/tree/master/library/std/src/sys.

So it compiles but any code path that actually uses it will fail unless the user does some extra low level work (unlike in wasm32-wasi where it's actually implemented out of the box).

My personal opinion it that it's perfectly fine to just ensure that it compiles and users would need to ensure that their specific usage is using only valid execution paths. It's just that having this specific feature in the CI targeting wasm32-unknown-unknown might mislead users. That's minor though.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks a lot for the investigation!

What is the specific path that would fail? the usage of println!?

Copy link

Choose a reason for hiding this comment

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

So I wrote a little sample and it all works perfectly fine:

  • The arrow2::io::print::write function works as expected
  • The arrow2::io::print::print function doesn't print anything in wasm32-unknown-unknown but it doesn't fail nor panics.

We're good here. Thanks for the opportunity to learn something new ;)

Copy link
Owner Author

@jorgecarleitao jorgecarleitao Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you. I really do not understand wasm, so every opportunity for a technical discussion about it is awesome :)

maybe we should deprecate arrow2::io::print::print, since it is basically just println!("{}", write(...)?), anyways.


linux-simd-test:
name: SIMD
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ serde_json = { version = "^1.0", features = ["preserve_order"], optional = true
indexmap = { version = "^1.6", optional = true }

# used to print columns in a nice columnar format
comfy-table = { version = "4.0", optional = true }
comfy-table = { version = "4.0", optional = true, default-features = false }

flatbuffers = { version = "=2.0.0", optional = true }
hex = { version = "^0.4", optional = true }
Expand Down