fix(rust): fix build for wasm #9502

lorepozo · 2023-06-22T13:24:08Z

Fixes issue #8783, where v0.29 broke the build for wasm. In addition to fixing the build, this adds a CI test to prevent regressions like this in the future. I've gone ahead and added every feature that works with wasm (not all features work out of the box and adding support for those can be done in the future as needed).

Relatedly there's an issue for the home crate rust-lang/cargo#12297 to make it compile for wasm.

lorepozo · 2023-06-23T16:12:29Z

@ritchie46 would you review this change? Build failures are unrelated (probably just need to rebase)

universalmind303 · 2023-06-23T21:02:59Z

polars/polars-io/src/utils.rs

 pub fn resolve_homedir(path: &Path) -> PathBuf {
    // replace "~" with home directory
    if path.starts_with("~") {
-        if let Some(homedir) = home_dir() {
+        // home crate does not compile on wasm https://github.com/rust-lang/cargo/issues/12297
+        #[cfg(not(target_arch = "wasm32"))]
+        if let Some(homedir) = home::home_dir() {
            return homedir.join(path.strip_prefix("~").unwrap());
        }
    }


this is probably fine if we are just concerned with compiling, but since WASM doesn't have a filesystem, i'm wondering if resolve_homedir should be feature flagged instead? Likely anything that calls resolve_homedir will panic on wasm32 anyways.

With WASI it could make sense for wasm to have a file system, and even a home dir. And generally this function can be used with paths that don't need homedir at all.

Seeing as it's likely the upstream will make this compile and return None on wasm rust-lang/cargo#12297, my proposed change will match that behavior and be safe to remove without consequences on behavior. This is also a reason not to add a feature flag. Ultimately that means we will fix the regression without changing behavior.

.github/workflows/test-rust.yml

lorepozo · 2023-07-19T20:24:55Z

@ritchie46 — would you please review this? I'm stuck on 0.28 because the build has been broken for wasm.

ritchie46 · 2023-07-20T08:25:19Z

.github/workflows/test-rust.yml

+        - name: Wasm
+          target: wasm32-unknown-unknown
+          features: >
+            -F abs


Not that familiar with this. What does -F mean in this context?

this is used further down on line 226 — it specifies a feature:

cargo build --target ${{ matrix.target }} -p polars --no-default-features ${{ matrix.features }}

ritchie46 · 2023-07-20T08:26:16Z

polars/polars-utils/src/wasm.rs

@@ -5,6 +5,14 @@ impl Pool {
        rayon::current_num_threads()
    }

+    pub fn current_thread_index(&self) -> Option<usize> {


Why was this added?

Adding this was necessary to build on wasm.

For context, crate::POOL is configured to a custom struct when building on wasm rather than the corresponding lazily-built rayon::ThreadPool.

In particular, this method is used in polars-core/src/frame/groupby/hashing.rs and polars-core/src/series/implementations/struct_.rs.

lorepozo · 2023-07-31T14:51:03Z

Just rebased, no manual changes were necessary. @ritchie46 please take another look! Would like to fix the regression from 0.28.

lorepozo · 2023-08-08T11:43:40Z

rebased to deal with conflict from #10279 in crates/polars-io/Cargo.toml

Fixes issue #8783, where v0.29 broke the build for wasm. In addition to fixing the build, this adds a CI test to prevent regressions like this in the future. Relatedly there's an issue for the `home` crate rust-lang/cargo#12297 to make it compile for wasm. The previously used crate there, `dirs`, _does_ support wasm (just returning None for the home dir).

lorepozo · 2023-08-11T21:02:32Z

Rebased again — @orlp perhaps you can review?

universalmind303

seems reasonable to me.

only suggestion I have is maybe moving the ci stuff to the Makefile so it is easier to build outside of CI as well.

lorepozo · 2023-08-13T13:07:29Z

Thanks! It appears the Makefile is only used for working in py-polars, but the relevant libraries are part of the rust crates. I'm happy to put the ci stuff in a more appropriate place — I think the next best place to list out the features would be in Cargo.toml as a new feature like wasm-all, but that would add to the public API of the crate for the purposes of testing, which is usually a pattern to avoid. So I'm leaning towards keeping it in CI. I'm open to other ideas if there's another good place! (Or if you think a feature like wasm-all would be preferable to keeping the list in CI, I can move it there)

orlp · 2023-08-14T10:28:42Z

@lorepozo

Thanks! It appears the Makefile is only used for working in py-polars, but the relevant libraries are part of the rust crates.

There is another Makefile in the crates folder, it can go there.

On another note, is there no other solution than to manually specify an entire list of feature flags? That's rather brittle and needs maintaining forever.

lorepozo · 2023-08-14T11:31:03Z

There are a few other ways that come to mind:

support all features on wasm
have a smaller explicit list of supported features on wasm.
use a regression testing framework that only fails version N+1 if version N worked under the same conditions (target + features).

I haven't looked into the difficulty of the first choice. The second choice risks regressions when other features lose support, and is hence tied to a judgement call I'd rather not make. I don't know of solutions for the third choice, though it seems technically solvable with the limited scope of cargo features across targets.

And just pushed the change for the Makefile.

lorepozo · 2023-08-14T11:38:07Z

Actually, another option is to have a list of excludes — features that are not supported on wasm. That list would be smaller and more precisely capture where the problems exist. I'll try making that change now.

lorepozo · 2023-08-14T12:03:05Z

Nice, it looks much better.

In case you're curious, most of the build failures are coming from mio.

lorepozo · 2023-08-14T13:39:00Z

I can submit a small PR after this which will fix many of the mio issues so that dtype-full works on wasm. The problem stems from polars-pipe unconditionally depends on ipc/async of polars-io.

lorepozo · 2023-08-16T11:54:42Z

@ritchie46 looks like this is ready to go — could you merge?

orlp · 2023-08-16T15:27:41Z

That's a lot of excludes, but the solution to that isn't part of this pull request. Would love pull requests to reduce the amount of WASM-incompatible features.

This reverts commit be9ebb8.

orlp · 2023-08-16T16:16:33Z

@lorepozo I am reverting this because it broke our CI.

You can view the error log here: https://github.com/pola-rs/polars/actions/runs/5880942481/job/15948348046?pr=10388.

Could you make a new pull request, and test it first in CI on a fork?

lorepozo · 2023-08-16T16:26:41Z

It was tested correctly in CI via this PR, looks like a merge race because of #10492 breaking the decompress feature on wasm. I'll create a new PR with that feature excluded as well.

lorepozo requested review from ritchie46 and stinodego as code owners June 22, 2023 13:24

github-actions bot added fix Bug fix rust Related to Rust Polars labels Jun 22, 2023

universalmind303 reviewed Jun 23, 2023

View reviewed changes

highway900 reviewed Jul 4, 2023

View reviewed changes

.github/workflows/test-rust.yml Outdated Show resolved Hide resolved

ritchie46 reviewed Jul 20, 2023

View reviewed changes

lorepozo requested a review from orlp as a code owner August 8, 2023 11:42

universalmind303 approved these changes Aug 13, 2023

View reviewed changes

move wasm check to Makefile

52d8a49

use excluded features when checking wasm

afd30de

update github workflow steps for check-wasm

5c5bbbb

orlp changed the title ~~fix(rust) fix build for wasm~~ fix(rust): fix build for wasm Aug 16, 2023

orlp merged commit be9ebb8 into pola-rs:main Aug 16, 2023
18 checks passed

orlp added a commit that referenced this pull request Aug 16, 2023

Revert "fix(rust): fix build for wasm (#9502)"

29d9350

This reverts commit be9ebb8.

orlp added a commit that referenced this pull request Aug 16, 2023

revert: fix(rust): fix build for wasm (#9502) (#10535)

1209b6b

lorepozo deleted the wasm branch August 16, 2023 16:28

lorepozo restored the wasm branch August 16, 2023 16:33

lorepozo mentioned this pull request Aug 16, 2023

fix(rust): fix build for wasm #10536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rust): fix build for wasm #9502

fix(rust): fix build for wasm #9502

lorepozo commented Jun 22, 2023

lorepozo commented Jun 23, 2023

universalmind303 Jun 23, 2023

lorepozo Jun 26, 2023

lorepozo commented Jul 19, 2023

ritchie46 Jul 20, 2023

lorepozo Jul 20, 2023

ritchie46 Jul 20, 2023

lorepozo Jul 20, 2023 •

edited

Loading

lorepozo commented Jul 31, 2023

lorepozo commented Aug 8, 2023

lorepozo commented Aug 11, 2023

universalmind303 left a comment

lorepozo commented Aug 13, 2023

orlp commented Aug 14, 2023 •

edited

Loading

lorepozo commented Aug 14, 2023

lorepozo commented Aug 14, 2023

lorepozo commented Aug 14, 2023

lorepozo commented Aug 14, 2023

lorepozo commented Aug 16, 2023

orlp commented Aug 16, 2023

orlp commented Aug 16, 2023

lorepozo commented Aug 16, 2023

fix(rust): fix build for wasm #9502

fix(rust): fix build for wasm #9502

Conversation

lorepozo commented Jun 22, 2023

lorepozo commented Jun 23, 2023

universalmind303 Jun 23, 2023

Choose a reason for hiding this comment

lorepozo Jun 26, 2023

Choose a reason for hiding this comment

lorepozo commented Jul 19, 2023

ritchie46 Jul 20, 2023

Choose a reason for hiding this comment

lorepozo Jul 20, 2023

Choose a reason for hiding this comment

ritchie46 Jul 20, 2023

Choose a reason for hiding this comment

lorepozo Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

lorepozo commented Jul 31, 2023

lorepozo commented Aug 8, 2023

lorepozo commented Aug 11, 2023

universalmind303 left a comment

Choose a reason for hiding this comment

lorepozo commented Aug 13, 2023

orlp commented Aug 14, 2023 • edited Loading

lorepozo commented Aug 14, 2023

lorepozo commented Aug 14, 2023

lorepozo commented Aug 14, 2023

lorepozo commented Aug 14, 2023

lorepozo commented Aug 16, 2023

orlp commented Aug 16, 2023

orlp commented Aug 16, 2023

lorepozo commented Aug 16, 2023

lorepozo Jul 20, 2023 •

edited

Loading

orlp commented Aug 14, 2023 •

edited

Loading